-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix: memory leak and exit problems #586
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
@@ -27,7 +27,7 @@ dependencies = [ | |||
"httpx-sse>=0.4", | |||
"pydantic>=2.7.2,<3.0.0", | |||
"starlette>=0.27", | |||
"sse-starlette>=1.6.1", | |||
"sse-starlette>=2.3.0", |
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.
Is this needed?
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.
Yes, sse-starlette
supports client_close_handler_callable
since version 2.3.0.
This looks great, Thanks @PWZER |
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.
Unfortunately we can't upgrade sse-starlette
in SDK now.
We can achieve similar functionality as client_close_handler_callable
provides in this case by using disconnect_listener
Something like this should work:
async with anyio.create_task_group() as tg:
response = EventSourceResponse(
content=sse_stream_reader,
data_sender_callable=sse_writer,
)
async def disconnect_listener():
try:
await response.listen_for_disconnect(receive)
logger.error(f"Disconnect detected for session: {session_id}")
await client_close_handler() # defined earlier in the PR - name should be changed probably
except Exception as e:
logger.error(f"Error in disconnect listener: {e}")
tg.start_soon(disconnect_listener)
logger.debug("Starting SSE response task")
tg.start_soon(response, scope, receive, send)
...
yield (read_stream, write_stream)
async def client_close_handler(message: Message) -> None: | ||
await read_stream_writer.aclose() | ||
await write_stream_reader.aclose() | ||
del self._read_stream_writers[session_id] |
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.
del self._read_stream_writers[session_id] | |
self._read_stream_writers.pop(session_id, None) |
Motivation and Context
Here
handle_sse
callsapp.run()
every time it processes a sse request, butapp.run()
keeps running and does not stop automatically, which causeshandle_sse
to never end, and finally causes a memory leak.python-sdk/examples/servers/simple-tool/mcp_simple_tool/server.py
Lines 67 to 73 in b4c7db6
And this is also the key reason for the following exit problems issues#541, Fixed.
INFO: Waiting for background tasks to complete. (CTRL+C to force quit)
Types of changes
Checklist