Skip to content

Cancel blocked server-stream handlers on client disconnect - #395

Open
stefanvanburen wants to merge 3 commits into
mainfrom
svanburen/stream-disconnect-prompt
Open

stefanvanburen wants to merge 3 commits into
mainfrom
svanburen/stream-disconnect-prompt

Conversation

@stefanvanburen

Copy link
Copy Markdown
Member

#175 made the server watch receive() for http.disconnect during server streams, but the response loop only checks for it between messages, so a handler that's waiting to produce its next message (a health Watch that only yields on status changes, say) keeps running until it next yields or hits its deadline. This runs the response loop inside an asyncio.timeout(None) scope that the watcher reschedules to now on disconnect, which cancels whatever the loop is awaiting, the handler's anext or a send, and ends the call as CANCELED as before. The handler's and stream interceptors' finally blocks run and on_end sees CANCELED.

Bidi and client streams have the same gap once the request body ends, but there the handler is the one reading receive(), so a separate watcher would steal body chunks. Covering them needs a single task that owns receive() for the whole call, which I'd rather do as a follow-up, especially since #348 rewrites this code for trio. This maps onto its watch_for_disconnect; under trio the scope would be a CancelScope.

The disconnect watcher added in #175 only set an event that the
response loop checked between messages, so a handler blocked before
producing its next message (such as a health `Watch` waiting for a
status change) kept running until it next yielded or its deadline
passed.

Run the response loop inside an `asyncio.timeout(None)` scope that the
watcher expires on `http.disconnect`. The cancellation interrupts
whatever the loop awaits, including the handler, so its generator and
any wrapping interceptors unwind with their `finally` blocks, and the
RPC still ends as CANCELED with leading metadata interceptors seeing
that error in `on_end`.

Signed-off-by: Stefan VanBuren <stefan@vanburen.xyz>
Both server-stream disconnect tests in `test_roundtrip.py` built the
same scope inline. Also leave the new test's metadata interceptor
untyped, like the one in `test_interceptor.py`.

Signed-off-by: Stefan VanBuren <stefan@vanburen.xyz>
Signed-off-by: Stefan VanBuren <stefan@vanburen.xyz>
@stefanvanburen
stefanvanburen marked this pull request as ready for review October 14, 2026 00:28

@anuraaga anuraaga left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense and the context manager seems much cleaner. But wondering if we need to merge this before #348 and #383

PS: I could finally get it from reading the code but Claude descriptions are ungrokkable to me now adays ;)

@stefanvanburen

Copy link
Copy Markdown
Member Author

PS: I could finally get it from reading the code but Claude descriptions are ungrokkable to me now adays ;)

agreed; this one was particularly dense. although tbh, the code itself is really quite subtle, at least to me. :)

But wondering if we need to merge this before #348 and #383

do you mean you'd like to land those first? Happy to hold off until we get those situated given that these are all in roughly the same area of the codebase!

This branch has not been deployed

No deployments
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.

2 participants