Skip to content

Conversation

@hopeful0
Copy link
Contributor

@hopeful0 hopeful0 commented Jul 6, 2025

According to #1051, the context manager of the Client may cause a hang in concurrent situations. This PR refactors the methods related to the Client's context manager:

  1. Moved exception handling from __aenter__ to _connect.
  2. Adjusted the logic of _connect to ensure that the session startup is completed within _context_lock.
  3. Adjusted the logic of _disconnect to ensure that the session cleanup is completed within _context_lock.
  4. Removed unnecessary _ready_event settings in _session_runner.

These changes might seem somewhat aggressive, but I believe they should make the logic of the Client's context management clearer, thereby avoiding hard-to-detect race conditions. I would like to know your thoughts on these modifications.

@github-actions github-actions bot added the client Related to the FastMCP client SDK or client-side functionality. label Jul 6, 2025
Copy link
Owner

@jlowin jlowin left a comment

Choose a reason for hiding this comment

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

Thanks @hopeful0! This is excellent work. I've been trying to figure out why this is cropping up and appreciate the detective work in #1051 -- it stems from additional calls to list_tools() added in the low-level SDK (modelcontextprotocol/python-sdk#993). Essentially, schema validation now requires each tool call to make additional tool lists to load those schemas, which (in the case of proxy servers) forcibly creates the re-entrant situation that created this race condition.

I like your approach a lot - the only change I want to make is to remove the asserts, which I'd prefer to replace with runtime checks. I can't push to your branch so I'll do that in a subsequent PR. Thanks!

@jlowin jlowin merged commit d16e117 into jlowin:main Jul 6, 2025
4 checks passed
@jlowin jlowin added the bug Something isn't working. Reports of errors, unexpected behavior, or broken functionality. label Jul 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working. Reports of errors, unexpected behavior, or broken functionality. client Related to the FastMCP client SDK or client-side functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants