fix(chains): release the client and response when a stream is abandoned - #2597
Open
EnesYilmazcode wants to merge 1 commit into
Open
fix(chains): release the client and response when a stream is abandoned#2597EnesYilmazcode wants to merge 1 commit into
EnesYilmazcode wants to merge 1 commit into
Conversation
predict_async_stream returned response.content.iter_any() from inside the `async with self._client_async()` block, so the client context exited while the caller was still reading. That released the concurrency slot before the stream had been consumed and left the aiohttp response checked out of the pool with nothing to release it, so a stream that ended by cancellation never gave its connection back. After concurrency_limit cancellations the stub could not reach its dependency at all until the hourly client cycle. Hand ownership of the client context and the response to the generator the caller iterates, so both are released when it is exhausted or closed. This matches predict_async, which already enters the response as a context manager. Signed-off-by: Enes Yilmaz <enesyilmaz5157@gmail.com>
Contributor
|
@EnesYilmazcode can't you check |
Contributor
Author
|
It also doesn't cancel anything. The cancel already happens when uvicorn kills the parent's generator, this just releases the response and the slot afterwards. Full consumption is untouched, last chunk still lands before cleanup. They're complementary really. Even with disconnect detection something has to give the connection back, and right now nothing does. |
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
predict_async_streamreturns the response iterator from inside theasync with self._client_async()block, so the client context exits while the caller is still reading, releasing the concurrency slot early and leaving the aiohttp response checked out of the pool with nothing to release it.This hands ownership of the client context and the response to the generator the caller iterates, so both are released when it is exhausted or closed.
The practical effect is a stream that ends by cancellation, which is what happens whenever an end user closes the tab mid-generation: uvicorn cancels the parent chainlet's response generator, that propagates into
async for data in await self.predict_async_stream(...), and the abandoned response never returns its connection. Each one permanently consumes a slot inTCPConnector(limit=concurrency_limit). Afterconcurrency_limitof them the stub cannot reach that dependency at all until the session is cycled an hour later.ongoing_requestsreads0throughout, so the gauge reports idle while the stub is wedged.Measured against a local server with the real
StubBase,concurrency_limit=2, two consumers cancelled mid-stream:ongoing_requestsmid-streamgc.collect()does not rescue it, so this is not something the garbage collector cleans up later.💻 How
contextlib.AsyncExitStackacquires the client context and the response,pop_all()transfers ownership once the request has succeeded, and the generator releases them in its ownasync with. Errors raised during setup still close the stack before propagating, so retries do not accumulate connections.The request stays eager rather than moving into the generator, because
RPCOptions.retriesis documented as only retrying before results stream back, andcode_gen.pyemitsasync for data in await self.predict_async_stream(...), which cannot await a plain async generator.One behavior change worth flagging: the slot is now held for the life of the stream, which is what
predict_asyncalready does. Concurrency is unchanged in count, sinceTCPConnector(limit=...)already capped it, but a caller that exceeds the limit now waits on the semaphore instead of failing withTimeoutErroraftertimeout_sec. At the defaultconcurrency_limit=300that needs 300 concurrent streams to one dependency; atconcurrency_limit=1a chainlet that calls the same stub while consuming a stream from it would deadlock.🔬 Testing
Three tests in
truss-chains/tests/test_remote_chainlet.py, all against a loopback aiohttp server, no credentials or network. Each fails onmainand passes here:aclose()(assert 0 == 1onmain)concurrency_limitcancellations the stub can still reach its dependency (hangs out the timeout onmain)truss-chainsunit suite: 177 passed, 1 skipped.ruff check,ruff format --checkandmypyclean on both files.