Stop handing back pipelined requests the parser buffered - #13356
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.
psf-chronographer
Bot
added
the
bot:chronographer:provided
for more information, see https://pre-commit.ci
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #13356 +/- ##
=======================================
Coverage 98.99% 98.99%
=======================================
Files 132 132
Lines 49156 49189 +33
Branches 2560 2562 +2
=======================================
+ Hits 48661 48694 +33
Misses 371 371
Partials 124 124
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 improve performance by 10.18%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | test_resolve_gitapi |
660 ms | 599 ms | +10.18% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing rodrigobnogueira:fix-parser-tail-duplication (a8e71e2) with master (288cf46)
Footnotes
-
83 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
rodrigobnogueira
added
backport-3.14
Co-authored-by: Sam Bull <aa6bs0@sambull.org>
Backport to 3.14: 💚 backport PR created✅ Backport PR branch: Backported as #13360 🤖 @patchback |
Backport to 3.15: 💚 backport PR created✅ Backport PR branch: Backported as #13361 🤖 @patchback |
2 participants
What do these changes do?
When the pipelined message queue fills, the pure-Python parser buffers the unparsed remainder in
self._tailand returns it (http_parser.py, queue-full break). The caller therefore holds a second copy of bytes the parser is already keeping, and the nextfeed_data()prepends the retained copy onto the returned one.data_received()discards the returned tail unless the request upgraded, so ordinary pipelining never notices.finish_response()stores it unconditionally, and that is the path taken when a handler answers anUpgrade:request with a normal response. Pipelining more requests than the queue holds behind such a request then makes the server either serve them repeatedly or grow the buffer by a copy of itself per handled request.Measured on this branch's parent, pure-Python parser, one upgrade request followed by 40 pipelined GETs in a single write:
The fix clears the returned buffer, which is exactly what the incomplete-request-line branch a few lines below already does. The C parser never returned the tail, so the duplication is pure-Python only, and the added parser test passes on the C backend before and after.
The second defect is not parser-specific.
finish_response()appends to the message queue without the pause bookkeepingdata_received()performs, so_msg_queue_pausedstaysFalse,start()never calls_resume_msg_queue_reading(), and nothing ever feeds the parser the remainder it kept. The requests past the queue limit simply go unanswered (33 of 41 above). This half affects the C parser too — arguably more cleanly, since before the fix the pure-Python duplicate kept_message_tailnon-empty and so kept pumping. Reverting both source files and running the new functional test under the C extensions fails on its 10s timeout. Pausing infinish_response()lets the existing drain resume them.Are there changes in behavior for the user?
Requests pipelined behind a declined upgrade past the queue limit are answered instead of being left buffered — that applies to both parsers. With the pure-Python parser they are additionally handled exactly once rather than repeatedly. No API change.
Is it a substantial burden for the maintainers to support this?
No. Five lines of parser change plus the queue-pause call that the neighbouring code path already makes, and both new tests fail without them.
Related issue number
None.
Checklist
CONTRIBUTORS.txt— already listedCHANGES/folderTest run
Both parser backends, C extensions built (
make cythonize+ editable install):Revert check, with the source changes reverted and the tests kept:
Under
AIOHTTP_NO_EXTENSIONS=1that functional test does not fail cleanly when reverted: the repeated dispatch keeps the loop busy and the test's own timeout never fires. With the C extensions there is no repeated dispatch, so it fails on the timeout as shown above. It passes in well under a second either way with the change applied.