-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
@@ -87,7 +87,7 @@ class Settings(BaseSettings, Generic[LifespanResultT]): | |||
# HTTP settings | |||
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
EventSourceResponse
from 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 sessionRoute
to 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
ServerSession
to 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
Route
in 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_sse
to 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_sse
function to aMount
to not get"TypeError: 'NoneType' object is not callable"
errors when client disconnects.Types of changes
Checklist
Additional context