Fix server streaming handler not cancelled on client disconnect#175
Fix server streaming handler not cancelled on client disconnect#175stefanvanburen merged 4 commits intomainfrom
Conversation
When a client disconnects during a server streaming RPC, the async generator continued yielding indefinitely because receive() was never consulted after the initial request was consumed. Fix this by spawning a background task in the `EndpointServerStream` case that monitors receive() for http.disconnect. When detected, an Event is set; the streaming loop checks it between yields and raises ConnectError(CANCELED) if set. The response stream is also explicitly `aclose()`'d in the finally block so generator finally-clauses run promptly rather than being deferred to GC. Fixes #174. Signed-off-by: Stefan VanBuren <svanburen@buf.build>
stefanvanburen
force-pushed
the
svanburen/client-disconnect
branch
from
8774a61 to
4df01d1
Compare
|
FWIW, this is vaguely similar to how Starlette handles this, but they're using |
| # run promptly (Python defers async-generator cleanup to GC otherwise). | ||
| aclose = getattr(response_stream, "aclose", None) | ||
| if aclose is not None: | ||
| await aclose() |
There was a problem hiding this comment.
IIUC it's possible for this to throw since it's user code and then monitor task would be leaked. When thinking about letting it propagate vs catch and maybe log, I guess a finally in a user generator is still part of the request handler and makes sense to allow to affect the response. How about just reordering then if that makes sense?
src/connectrpc/_server_async.py
Outdated
| await aclose() | ||
| if monitor_task is not None: | ||
| monitor_task.cancel() | ||
| with contextlib.suppress(CancelledError, Exception): |
There was a problem hiding this comment.
Since we know monitor task never raises, maybe can keep it simply canceled error
| request_bytes = Size(inches=10).SerializeToString() | ||
| request_body = struct.pack(">BI", 0, len(request_bytes)) + request_bytes | ||
|
|
||
| disconnect_trigger = asyncio.Event() |
There was a problem hiding this comment.
I think a client with a timeout of some tens of Ms should be non-flaky, but maybe the windows runner would try extra hard to prove otherwise.
Manually invoking the app seems fine too but let's add a comment then about potential flakiness if using a client with a timeout (we need it to happen after the request has been fully read)?
Signed-off-by: Stefan VanBuren <svanburen@buf.build>
;) Signed-off-by: Stefan VanBuren <svanburen@buf.build>
2 participants
When a client disconnects during a server streaming RPC, the async generator continued yielding indefinitely because receive() was never consulted after the initial request was consumed.
Fix this by spawning a background task in the
EndpointServerStreamcase that monitors receive() for http.disconnect. When detected, an Event is set; the streaming loop checks it between yields and raises ConnectError(CANCELED) if set. The response stream is also explicitlyaclose()'d in the finally block so generator finally-clauses run promptly rather than being deferred to GC.Fixes #174.