Fix uncaught exception in MCP server#822
Fix uncaught exception in MCP server#822johnw188 merged 13 commits intomodelcontextprotocol:mainfrom
Conversation
|
So to clarify, currently this is blocking from using the |
|
Hmm, this shouldn't be blocking any clients, it is just adding error handling for a previously unhandled exception. Have you observed issues with this PR in certain clients? |
|
@bruno-oliveira This shouldn't be blocking anyone, but it does fix a huge security issue with remote servers. Currently anyone can lock up a server just by sending a bad payload #820 |
| await self._handle_incoming(responder) | ||
| if not responder._completed: # type: ignore[reportPrivateUsage] | ||
| await self._handle_incoming(responder) | ||
| except Exception as e: |
There was a problem hiding this comment.
I think is better to handle a specific exceptions before the general one, such as RuntimeError (Which mentioned in this issue)
There was a problem hiding this comment.
Given that the risk is the server becoming unresponsive, I believe catching all exceptions to isolate errors to a single request is correct.
There was a problem hiding this comment.
I don't think this is a valid answer from the package's point of view.
It makes it considerably hard to maintain the package if everything is except Exception.
|
I'm a bit baffled that a security issue like this is still open |
| session_message = SessionMessage( | ||
| message=JSONRPCMessage(error_response)) |
…l#1227) The fix addresses stale session entries in _read_stream_writers after disconnect (202 + ClosedResourceError symptom from modelcontextprotocol#1227), not the EventSource auto-reconnect scenario from modelcontextprotocol#423 which is a client-side concern already mitigated by modelcontextprotocol#822 and modelcontextprotocol#1478.
Motivation and Context
This fixes an uncaught exception in the MCP server
How Has This Been Tested?
Via unit tests and locally.
Breaking Changes
None.
Types of changes
Checklist
Additional context