Skip to content

Add tests pinning ownership of a stalled WebSocket parser - #13408

Draft
aiolibsbot wants to merge 1 commit into
aio-libs:fix-ws-queuefrom
aiolibsbot:koan/ws-stalled-parser-ownership-tests
Draft

Add tests pinning ownership of a stalled WebSocket parser#13408
aiolibsbot wants to merge 1 commit into
aio-libs:fix-ws-queuefrom
aiolibsbot:koan/ws-stalled-parser-ownership-tests

Conversation

@aiolibsbot

@aiolibsbot aiolibsbot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

What do these changes do?

Add two functional tests that pin the parser-ownership contract #13393 introduces:
ClientWebSocketResponse._parser and WebSocketResponse._parser. Each test feeds one
oversized read, simulates the peer vanishing, collects, and drains.

Targets fix-ws-queue, not master.

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

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes — N/A
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt — already listed
  • Add a new news fragment into the CHANGES/ folder — N/A, covered by CHANGES/13393.bugfix.rst on the base branch (needs bot:chronographer:skip if 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_lost and
RequestHandler.connection_lost both feed_eof() then _payload_parser = None). The
two _parser assignments in this PR are the only thing keeping the parked frames
reachable, and nothing failed if they were removed.

Reverting ws_resp._parser = parser in client.py, or the self._parser assignment in
web_ws.py, makes each test report assert 4097 == 8000 — 3903 already-received frames
collected, delivered as a normal CLOSED, no exception anywhere.

Test run
$ PYTHONPATH=. pytest tests/test_websocket_parser.py tests/test_client_ws_functional.py \
    tests/test_web_websocket_functional.py tests/test_web_websocket.py \
    tests/test_client_ws.py tests/test_websocket_handshake.py --numprocesses=auto
336 passed in 5.36s                       # with the _websocket Cython extensions built

$ AIOHTTP_NO_EXTENSIONS=1 PYTHONPATH=. pytest <same>
334 passed, 2 skipped in 4.87s

Cython coverage: aiohttp/_websocket/reader_c and mask were cythonized and compiled
locally (the full make install-dev needs a Node.js build of the vendored llhttp, which
was unavailable). weakref.ref() on the compiled WebSocketReader works with the new
cdef 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

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant