Don't close real process stdio in stdio_server (#1933)#3090
Open
steps-re wants to merge 3 commits into
Open
Conversation
…textprotocol#1933) stdio_server re-wraps sys.stdin.buffer/sys.stdout.buffer in a TextIOWrapper to force UTF-8. The wrapper's __del__ finalizer closes the buffer it wraps, so once the wrapper is garbage-collected the real sys.stdin/sys.stdout is closed and any code that runs after the server exits raises "ValueError: I/O operation on closed file." on print() or a stdout write. Dup the underlying fd (os.dup, Windows-safe) and wrap the copy, so the wrapper only ever closes the duplicate; close those wrappers on exit to free the dup'd fds. When sys.std* has no real fd (pytest capture, embedded interpreters, injected in-memory streams) fall back to wrapping .buffer and detach() the wrapper on exit, which severs it from the buffer without closing it so the finalizer can no longer close the real handle either. Adds a regression test asserting the real stdin/stdout buffers survive after the transport exits. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The no-fd fallback reached for std.buffer, which raises AttributeError on bufferless text streams such as io.StringIO. Guard with hasattr and wrap the text stream directly in that case (nothing to tear down). Adds a regression test. Addresses the cubic review on modelcontextprotocol#3090. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The bare `async with read_stream, write_stream: pass` left an uncovered exceptional-exit arc, failing the repo 100% coverage gate (surfaced on the 3.14 CI shard). Replace with explicit aclose() calls in both stdio server tests. Streams/behavior unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
transport="stdio"closes the real process stdin/stdout. Any code that runs after the server exits raises:A minimal reproduction is just a
print()afterrun(transport="stdio")returns.Root cause
src/mcp/server/stdio.pyre-wrapssys.stdin.buffer/sys.stdout.bufferin aTextIOWrapperto force UTF-8:The comment says these purposely avoid a context manager so the standard handles stay open. But a
TextIOWrappercloses the buffer it wraps in its__del__finalizer. Once the wrapper is garbage-collected,sys.stdin.buffer/sys.stdout.bufferare closed, so the realsys.stdin/sys.stdoutare dead and every later read or write fails. The wrapper defeats the very thing the comment intends.The fix
Dup the underlying fd and wrap the copy, so the wrapper only ever closes the duplicate, never the real handle.
os.dupis Windows-safe. The dup'd wrappers are closed on exit so their fds don't leak.When
sys.std*has no real fd (pytest capture, embedded interpreters, injected in-memory streams), it falls back to wrapping.bufferdirectly anddetach()es the wrapper on exit, which severs it from the buffer without closing it, so the finalizer can no longer close the real handle either. This keeps the existing in-memory / monkeypatched test paths working. This matches the approach the issue author proposed.Test
Adds a regression test in
tests/server/test_stdio.py: afterasync with stdio_server(): ...exits, the realsys.stdin/sys.stdoutbuffers must not be closed, and a write to stdout after exit must succeed. It fails onmain(AssertionError: stdio_server closed the real stdin buffer) and passes with the fix.All stdio tests pass (
pytest tests/ -k stdio→ 51 passed, 5 Windows-only skipped), including the real-subprocess round-trip interaction test.ruff check,ruff format --check, andpyrightare clean on the changed files.Fixes #1933
This was developed with AI assistance (Claude Code). I understand and have tested the change; happy to walk through any part.