Skip to content

fix: implemented exception handling for client indefinite blocks #500

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

fix: implemented exception handling for client indefinite blocks #500

wants to merge 13 commits into from
+115 −101

Conversation

kavinkumar807
Copy link

@kavinkumar807 kavinkumar807 commented Apr 13, 2025 •
edited
Loading

This PR fixes #447 #642

Mismatch URL

Screenshot 2025-04-13 at 9 04 39 PM

Connection Error

Screenshot 2025-04-13 at 10 14 42 PM

Motivation and Context

This PR solves the indefinite blocking issue of the client by handling the exception thrown by url mismatch and also all the exceptions happens during connection:

  • by propagating the error once value error is raised instead of sending it via stream
  • by adding exception groups to handle value error as well as connection error etc with exception groups
  • by adding errors list to add the exception groups and convert it to normal exception and throw to client
  • It also fixes the yield placement issue

How Has This Been Tested?

  • Tested with local python servers and typescript servers.
  • Tested by building the package and installing it in local usinguv pip install /dist/mcp-1.6.1.dev12+70115b9-py3-none-any.whl in the local servers.
  • Tested URL mismatch scenarios, connection errors.
  • Tested success scenarios too.

Breaking Changes

There are no breaking changes.

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

@kavinkumar807
fix: implemented exception handling for client indefinite blocks
c202846 
- propagated the error once value error is raised instead of send it to stream
- added exception groups to handle value error as well as connection error etc with exception groups
- added errors list to add the exception groups and convert it to normal exception and throw to client
@kavinkumar807
Copy link
Author

kavinkumar807 commented Apr 13, 2025 •
edited
Loading

Screenshot 2025-04-13 at 10 26 41 PM

Seems like except* is not compatible with python 3.10 version will check with it and change it

@kavinkumar807 kavinkumar807 marked this pull request as draft April 13, 2025 17:04
@kavinkumar807
fix: except* python 3.10 incompatibility issue
c2dc97f 
* added exceptiongroup dependency in toml for handling exception group issue from python 3.10 to python 3.13
* added handle_exception function to handle the exceptions
* updated the uv.lock
@kavinkumar807
fix: type check issue for Exception
ea74ee7 
- changed the handle_exception functions implementation
- replaced the Exception parameter with BaseExceptionGroup[Exception]
@kavinkumar807 kavinkumar807 marked this pull request as ready for review April 14, 2025 06:02
@kavinkumar807 kavinkumar807 mentioned this pull request Apr 14, 2025
Open
@kavinkumar807
Copy link
Author

@jerome3o-anthropic @jspahrsummers @dsp-ant Adding here for PR review

@kavinkumar807
Copy link
Author

kavinkumar807 commented Apr 16, 2025 •
edited
Loading

@Kludex Synced the PR. It’s up to date with main

@kavinkumar807
Copy link
Author

@Kludex @jerome3o-anthropic Synced the PR. It's up to date with main

@kavinkumar807
Copy link
Author

@Kludex Synced the PR.

@kavinkumar807
Copy link
Author

@dsp-ant @jerome3o-anthropic @Kludex Synced the PR with main

@kavinkumar807
Copy link
Author

@dsp-ant @jerome3o-anthropic @Kludex @ihrpr Synced the PR with main. Guys I've been following this PR and syncing for past 3 weeks I'm not getting any feedback and suggestions on this. I'd be happy to know the status of this PR

@kavinkumar807
Copy link
Author

@dsp-ant @jerome3o-anthropic @Kludex @ihrpr Synced the PR with main.

@kavinkumar807
Copy link
Author

@dsp-ant @jerome3o-anthropic @Kludex @ihrpr Synced the PR with main.

@modelcontextprotocol modelcontextprotocol locked as spam and limited conversation to collaborators May 7, 2025
@modelcontextprotocol modelcontextprotocol unlocked this conversation May 7, 2025
@kavinkumar807
Copy link
Author

I'm not sure why test cases are failing. @ihrpr

mcp-shadow[bot]
mcp-shadow bot suggested changes May 13, 2025
View reviewed changes
Copy link

@mcp-shadow mcp-shadow bot left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

The PR addresses a real issue that should be fixed, but the current implementation is overly complex and introduces unnecessary dependencies. Here's my recommendation:

  1. Simplify exception handling:
    - Use specific exception types instead of catching all exceptions
    - Preserve original exception types and tracebacks
    - Consider using regular try/except instead of ExceptionGroups
  2. Maintain MCP client configuration:
    - Keep using create_mcp_http_client instead of raw httpx.AsyncClient
    - If there's a reason for the change, document it
  3. Improve code clarity:
    - Reduce nesting levels
    - Add comments explaining the exception flow
    - Consider extracting some logic into helper functions
  4. Add tests:
    - The PR needs unit tests specifically for the error scenarios
    - Test cases for URL mismatch, connection errors, etc.
  5. Fix the handle_exception function:
    - Either make it return a string or update the type signature
    - Preserve the original exception context

@ihrpr ihrpr added this to the r-05-25 milestone May 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcp-shadow mcp-shadow[bot] mcp-shadow[bot] requested changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
r-05-25
Development

Successfully merging this pull request may close these issues.

sse_client blocks indefinitely when server has incorrect base URL
2 participants
@kavinkumar807 @ihrpr