Raise AttributeError on request.tm if tm inactive
#46
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This will allow the `request.tm` request property to determine whether or not the transaction manager is active for the current request and thus whether to return a reference to the manager to the caller.
|
I reckon it's probably time to do a bit of cleanup of the core loop in the tween, but I didn't want to confuse the issue by doing any refactoring in this PR. Happy to follow up with a bit of cleanup. Perhaps we can use |
nickstenning
force-pushed
the
raise-if-tm-inactive
branch
from
60889aa
to
ce1179a
Compare
nickstenning
added a commit
to hypothesis/h
that referenced
this pull request
Aug 29, 2016
Opening a database session to load feature flags for these requests is
not only not necessary, it is quite problematic. It causes the leakage
of references to the database session inside `zope.sqlalchemy` because
these requests have been opted out of the transaction manager by an
`activate_hook` (see `h.app`).
By using the database in these requests, we were accidentally
registering a database session with a transaction manager that wasn't
hooked up to the request lifecycle. This meant that zope.sqlalchemy's
internal references to the session weren't properly cleaned up. In a
high traffic environment -- these leaked session references would then
later lead to a session (in an entirely different request) not being
correctly registered with the transaction manager.
I've also opened a pull request against `pyramid_tm` which should
prevent this class of bug in the future:
Pylons/pyramid_tm#46
seanh
pushed a commit
to hypothesis/h
that referenced
this pull request
Aug 30, 2016
Opening a database session to load feature flags for these requests is
not only not necessary, it is quite problematic. It causes the leakage
of references to the database session inside `zope.sqlalchemy` because
these requests have been opted out of the transaction manager by an
`activate_hook` (see `h.app`).
By using the database in these requests, we were accidentally
registering a database session with a transaction manager that wasn't
hooked up to the request lifecycle. This meant that zope.sqlalchemy's
internal references to the session weren't properly cleaned up. In a
high traffic environment -- these leaked session references would then
later lead to a session (in an entirely different request) not being
correctly registered with the transaction manager.
I've also opened a pull request against `pyramid_tm` which should
prevent this class of bug in the future:
Pylons/pyramid_tm#46
| @@ -50,6 +50,9 @@ def tm_tween(request): | |||
| if not activate(request): | |||
| return handler(request) | |||
|
|
|||
| # Set a flag in the environment to enable the `request.tm` property. | |||
| request.environ['tm.active'] = True | |||
There was a problem hiding this comment.
there should be a guard against something else setting tm.active similar to the guard against repoze.tm.active as transaction cannot be nested
|
I cannot remember the reason off the top of my head but pyramid_tm used to use This PR looks good, just address the one comment and add your name to CONTRIBUTORS.txt in the repo. |
This makes it much harder for an application to mistakenly get hold of a reference to the transaction manager during a request for which the activate_hook returns `False`. There should never be any need to get a reference to the transaction manager during a request that has been opted out. The ability to do so opens up possibilities for subtle bugs where database sessions are registered with a manager that isn't itself hooked up to the request lifecycle, resulting in leaked references to those sessions inside `zope.sqlalchemy`.
nickstenning
force-pushed
the
raise-if-tm-inactive
branch
from
ce1179a
to
91b6465
Compare
|
And it looks like we stopped using So it may be that switching back to that will work fine as long as we're ok to require |
|
Sure, you're welcome to consider that in a separate PR but I'm not likely to accept it quite yet... you'll see in #42 where going back to |
This was referenced Feb 26, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses the issue raised in #45.
Raising an
AttributeErrorforrequest.tmif the current request doesn't have an active transaction manager makes it much harder for an application to mistakenly get hold of a reference to a transaction manager which is not committed (or rolled back) at the end of the request.There should never be any need to get a reference to the transaction manager during a request that has been opted out using an
activate_hook. The ability to do so opens up possibilities for subtle bugs where database sessions are registered with a manager that isn't itself hooked up to the request lifecycle, resulting in leaked references to those sessions insidezope.sqlalchemy.As suggested in #45, this is implemented by adding
tm.activeto the WSGI environment on the way through the tween, and removing it on the way out.