-
Notifications
You must be signed in to change notification settings - Fork 140
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
treq.testing.StubTreq: fix persisting twisted.web.server.Session objects between requests #328
Conversation
Before this commit, twisted.web.server.Session objects were not persisted properly between requests. Thus, two requests with the same cookies would result in two different sessions. See twisted#327 for details.
src/treq/testing.py
Outdated
|
||
def cleanSessions(self): | ||
""" | ||
Clean up sessions to prevent leaving behind a dirty reactor. |
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 behavior of leaving a dirty reactor looks like a bug in Twisted to me, actually. The session's delayed call should be registered with the reactor passed to Site
.
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.
I filed https://twistedmatrix.com/trac/ticket/10177 for this issue.
I think that it could be worked around here by overriding the site's sessionFactory
to pass the memory reactor to the Session
constructor. Then you wouldn't need this method.
src/treq/testing.py
Outdated
Clean up sessions to prevent leaving behind a dirty reactor. | ||
If you are using :obj:`StubTreq` with :obj:`twisted.web.server.Session` | ||
objects, you most likely have to call this method once you are done, | ||
for example during the tearDown of a unittest TestCase. |
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.
For stuff like this using self.addCleanup
is almost always superior.
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.
@twm Are you suggesting replacing this whole method to somehow use said function or to change the documentation?
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.
To change the documentation, but it's a moot point if this method is going away.
Thank you for the well-stated bug report and nicely polished PR! I think that the overall approach of reusing the Would you find the |
Hi,
It depends, but I'm leaning towards "no". It is useful when using Still, the primarily concern for writing Personally, I also feel like |
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.
Okay, then I'd say remove cleanSessions()
entirely.
It is likely that we'll end up making some (all?) of these attributes public as part of #227. In practice tests that use StubTreq
end up pretty tightly coupled to the details of these objects so I don't think it's useful to hide them — we couldn't make substantial internal changes without breaking folks' tests (even this PR has some Hyrum's Law risk in that regard, though I think it's acceptable).
Thanks!
Hi, I have now removed |
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.
Getting closer! I've tried to clarify my prior feedback.
@@ -88,6 +88,7 @@ def __init__(self, rootResource): | |||
reactor=self._memoryReactor, | |||
endpointFactory=_EndpointFactory(self._memoryReactor)) | |||
self._rootResource = rootResource | |||
self._serverFactory = Site(self._rootResource, reactor=self._memoryReactor) |
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.
I meant that you can cause the session to use the correct reactor by overriding sessionFactory
here like this:
self._serverFactory = Site(self._rootResource, reactor=self._memoryReactor) | |
self._serverFactory = Site(self._rootResource, reactor=self._memoryReactor) | |
self._serverFactory.sessionFactory = lambda site, uid: Session(site, uid, reactor=self._memoryReactor) |
Then you don't need the cleanup code in the test you added, and more importantly you don't need cleanup code in any test that uses RequestTraversalAgent
.
src/treq/test/test_testing.py
Outdated
# or expiring the sessions | ||
|
||
# manually expire the sessions. | ||
for sid in list(stub._agent._serverFactory.sessions.keys()): |
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.
I think that you could do this without poking all these private variables by making _SessionIdTestResource
append each session it creates to a public attribute and traversing that attribute here.
I've implemented the changes proposed above:
|
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.
Looks great! Thank you very much.
Sorry I lost track of this... I will merge now, and try to release by the weekend.
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.
Oh no, lint failure. :( Please fix that.
@twm Oops, I forgot that I configured the flake8 I'm using to ignore E501... Should be fixed now. |
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.
Looks great! Thank you!
This PR contains my proposed fix for #327. For details about the issue and this fix, see said issue.
tl;dr of original issue: Previously, a resource wrapped by
treq.testing.StubTreq
could not properly userequest.getSession()
, as it would always create a newtwisted.web.server.Session
instead of returning the same one. This is becausetreq.testing.StubTreq
always create a new instance oftwisted.web.server.Site
for each request. As sessions are stored within aSite
instance, this meant that upon completion of a request, the original session was lost. This PR solves this issue by moving thetwisted.web.server.Site
instance to an attribute of the agent. Additionally, the newtreq.testing.StubTreq.cleanSessions()
method provides a simple way of expiring the sessions in order to avoid leaving behind a dirty/unclean reactor. Although I am not sure if this is the best way to do this.