-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Handle SSE Disconnects Properly #612
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
Handle SSE Disconnects Properly #612
Conversation
src/mcp/server/fastmcp/server.py
Outdated
| host: str = "0.0.0.0" | ||
| port: int = 8000 | ||
| sse_path: str = "/sse" | ||
| sse_path: str = "/sse/" |
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 the trailing /?
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.
Apparently Mount makes it trailing by default.
Starlette adds a trailing slash to the mount path by default and redirects requests without a trailing slash to the path with a trailing slash.
Our test client doesn't follow redirects so didn't like it.
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.
Discussed offline; we don't want to break current users so switched back to Route but return an empty Response to get around this.
Motivation and Context
The
EventSourceResponsefrom https://github.com/sysid/sse-starlette returns on SSE session disconnect. However, since we start this in a sub task group, the current implementation does nothing when a disconnect happens. This causes the sessionRouteto stay open forever in these cases.This PR modifies this behavior to close the streams in the SSE server when EventSourceResponse wraps up, and updates
ServerSessionto properly wrap up and clean up its streams as well when the upstream streams close.This exposes a different error in that we have this set up as a
Routein most cases which shouldn't returnNone. By fixing the 'running forever' issue we end up seeing exceptions related to this whenever a client disconnects. I updatedmount_sseto return an empty response to fix this.How Has This Been Tested?
Tested manually and ensured that the route was closed when clients close the connection.
Breaking Changes
It isn't breaking, but users need to change the
handle_ssefunction to aMountto not get"TypeError: 'NoneType' object is not callable"errors when client disconnects.Types of changes
Checklist
Additional context