Collapse WS fragments instead of pausing mid-frame - #13487
Conversation
A frame delivered across more reads than the fragment cap pauses the transport mid-frame. Nothing can undo that pause: the frame only completes once more data arrives, and the sole resume path is a read off the queue, which is still empty because no message has been queued yet. The connection wedges permanently. The test fails against the current parser.
The per-message fragment cap paused the transport once a frame arrived in more reads than the cap allowed. That pause can never be lifted: the frame only completes when more data arrives, and the sole resume path runs when a message is read off the WebSocket queue, which is still empty because the frame has not produced one. A peer splitting a single frame across enough reads wedged the connection for good and the process stopped reading that socket. Join the pending fragments into one buffer when the cap is exceeded instead. That keeps the bound the cap was added for -- retained overhead stays proportional to the frame, which max_msg_size already caps -- and the joins stay amortised because the cap scales with max_msg_size.
psf-chronographer
Bot
added
the
bot:chronographer:provided
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #13487 +/- ##
=======================================
Coverage 99.00% 99.00%
=======================================
Files 132 132
Lines 49626 49637 +11
Branches 2575 2575
=======================================
+ Hits 49132 49143 +11
Misses 370 370
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. |
|
Superseded by #13488 |
Merging this PR will degrade performance by 13.6%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | test_read_one_hundred_websocket_text_messages |
517.1 µs | 598.5 µs | -13.6% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing aiolibsbot:koan/ws-fragment-limit-strand (cb950f2) with master (5e95a13)2
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. ↩
-
No successful run was found on
master(8b9cfb5) during the generation of this report, so 5e95a13 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
2 participants
What do these changes do?
The per-message WebSocket fragment cap added in #13352 pauses the transport
when a frame arrives in more reads than the cap allows. That pause can never
be lifted: the frame only completes once more data arrives, and the sole
resume path (
WebSocketDataQueue._read_from_buffer) runs when a message isread off the queue — which is still empty, because the frame in flight has
not produced one.
A peer that splits a single frame across more than
max(1024, max_msg_size // 256)transport reads therefore wedges the connection permanently: the process stops
reading that socket for good and the connection is never released (the server
WebSocket path has no read timeout unless a heartbeat is configured). It costs
the peer ~1024 small writes per connection.
This replaces the pause with joining the pending fragments into a single
buffer. The bound the cap was added for is kept — retained per-read object
overhead stays proportional to the frame, which
max_msg_sizealready caps —and the joins stay amortised because the cap scales with
max_msg_size.Are there changes in behavior for the user?
Yes, and it is the point: a frame delivered in many small reads is now parsed
and delivered instead of hanging the connection. Reading is no longer paused
mid-frame.
Is it a substantial burden for the maintainers to support this?
No — it removes a mechanism rather than adding one. Three lines in the hot
path replace a pause with a join.
Related issue number
Follow-up to #13352 (unreleased, master only — no released version is
affected). Found while auditing #13393 for stranded readers.
Checklist
THREAT_MODEL.md§5.3 / §6.3)CONTRIBUTORS.txt(bot account, drafted under maintainer review)CHANGES/folderRun output
End-to-end repro against a real
aiohttpserver over a real socket, one4 KiB binary frame written 2 bytes at a time (2048 reads, cap 1024):
Test suites, Cython extension built:
The new test fails against the parent commit:
Drafted with Kōan (Claude Opus 5); reviewed by @bdraco.
Quality Report
Changes: 4 files changed, 72 insertions(+), 17 deletions(-)
Code scan: clean
Tests: failed (FAILED)
Branch hygiene: clean
Generated by Kōan