twisted.web.http.Request allows you to write to a finished request.

[David Reid]

This can cause corruption to subsequent requests, it is also partially responsible for the behavior described in #3684

[Jean-Paul Calderone]

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.

[hderms]

Simple fix, uses pythonic idiom that was used in the same class

[Jean-Paul Calderone]

Thanks. We need a unit test which verifies that this behaves as desired, too. Can you add that to the patch?

[hderms]

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.

[hderms]

Rewrite of patch and addition of unit test

[hderms]

Added a new patch which raises a RuntimeError when Request.write is called after Request.finish. Added a unit test which tests this functionality.

[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!

[hderms]

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.

[therve]

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.

[hderms]

adjusted changes requested by reviewer

[Jean-Paul Calderone]

(In [30650]) Branching to 'request-write-after-finish-4317'

[Jean-Paul Calderone]

(In [30651]) Apply patch from the ticket, with a couple trivial changes

refs #4317

[Jean-Paul Calderone]

(In [30652]) news fragment

refs #4317

[Jean-Paul Calderone]

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!

[Jean-Paul Calderone]

(In [30653]) Merge request-write-after-finish-4317

Author: hderms Reviewer: therve, exarkun Fixes: #4317

In Request.write, check to see if the response has already been finished and raise an exception if it has, rather than tossing garbage bytes onto the connection.