FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix uncaught exception in MCP server by ddworken · Pull Request #822 · modelcontextprotocol/python-sdk · GitHub

/ python-sdk Public

Fix uncaught exception in MCP server - #822

Merged
johnw188 merged 13 commits into
modelcontextprotocol:mainfrom
ddworken:main
Jun 10, 2025
ConversationCommits (13)ChecksFiles changed
Merged

Fix uncaught exception in MCP server#822
johnw188 merged 13 commits into
modelcontextprotocol:mainfrom
ddworken:main

Conversation

ddworken commented May 27, 2025

Copy link
Copy Markdown
Collaborator

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

  • 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

ddworken marked this pull request as ready for review May 27, 2025 20:58

ddworken commented May 27, 2025

Copy link
Copy Markdown
Collaborator Author

@johnw188

johnw188 reviewed May 27, 2025
View reviewed changes
Comment thread src/mcp/shared/session.py Outdated
Sillocan mentioned this pull request May 27, 2025
Closed

bruno-oliveira commented May 29, 2025

Copy link
Copy Markdown

So to clarify, currently this is blocking from using the streamable-http transport inside clients such as Windsurf or Claude Desktop correct?

ddworken commented May 29, 2025

Copy link
Copy Markdown
Collaborator Author

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?

Sillocan commented May 30, 2025

Copy link
Copy Markdown

@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

NAVNAV221 reviewed Jun 6, 2025
View reviewed changes
Comment thread src/mcp/shared/session.py
Comment thread src/mcp/shared/session.py
await self._handle_incoming(responder)
if not responder._completed: # type: ignore[reportPrivateUsage]
await self._handle_incoming(responder)
except Exception as e:

NAVNAV221 Jun 6, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think is better to handle a specific exceptions before the general one, such as RuntimeError (Which mentioned in this issue)

ddworken Jun 6, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Given that the risk is the server becoming unresponsive, I believe catching all exceptions to isolate errors to a single request is correct.

Kludex Jun 11, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Sillocan commented Jun 8, 2025

Copy link
Copy Markdown

I'm a bit baffled that a security issue like this is still open

Sillocan mentioned this pull request Jun 8, 2025
Merged
9 tasks
johnw188 approved these changes Jun 9, 2025
View reviewed changes

johnw188 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

johnw188 merged commit 29c69e6 into modelcontextprotocol:main Jun 10, 2025
Kludex reviewed Jun 11, 2025
View reviewed changes
Comment thread src/mcp/shared/session.py
Comment on lines +396 to +397
session_message = SessionMessage(
message=JSONRPCMessage(error_response))

Kludex Jun 11, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

How this got merged?

waylandun mentioned this pull request Jun 24, 2025
Closed
maxisbey added a commit to Varun6578/python-sdk that referenced this pull request Mar 4, 2026
Update test docstring to reference correct issue (modelcontextprotoco…
f0057a5 
…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.
maxisbey mentioned this pull request Mar 4, 2026
Merged
coderabbitai Bot mentioned this pull request Apr 4, 2026
Merged
coderabbitai Bot mentioned this pull request Apr 14, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Kludex Kludex left review comments
johnw188 johnw188 approved these changes
+1 more reviewer
NAVNAV221 NAVNAV221 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL