-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Fix leaked anyio streams in streamable_http #1991
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
aad9e69 to
aff1b13
Compare
aff1b13 to
42891f4
Compare
42891f4 to
de5d624
Compare
| finally: | ||
| await sse_stream_reader.aclose() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the reader moves to the finally but not the writer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On L569 it gets stored
# Store writer reference so close_sse_stream() can close it
self._sse_stream_writers[request_id] = sse_stream_writerShould I be closing it here too, or does it need to outlive this function?
| except Exception: | ||
| except Exception: # pragma: lax no cover | ||
| logger.exception("Error in standalone SSE response") | ||
| await self._clean_up_memory_streams(GET_STREAM_KEY) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this moves up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This stays in the except block same as before. Should this happen in finally regardless of exception?
acf0bbf to
b11e04f
Compare
The merge-base changed after approval.
Head branch was pushed to by a user without write access
|
Tests are fixed now |
Motivation and Context
Fixes a resource leak. I was seeing this in tests I am writing for a different issue (#421 (comment)). Before:
How Has This Been Tested?
Added a new test as first commit in this PR which fails. It passes with the fix, and I was able to remove a bunch of
pragma: no coverstatements.Breaking Changes
No
Types of changes
Checklist
Additional context