Skip to content

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

Merged

Conversation

akash329d
Copy link
Contributor

@akash329d akash329d commented May 2, 2025

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 session Route 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 return None. By fixing the 'running forever' issue we end up seeing exceptions related to this whenever a client disconnects. I updated mount_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 a Mount to not get "TypeError: 'NoneType' object is not callable" errors when client disconnects.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

@akash329d akash329d changed the title Handle SSE disconnects Properly Handle SSE Disconnects Properly May 2, 2025
@@ -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/"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the trailing /?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@akash329d akash329d requested a review from praboud-ant May 2, 2025 01:23
@praboud-ant praboud-ant merged commit 83968b5 into modelcontextprotocol:main May 2, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants