-
Notifications
You must be signed in to change notification settings - Fork 563
Fix: Handle SSE ping messages in WebFluxSseClientTransport #272
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
Conversation
- Add check for SSE ping messages from fastMCP server - Log debug message when ping received instead of throwing exception - Complete the stream on ping events - Matches behavior of Python SDK which treats pings as keep-alive
Hi @goecho , can you please elaborate on this. I'm not able to find |
Hi @tzolov this behavior is not defined in the MCP protocol itself—it's specific to the Python implementation using the fastmcp library. The behavior comes from the keep-alive mechanism in its dependency, the sse-starlette library. You can refer to the code here: https://github.com/sysid/sse-starlette/blob/main/sse_starlette/sse.py#L214 |
Hi @YunKuiLu ,I don't think this is a good idea — we shouldn't change the original logic. The original approach was better: throwing an exception for unknown types helps expose issues, which is better than just logging them. |
@goecho Thank you for your reply. I actually considered both options: throwing an exception to stop the client or just logging it. But since the issue is only logged in Lines 363 to 365 in d9006ec
|
I think we can combine two ways. First, expand the ping type, and then use the error handler to support any errors |
Hi @ tzolov, ping is not an event, but a comment in the sse protocol that starts with ":". The official handling method is: it should be ignored. I think PR should be merged immediately because sending ping messages is present in many frameworks. |
@chengyi3192 thanks for clarifying the semantics of the comment SSE (non)events. |
@goecho I guess we can strengthen the check to verify that the |
- Replace McpError exceptions with debug/warning logs for unrecognized SSE event types - Continue processing instead of failing when encountering unknown SSE events - Update transport implementations: - WebClientStreamableHttpTransport: return empty tuple instead of throwing - WebFluxSseClientTransport: complete stream instead of erroring - HttpClientSseClientTransport: call sink.success() instead of sink.error() - HttpClientStreamableHttpTransport: return empty Flux for unknown events This improves client resilience when servers send non-standard or future SSE event types. Resolves modelcontextprotocol#272 , modelcontextprotocol#223 , modelcontextprotocol#93 Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
…ntLine() - Strengthen the logic for detecting SSE ping comments by checking that id, event, and data fields are empty. - Generalize comment detection into isCommentLine(ServerSentEvent) for clarity and reusability. - Improves consistency with the SSE specification and matches behavior of the Python SDK. - Replaces previous hardcoded check with a more robust and maintainable solution.
@tzolov Great point — I've now added the checks for id, event, and data, and pulled the logic into an isCommentLine() helper. Thanks! |
Motivation and Context
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context