Handle the replayed tail the way data_received does - #13358
Draft
rodrigobnogueira wants to merge 8 commits into
Draft
Handle the replayed tail the way data_received does#13358rodrigobnogueira wants to merge 8 commits into
rodrigobnogueira wants to merge 8 commits into
Conversation
When the message queue fills, the pure-Python parser stores the unparsed remainder in self._tail and also returns it. The caller gets a second copy of bytes the parser is already holding, and the next feed_data() prepends the retained copy to the returned one. data_received() drops the returned tail unless the request upgraded, so ordinary pipelining is unaffected. finish_response() keeps it unconditionally, though, which is the path taken when a handler answers an upgrade request normally. Pipelining more requests than the queue holds behind such a request then serves them repeatedly, or grows the buffer by a copy of itself per request until memory runs out. The C parser never returned the tail, so only the pure-Python one is affected. Clear the returned buffer, matching what the incomplete-request-line branch already does. finish_response() also appended straight to the queue without the pause bookkeeping data_received() does, so nothing later fed the parser the remainder it kept: the requests past the queue limit went unanswered. Pause there too and let start() resume as it drains.
for more information, see https://pre-commit.ci
finish_response() replays the bytes buffered behind an upgrade request the handler declined. It feeds them to the parser without the two things data_received() does around the same call. A parse error escapes. It is raised before the declined upgrade's own response is written, so that response is lost, the connection is force closed, and a client error is logged as an unhandled exception. The same malformed request on the ordinary path is answered with 400. Queue the error message instead, exactly as data_received does. The upgraded flag from the replay is discarded. When the replayed bytes contain a second upgrade the parser switches to upgraded while this stays False, so the next data_received() takes the parser branch and assigns _message_tail rather than appending to it. The requests already buffered there are dropped, silently with the pure-Python parser and together with the following request under the C one. Keep the flag.
psf-chronographer
Bot
added
the
bot:chronographer:provided
rodrigobnogueira
added
backport-3.14
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #13358 +/- ##
=======================================
Coverage 98.99% 98.99%
=======================================
Files 132 132
Lines 49156 49246 +90
Branches 2560 2563 +3
=======================================
+ Hits 48661 48753 +92
+ Misses 371 370 -1
+ Partials 124 123 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
Merging this PR will not alter performance
Comparing 
Footnotes
|
The 400 closes the connection, so a single read to EOF collects both responses. The loop needed an end-of-stream break that never ran while the test passed, and tests are measured for coverage.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
What do these changes do?
finish_response()replays the bytes buffered behind an upgrade request the handler declined (#12796). It feeds them to the parser without the two thingsdata_received()does around the same call, and both are reachable from the network.A parse error escapes.
data_received()wrapsfeed_data()and turnsHttpProcessingErrorinto a queued 400.finish_response()does not, and it replays beforeresp.prepare()runs — so a malformed request pipelined behind the declined upgrade discards the response already built for the upgrade, force-closes the connection, and logs a routine client error as an unhandled server exception:The identical malformed request on the ordinary path already gets a clean 400.
The upgraded flag is discarded. When the replayed bytes contain a second upgrade, the parser switches to upgraded while
self._upgradedstaysFalse. The nextdata_received()therefore takes the parser branch and assigns_message_tailinstead of appending to it, dropping whatever was buffered there:/pipelineddropped, silently/pipelineddropped and the next request corruptedThe C parser's corruption is worth spelling out, because it is the sharper symptom.
nbis declared at_http_parser.pyx:631and assigned only inside theHPE_PAUSED_UPGRADEbranch at:675, but it is read at:711:Entering
feed_data()already upgraded, without llhttp returningHPE_PAUSED_UPGRADEon that call, therefore slicesdatawith an uninitialisedsize_t. In the reproducer it lands on 1, and the next request line arrives asb"ET /later HTTP/1.1"— the leadingGeaten. Keeping the flag stops the protocol from ever handing the parser that state, so this is a hardening of an invariant the C parser already assumes rather than only a bookkeeping fix.Are there changes in behavior for the user?
A malformed request pipelined behind a declined upgrade is answered with 400 instead of killing the connection and losing the preceding response. Requests buffered behind a second declined upgrade are dispatched instead of dropped. No API change.
Is it a substantial burden for the maintainers to support this?
No. It makes the replay path mirror
data_received(), which is where the same three concerns are already handled.Related issue number
None. Both defects predate #13356 — verified by running each reproducer against
288cf469band against this branch's parent and getting identical results — and both are on the path added in #12796.Checklist
CONTRIBUTORS.txt— already listedCHANGES/folderTest run
Both parser backends, C extensions built:
Both new tests fail with only
web_protocol.pyreverted, on both backends:mypy aiohttp/web_protocol.pyreports the same count as the parent commit, with no errors in the file itself.