lareferencia / dspace-stats-collector

Sends repository usage statistics events to Matomo
Other
13 stars 2 forks source link

Nunca se cierra la conexión de la base de datos #11

Closed santit96 closed 3 years ago

santit96 commented 3 years ago

Si bien se abre una conexión a la base de datos a través de sqlalchemist en esta línea de la clase DSpaceDB, esta conexión no se cierra nunca en el programa.

Se debería ejecutar un connection.close() en el momento que ya no se realicen consultas a la bd.

La documentación de pandas dice que el método read_sql, que se usa para realizar las queries a la bd, delega en el usuario el cerrado de la conexión. Y además la documentación de sqlalchemist sugiere no dejar conexiones abiertas cuando se termina el programa, porque, si bien el garbage collector de python en algún momento "barrerá" el pool de conexiones cerrando así cualquier conexión abierta, no es aconsejable confiar en que lo haga y lo mejor es cerrarlas explícitamente.

lmatas commented 3 years ago

Perfecto, vamos a revisar y subsanar eso a la brevedad, gracias por el reporte. Lautaro Matas

El vie, 21 may 2021 a las 18:30, santit96 @.***>) escribió:

Si bien se abre una conexión a la base de datos a través de sqlalchemist en esta línea https://github.com/lareferencia/dspace-stats-collector/blob/074e7640ed4af14dce3f07df120483c8e727d627/dspace_stats_collector/dspacedb.py#L43 de la clase DSpaceDB, esta conexión no se cierra nunca en el programa.

Se debería ejecutar un connection.close() en el momento que ya no se realicen consultas a la bd.

La documentación https://pandas.pydata.org/docs/reference/api/pandas.read_sql.html de pandas dice que el método read_sql, que se usa para realizar las queries a la bd, delega en el usuario el cerrado de la conexión. Y además la documentación https://docs.sqlalchemy.org/en/12/core/connections.html de sqlalchemist sugiere no dejar conexiones abiertas cuando se termina el programa, porque, si bien el garbage collector de python en algún momento "barrerá" el pool de conexiones cerrando así cualquier conexión abierta, no es aconsejable confiar en que lo haga y lo mejor es cerrarlas explícitamente.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/lareferencia/dspace-stats-collector/issues/11, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAH7G6Q5CGIDCUKBUVOJSIDTO2DC3ANCNFSM45JPCETQ .

lareferencia commented 3 years ago

@santit96 Gracias por el review, te cuento que hemos hecho un fix rápido de esto y ya está desplegado. El problema es que hay una única conexión que se abre cuando el colector empieza a funcionar, entonces tuve que hacer una limpieza de recursos antes de terminar de ejecutar. Probablemente hay que cambiar esto para que no deje abierta tanto tiempo una misma conexión ( el collector corre algunos minutos y se cierra, no es un daemon ). Este código fue hecho en colaboración y pasó por varias manos, entonces hay varias cosas que vamos descubriendo y que incluso debemos rediseñar. Muchas gracias por tu ayuda y sos bienvenido a colaborar o marcarnos mejoras cuando desees. Un saludo

santit96 commented 3 years ago

Buenísimo! Que rápido lo resolvieron! Joya, de todas maneras por lo que estuve leyendo de sqlalchemist, cuando se cierra una conexión, por detrás lo que hace es dejar abierta "por las dudas" la conexión real con la base de datos en caso de que se quiera abrir otra, por una cuestión de performance. Luego, si al finalizar el engine no se abrió ninguna otra, recién ahí la cierra. Por lo que quizás sea bueno tener una única conexión abierta durante todo el programa dado que en términos de base de datos termina siendo lo mismo.

Muchas gracias por la invitación a colaborar y la respuesta rápida. Saludos