Opened 12 years ago
Closed 11 years ago
#4317 defect closed fixed (fixed)
twisted.web.http.Request allows you to write to a finished request.
Reported by: | David Reid | Owned by: | |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | web | Keywords: | easy |
Cc: | Branch: |
branches/request-write-after-finish-4317
branch-diff, diff-cov, branch-cov, buildbot |
|
Author: | hderms |
Description
This can cause corruption to subsequent requests, it is also partially responsible for the behavior described in #3684
Attachments (3)
Change History (18)
comment:1 Changed 12 years ago by
Keywords: | easy added |
---|
Changed 11 years ago by
Attachment: | ticket 4317 patch.patch added |
---|
Simple fix, uses pythonic idiom that was used in the same class
comment:2 Changed 11 years ago by
Keywords: | review added |
---|---|
Owner: | jknight deleted |
comment:3 Changed 11 years ago by
Keywords: | review removed |
---|---|
Owner: | set to hderms |
Thanks. We need a unit test which verifies that this behaves as desired, too. Can you add that to the patch?
comment:4 Changed 11 years ago by
I will do so. I was confused and thought unit-tests were only required when adding new features. Will read the twisted documentation on unit tests and then implement.
Changed 11 years ago by
Attachment: | ticket 4317 patch v2.patch added |
---|
Rewrite of patch and addition of unit test
comment:5 Changed 11 years ago by
Keywords: | review added |
---|---|
Owner: | hderms deleted |
Added a new patch which raises a RuntimeError when Request.write is called after Request.finish. Added a unit test which tests this functionality.
comment:6 follow-up: 7 Changed 11 years ago by
Author: | → hderms |
---|---|
Keywords: | review removed |
Owner: | set to hderms |
+ if self.finished == 1:
You should simply call `if self.finished:' here. This ought to be a boolean, if though it's not...twisted.web.test.test_xml.MicroDOMTest.testLaterCloserTable
fails after your fix is applied. I suspect the test is broken, but it should be fixed.-
- self.assertRaises(RuntimeError, req.finish) + self.assertRaises(RuntimeError, req.finish())
This change is broken, it makes the test fails. - There should be 2 blank lines before your test
- The test docstring looks a bit weird. Is there an extra "L{Request.finish}" at the beginning?
Thanks!
comment:7 Changed 11 years ago by
Replying to therve:
+ if self.finished == 1:
You should simply call `if self.finished:' here. This ought to be a boolean, if though it's not...twisted.web.test.test_xml.MicroDOMTest.testLaterCloserTable
fails after your fix is applied. I suspect the test is broken, but it should be fixed. - self.assertRaises(RuntimeError, req.finish) + self.assertRaises(RuntimeError, req.finish())This change is broken, it makes the test fails.- There should be 2 blank lines before your test
- The test docstring looks a bit weird. Is there an extra "L{Request.finish}" at the beginning?
Thanks!
I examined that test that fails, and I can't seem to find any connection between it and /web/http, it doesn't import anything http related, and the test in particular that fails is just parsing strings. I'm not entirely sure but I think the test is broken as well.
I didn't implement the if self.finished == 1: and I found that to be rather unpythonic as well, but was reluctant to change it because this is my first patch.
The change to the the test_finishAfterConnectionLost(self): was unintentional. I think it resulted from me examining how these unit tests worked, I must have forgotten to eliminate that portion. I will remove it and resubmit.
I will fix the formatting errors.
comment:8 Changed 11 years ago by
Hum sorry, the test I mentioned (twisted.web.test.test_xml.MicroDOMTest.testLaterCloserTable
) was probably just a output parsing error on my part. It must have been the other one.
Changed 11 years ago by
Attachment: | ticket 4317 patch v3.patch added |
---|
adjusted changes requested by reviewer
comment:9 Changed 11 years ago by
Keywords: | review added |
---|---|
Owner: | hderms deleted |
comment:10 Changed 11 years ago by
Author: | hderms → exarkun, hderms |
---|---|
Branch: | → branches/request-write-after-finish-4317 |
(In [30650]) Branching to 'request-write-after-finish-4317'
comment:11 Changed 11 years ago by
comment:13 Changed 11 years ago by
Author: | exarkun, hderms → hderms |
---|---|
Keywords: | review removed |
Owner: | set to Jean-Paul Calderone |
Status: | new → assigned |
Did you manually edit the patch to remove the last hunk? However the latest version of the patch was created, neither patch nor emacs is happy with it (Emacs says "Hunk seriously messed up"). It's not a problem because this patch is short enough that I can manually apply it. Whatever happened though, try to avoid it in the future. :)
I checked the changes into a branch, added a news fragment, and made the if self.finished:
change therve suggested. Build results look good, so I'll go ahead and merge this. Thanks!
comment:14 Changed 11 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:15 Changed 11 years ago by
Owner: | Jean-Paul Calderone deleted |
---|
We should change
Request.write
to discard such bytes. We should also introduce a warning for this case (that could be done separately though). And ultimately, this should result in an exception.