-
Notifications
You must be signed in to change notification settings - Fork 91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Review Connection/DB closure #181
Conversation
@@ -951,7 +945,7 @@ def _release_resources(self): | |||
c._storage.release() | |||
c._storage = c._normal_storage = None | |||
c._cache = PickleCache(self, 0, 0) | |||
c.transaction_manager = None | |||
c.close(False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is 1 of the 3 main changes. I didn't plan to change this at the beginning but while running tests, I discovered that closing a DB left Connections registered to the global transaction manager. Before, these Connections were ignored by patching their afterCompletion/newTransaction.
src/ZODB/DB.py
Outdated
if conn.transaction_manager is not None: | ||
for c in six.itervalues(conn.connections): | ||
c.explicit_transactions = True | ||
conn.transaction_manager.abort() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 2 other big changes:
- Make sure that the real
newTransaction
is not called when aborting an existing transaction. This was not the case ifexplicit_transactions=0
. - As written above, Connections of closed DB could remain registered, and when it happened, multi-db was probably broken because only the primary connections were ignored.
Setting explicit_transactions=1
is also simpler than patching newTransaction.
In addition to some micro-optimisation, this fixes the following minor issues: - Closing a DB left Connections registered to the global transaction manager. Which broke at least multi-db because only such primary connections were ignored with monkey-patches. - Stop calling the real newTransaction when aborting an existing transaction. This was not the case if explicit_transactions=0.
I have rebased and I wrote a commit message. Unless there's any objection, I'll merge on Friday.
Then I'll open a merge request to process invalidations immediately for closed connections, mainly to avoid memory leaks when pool-timeout is not set. |
Yay simpler!!! Sorry I haven't looked at this. I'll look at this this weekend. |
This looks great. Thanks! I added a comment for |
Oh, WIP? Is this still a work in progress? Or is it ready for merge? |
Some parts of the code to close Connection/DB and release resources looks too complicated for me. At least, there is questionable behaviour and also some possible micro optimization. Above all, I've started to look at this for some other change that would depend on it.