Skip to content

Conversation

@jlowin
Copy link
Owner

@jlowin jlowin commented Jul 6, 2025

Fast follow to #1054 to remove asserts (replace with runtime errors) and document the reentrant strategy. Related to #1051

Copilot AI review requested due to automatic review settings July 6, 2025 15:11
@github-actions github-actions bot added the client Related to the FastMCP client SDK or client-side functionality. label Jul 6, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the client’s session management to remove ad-hoc asserts, add detailed documentation, and implement robust reentrant async with support with reference counting and a background runner task.

  • Added comprehensive class- and method-level docstrings explaining the reentrant context logic and race-condition fixes.
  • Streamlined __aenter__, __aexit__, _connect, _disconnect, and _session_runner by removing inline assertions and centralizing error handling.
  • Introduced an explicit runtime error when the nesting counter is invalid and ensured events are created only when needed.
Comments suppressed due to low confidence (1)

src/fastmcp/client/client.py:329

  • The new guard against a nonzero nesting counter when starting a session should have dedicated tests to verify both the valid reentrant scenarios and the error path when the counter is unexpectedly nonzero.
                if self._nesting_counter != 0:

Resolved conflicts in client.py by keeping our improved RuntimeError
checks instead of the assert statements from main.
@jlowin jlowin merged commit 84adbbc into main Jul 6, 2025
9 checks passed
@jlowin jlowin deleted the concurrency-fix-1054 branch July 6, 2025 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

client Related to the FastMCP client SDK or client-side functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants