Add tests pinning ownership of a stalled WebSocket parser - #13408
Draft
aiolibsbot wants to merge 1 commit into
Draft
Add tests pinning ownership of a stalled WebSocket parser#13408aiolibsbot wants to merge 1 commit into
aiolibsbot wants to merge 1 commit into
Conversation
The queue only holds a weak reference to a parser that stopped mid-read, and both protocols drop their reference to it on connection loss. Cover the client and server responses owning the parser: without that, 3903 of 8000 already-received frames are collected and the application sees a short stream with a normal CLOSED.
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
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?
Add two functional tests that pin the parser-ownership contract #13393 introduces:
ClientWebSocketResponse._parserandWebSocketResponse._parser. Each test feeds oneoversized read, simulates the peer vanishing, collects, and drains.
Targets
fix-ws-queue, notmaster.Are there changes in behavior for the user?
No, tests only.
Is it a substantial burden for the maintainers to support this?
No. Two tests, no fixtures, no timing dependency — the read and the connection loss are
both driven synchronously.
Related issue number
Follow-up to #13393. Full reference-scope analysis posted as a comment there.
Checklist
CONTRIBUTORS.txt— already listedCHANGES/folder — N/A, covered byCHANGES/13393.bugfix.rston the base branch (needsbot:chronographer:skipif the bot objects)Why these tests
The queue reaches the stalled parser through a weak reference, and both protocols drop
their strong reference on connection loss (
ResponseHandler.connection_lostandRequestHandler.connection_lostbothfeed_eof()then_payload_parser = None). Thetwo
_parserassignments in this PR are the only thing keeping the parked framesreachable, and nothing failed if they were removed.
Reverting
ws_resp._parser = parserinclient.py, or theself._parserassignment inweb_ws.py, makes each test reportassert 4097 == 8000— 3903 already-received framescollected, delivered as a normal
CLOSED, no exception anywhere.Test run
Cython coverage:
aiohttp/_websocket/reader_candmaskwere cythonized and compiledlocally (the full
make install-devneeds a Node.js build of the vendored llhttp, whichwas unavailable).
weakref.ref()on the compiledWebSocketReaderworks with the newcdef object __weakref__slot.Drafted with Claude Opus 5 (Kōan agent); reviewed by @bdraco.
Quality Report
Changes: 18 files changed, 372 insertions(+), 21 deletions(-)
Code scan: clean
Tests: failed (FAILED)
Branch hygiene: clean
Generated by Kōan