Properly clean up response streams in BaseSession #515
Conversation
Wraps the request handling in a try/finally block to ensure that response streams are properly closed and removed from the tracking dictionary, even if an exception occurs during request processing. This change also prevents response_stream and response_stream_reader instances from piling up on _exit_stack over the course of the session. Github-Issue:#169
| await response_stream.aclose() | ||
| await response_stream_reader.aclose() |
There was a problem hiding this comment.
Is this correct tho? Do you want to close the stream on every request? 🤔
There was a problem hiding this comment.
Well, we're creating them on every request, so I think the old code was just piling
up open streams until the end of the session (or getting unclosed resource errors if they got __del__d before the session's __aexit__, or if the latter never ran).
Please check my logic though - I'm new to this code. E.g. is there a reason we need to leave streams open after getting the response?
There was a problem hiding this comment.
Yeah, but the connection stays open for some time after you send back the response.
There was a problem hiding this comment.
Ah ok, sounds like this would need a more sophisticated solution. I'll move this to draft and come back to it. Thanks!
There was a problem hiding this comment.
By default, it stays alive for 5 secs (https://www.uvicorn.org/settings/#timeouts) fyi
There was a problem hiding this comment.
@Kludex just to clarify - these are in-memory streams, not network streams. (_receive_loop gets the network response and writes it to response_stream). There may still be a problem I'm not aware of with closing the streams immediately, but want to make sure you weren't worried about closing while network connections were still open.
[edit: aha wrote this before seeing your fyi, so it sounds like you were worried about closing HTTPS streams, right? undrafting based on this assumption]
|
@ihrpr thanks for taking a look! Backed out the starlette bump, but as a result needed to add a warning suppression to get tests to pass (they're failing on |
pyproject.toml
Outdated
| "ignore:Returning str or bytes.*:DeprecationWarning:mcp.server.lowlevel" | ||
| "ignore:Returning str or bytes.*:DeprecationWarning:mcp.server.lowlevel", | ||
| # this is a problem in starlette 0.27, which we're currently pinned to | ||
| "ignore:Please use `import python_multipart` instead.:PendingDeprecationWarning", |
There was a problem hiding this comment.
tried running test without this change, all works for me, lets' remove this
| @@ -187,7 +187,6 @@ def __init__( | |||
| self._receive_notification_type = receive_notification_type | |||
| self._session_read_timeout_seconds = read_timeout_seconds | |||
| self._in_flight = {} | |||
|
|
|||
Summary
starlette0.27 (pinned), without which tests failTest plan
May fix #169