Skip to content
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

11976 stop processing pipelined HTTP/1.1 requests that are received together #11979

Merged
merged 16 commits into from
Sep 12, 2023

Conversation

glyph
Copy link
Member

@glyph glyph commented Sep 11, 2023

Scope and purpose

Fixes #11976

src/twisted/web/http.py Outdated Show resolved Hide resolved
@glyph glyph changed the title 11976 stop processing pipelined requests that are received together 11976 stop processing pipelined HTTP/1.1 requests that are received together Sep 11, 2023
Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is ready for review.
I left some comments as I had a bit of time to look into this.

Thanks for the quick fix.


I think the macOS failure is flaky ... I did a manual retry

src/twisted/web/test/test_web.py Outdated Show resolved Hide resolved
src/twisted/web/test/test_web.py Outdated Show resolved Hide resolved
src/twisted/web/test/test_web.py Outdated Show resolved Hide resolved
src/twisted/web/http.py Show resolved Hide resolved
src/twisted/web/http.py Outdated Show resolved Hide resolved
src/twisted/web/http.py Outdated Show resolved Hide resolved
src/twisted/web/http.py Show resolved Hide resolved
@glyph
Copy link
Member Author

glyph commented Sep 11, 2023

please review

@glyph
Copy link
Member Author

glyph commented Sep 11, 2023

sorry I forgot I need to summon the robot even for the first PR :)

@chevah-robot chevah-robot requested a review from a team September 11, 2023 15:24
Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good. Thanks for the quick fix.

I would prefer to have the new test added to t.w.test.test_http.PipeliningBodyTests , rather than test_web

This is the only major comment.

The others are minor.

Thanks again

src/twisted/web/http.py Outdated Show resolved Hide resolved
src/twisted/web/http.py Outdated Show resolved Hide resolved
src/twisted/web/test/test_web.py Show resolved Hide resolved
@@ -1849,3 +1852,50 @@ def test_defaultReactor(self):

factory = http.HTTPFactory()
self.assertIs(factory.reactor, reactor)


class QueueResource(Resource):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this test added to test_web.py and not to test_http.py ?

Just asking.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test_http already has a test for this which doesn't cover the full stack top-to-bottom.

The boundaries are fuzzy, and this fix obviously has its foot in both worlds, but HTTP is for the HTTP protocol parsing, whereas web is for request handling. This is for the boundary where a parsed HTTP protocol message becomes an invocation of the request handling logic.

At first I felt like the problem might be at a higher level because there's already a test for this in test_http, before I realized that it was a TCP segment colocality issue. But, problems might be at a higher level in the future, so having the test live at this later, both in test_web and with server.Site being the layer of the public API getting tested, still makes sense to me.

Copy link
Member

@adiroiban adiroiban Sep 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the info

Maybe test_http.py needs a docstring that is more than Test HTTP support. same for test_web.py with Tests for various parts of L{twisted.web}.

I will keep this in mind and will try to update the docstring, next time I will touch these files

@adiroiban
Copy link
Member

there was a flaky test failure on macOS . I manually triggered a retry.
I saw this test failing on macOS quite often.

@glyph glyph merged commit 1e6e9d2 into trunk Sep 12, 2023
23 checks passed
@glyph glyph deleted the 11976-accidental-pipelining-bug branch September 12, 2023 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

twisted.web.server doesn't support HTTP pipelining
3 participants