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

fix: avoid stdio cleanup BrokenResourceError race by lavish0000 · Pull Request #2219 · modelcontextprotocol/python-sdk · GitHub

fix: avoid stdio cleanup BrokenResourceError race - #2219

Closed
lavish0000 wants to merge 3 commits into
modelcontextprotocol:mainfrom
lavish0000:fix/stdio-cleanup-race-1960
Closed

fix: avoid stdio cleanup BrokenResourceError race#2219
lavish0000 wants to merge 3 commits into
modelcontextprotocol:mainfrom
lavish0000:fix/stdio-cleanup-race-1960

Conversation

Copy link
Copy Markdown

Summary

  • cancel the stdio client task group before closing its memory streams during shutdown
  • move memory stream teardown until after the task group has exited
  • add a regression test that exits while stdout_reader is blocked on a zero-buffer send

Problem

stdio_client() starts background tasks that read process stdout and forward messages into a zero-buffer memory stream. If the caller exits the context without consuming read_stream, stdout_reader can block on read_stream_writer.send(...).

Before this change, the cleanup path closed read_stream_writer while stdout_reader was still running inside the task group. That raises anyio.BrokenResourceError and surfaces as an ExceptionGroup during context exit.

Fix

The cleanup sequence now cancels the task group before any of the memory streams it owns are closed. The stream handles are closed only after the task group has exited, so stdout_reader and stdin_writer are no longer racing against stream teardown.

Validation

  • reproduced the failure locally on current main with a minimal stdio server that writes one JSON-RPC message and then idles
  • uv run pytest tests/client/test_stdio.py
  • uv run ruff check src/mcp/client/stdio.py tests/client/test_stdio.py
  • uv run ruff format --check src/mcp/client/stdio.py tests/client/test_stdio.py
  • uv run pyright src/mcp/client/stdio.py tests/client/test_stdio.py

maxisbey commented Mar 6, 2026

Copy link
Copy Markdown
Contributor

Thanks for the PR! #1960 is still labeled needs repro — could you post a reproduction on that issue first so we can confirm the root cause before reviewing a fix? The PR description here mentions reordering cleanup but the diff just widens the except clause, so a repro on the issue would help us verify which approach is actually correct.

AI Disclaimer

maxisbey closed this Mar 6, 2026

lavish0000 commented Mar 6, 2026
edited
Loading

Copy link
Copy Markdown
Author

I posted a runnable repro on #1960.

You were also right that the PR body drifted from the current diff. I'll update the PR description so it matches the implementation before asking for another review.

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants


Back | FazBrowse Home | New Git URL