PsqlDosMigrator: refactor the connection handling
#5783
Conversation
sphuber
force-pushed
the
fix/psql-dos-migrator-connections
branch
from
5980c28 to
4ec088d
Compare
There was a problem hiding this comment.
thank you @sphuber !
Besides the minor comments on the code, I am wondering:
- is there anything we can test here? is there a way to prove that we're now using "the right number of connections"? maybe using techniques like the memory leak test; looking for all connection objects in memory?
- if I understand correctly, then every migrator instance now (re)uses one connection. can there be scenarios where this also leads to too many connections being opened?
| self._engine = create_sqlalchemy_engine(self.profile.storage_config) | ||
| self._connection = None | ||
|
|
||
| def close(self) -> None: |
There was a problem hiding this comment.
If this method is mainly intended to be used upon destruction of the migrator, you might consider making it a private method.
There was a problem hiding this comment.
But it needs to be called from the outside. In fact, it is the PsqlDosBackend that calls it when it itself has received the close call. The PsqlDosBackend is also the only class the should be calling the PsqlDosMigrator.
aiida/storage/psql_dos/migrator.py
Outdated
| self._connection = None | ||
|
|
||
| self._engine.dispose() | ||
| self._engine = None |
There was a problem hiding this comment.
If I understand correctly, this currently breaks subsequent calls to the connection property.
Perhaps the cleaner way would be to create self._engine not in the constructor, but lazily upon calling self.property.
There was a problem hiding this comment.
True. I was thinking that once you call close the PsqlDosMigrator instance should no longer be used. The same principle applies for the RmqCommunicator of kiwipy for example. But then again, I don't really see a downside to moving the creation of the engine to self.connection as well, so will move it there.
I had thought about this, but I am not sure. We could test at the end of the test suite, but at that point, redundant connections may have already been cleaned. It would still be possible that in certain modes too many connections get opened that would not be caught by the test. Still, maybe some test is better than none. I will think a bit more to see if we can measure the open connections during the test suite.
I don't think so, at least not in "normal" usage. The |
sphuber
force-pushed
the
fix/psql-dos-migrator-connections
branch
from
70a8998 to
d956ef8
Compare
|
I have absolutely no idea why, but as soon as I move the instantiation of the I have no clue whatsoever why this small change causes |
sphuber
force-pushed
the
fix/psql-dos-migrator-connections
branch
2 times, most recently
from
743846b to
ff48704
Compare
The `PsqlDosMigrator` requires a connection to the database for various operations. To this end, it created an `Engine` from the connection parameters provided by the `Profile`, through which a connection was opened in the `_connection_context`. However, this connection was never explicitly closed. Moreover, although the method allowed passing an optional connection that was already open, this was not consistently used in the entire class. This could lead to too many connections being opened to the client resulting in an exception when the limit was reached. Here the class is refactored as follows: * The engine is created in the constructor and assigned to the private attribute `_engine`. * A `connection` property is added that will return an open connection. If the engine hasn't already been constructed or no connection is already open, the engine is constructed and a connection is opened which is then assigned to the `_connection` attribute. * The `close` method is added which when called will close the connection if it was opened and dispose of the engine. This should be called if the caller is done using the migrator and its connections to the database should be cleaned up. * The `_connection_context` context manager is removed and all internal methods now simply call the `connection` property to get an open connection. The `PsqlDosBackend` uses the new connection handling interface of the `PsqlDosMigrator` through the `migrator_context` context manager. Each method that requires a migrator should request it through this method. At the end of the context the `close` method is automatically called on the migrator ensuring its opened connections are properly relinquished.
sphuber
force-pushed
the
fix/psql-dos-migrator-connections
branch
from
2b681de to
6bf08b1
Compare
The
PsqlDosMigratorrequires a connection to the database for various operations. To this end, it created anEnginefrom the connection parameters provided by theProfile, through which a connection was opened in the_connection_context. However, this connection was never explicitly closed. Moreover, although the method allowed passing an optional connection that was already open, this was not consistently used in the entire class. This could lead to too many connections being opened to the client resulting in an exception when the limit was reached.Here the class is refactored as follows:
_engine.connectionproperty is added that will return an open connection. If a connection hasn't already been opened in a previous call, one is opened and assigned to the_connectionattribute.closemethod is added which when called will close the connection if it was opened and dispose of the engine. This should be called if the caller is done using the migrator and its connections to the database should be cleaned up._connection_contextcontext manager is removed and all internal methods now simply call theconnectionproperty to get an open connection.The
PsqlDosBackenduses the new connection handling interface of thePsqlDosMigratorthrough themigrator_contextcontext manager. Each method that requires a migrator should request it through this method. At the end of the context theclosemethod is automatically called on the migrator ensuring its opened connections are properly relinquished.