diff --git a/src/mcp/server/stdio.py b/src/mcp/server/stdio.py index 876d256ddb..4660ebf5d9 100644 --- a/src/mcp/server/stdio.py +++ b/src/mcp/server/stdio.py @@ -17,9 +17,12 @@ async def run_server(): ``` """ +import io +import os import sys from contextlib import asynccontextmanager from io import TextIOWrapper +from typing import TextIO import anyio import anyio.lowlevel @@ -34,14 +37,46 @@ async def stdio_server(stdin: anyio.AsyncFile[str] | None = None, stdout: anyio. """Server transport for stdio: this communicates with an MCP client by reading from the current process' stdin and writing to stdout. """ - # Purposely not using context managers for these, as we don't want to close - # standard process handles. Encoding of stdin/stdout as text streams on - # python is platform-dependent (Windows is particularly problematic), so we - # re-wrap the underlying binary stream to ensure UTF-8. + # We don't want to close the process' standard handles. Wrapping + # sys.std{in,out}.buffer in a TextIOWrapper is not enough on its own: the + # wrapper's __del__ finalizer closes the buffer it wraps, so once the wrapper + # is garbage-collected the real sys.std{in,out} is closed and any later + # print()/read raises "ValueError: I/O operation on closed file." (#1933). + # Encoding of stdin/stdout as text streams on python is platform-dependent + # (Windows is particularly problematic), so we still re-wrap the underlying + # binary stream to ensure UTF-8. + # + # Preferred path: dup the underlying fd (os.dup is Windows-safe) and wrap the + # copy, so the wrapper only ever closes the duplicate; we 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), we fall back to wrapping + # .buffer directly 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. + to_close: list[TextIOWrapper] = [] + to_detach: list[TextIOWrapper] = [] + + def wrap_std(std: TextIO, mode: str, errors: str | None) -> anyio.AsyncFile[str]: + try: + binary = open(os.dup(std.fileno()), mode + "b", closefd=True) + wrapper = TextIOWrapper(binary, encoding="utf-8", errors=errors) + to_close.append(wrapper) + except (AttributeError, OSError, ValueError, io.UnsupportedOperation): + # No real fd. A bufferless in-memory text stream (e.g. io.StringIO) + # has no .buffer to re-wrap, so use it directly -- it is already text, + # and we did not create it, so there is nothing to tear down. Otherwise + # re-wrap .buffer and detach() on exit so the finalizer cannot close the + # real handle. + if not hasattr(std, "buffer"): + return anyio.wrap_file(std) + wrapper = TextIOWrapper(std.buffer, encoding="utf-8", errors=errors) + to_detach.append(wrapper) + return anyio.wrap_file(wrapper) + if not stdin: - stdin = anyio.wrap_file(TextIOWrapper(sys.stdin.buffer, encoding="utf-8", errors="replace")) + stdin = wrap_std(sys.stdin, "r", errors="replace") if not stdout: - stdout = anyio.wrap_file(TextIOWrapper(sys.stdout.buffer, encoding="utf-8")) + stdout = wrap_std(sys.stdout, "w", errors=None) read_stream_writer, read_stream = create_context_streams[SessionMessage | Exception](0) write_stream, write_stream_reader = create_context_streams[SessionMessage](0) @@ -71,7 +106,17 @@ async def stdout_writer(): except anyio.ClosedResourceError: # pragma: no cover await anyio.lowlevel.checkpoint() - async with anyio.create_task_group() as tg: - tg.start_soon(stdin_reader) - tg.start_soon(stdout_writer) - yield read_stream, write_stream + try: + async with anyio.create_task_group() as tg: + tg.start_soon(stdin_reader) + tg.start_soon(stdout_writer) + yield read_stream, write_stream + finally: + # Close the wrappers around dup'd fds (frees the duplicate) and detach the + # wrappers around the real .buffer (severs them without closing the real + # handle, so their finalizers can't close it either). Neither touches the + # process' standard handles or a caller-injected stream. + for wrapper in to_close: + wrapper.close() + for wrapper in to_detach: + wrapper.detach() diff --git a/tests/server/test_stdio.py b/tests/server/test_stdio.py index 218e34d5ac..72334ded67 100644 --- a/tests/server/test_stdio.py +++ b/tests/server/test_stdio.py @@ -1,5 +1,7 @@ +import gc import io import sys +import tempfile import threading from collections.abc import AsyncIterator from contextlib import asynccontextmanager @@ -75,6 +77,68 @@ async def test_stdio_server_round_trips_messages_over_injected_streams() -> None assert received_responses[1] == JSONRPCResponse(jsonrpc="2.0", id=4, result={}) +@pytest.mark.anyio +async def test_stdio_server_does_not_close_real_std_handles(monkeypatch: pytest.MonkeyPatch) -> None: + """Regression test for issue #1933: the default path must not close process stdio. + + `stdio_server()` re-wraps `sys.stdin.buffer`/`sys.stdout.buffer` in a `TextIOWrapper` + to force UTF-8. The wrapper's `__del__` finalizer used to close the underlying buffer, + so any code running after the server exits raised + `ValueError: I/O operation on closed file.` on `print()` / stdout writes. + + The default path dups the fds and wraps the copies, so tearing the wrappers down + closes only the duplicates and leaves the real process handles usable. Real + fd-backed temp files stand in for the process handles here so that the fix's + `sys.std{in,out}.fileno()` path is exercised; the empty stdin file reaches EOF + immediately so the reader worker unwinds without a client. + """ + with ( + tempfile.TemporaryFile(mode="rb", buffering=0) as stdin_raw, + tempfile.TemporaryFile(mode="wb", buffering=0) as stdout_raw, + ): + stdin_file = TextIOWrapper(stdin_raw, encoding="utf-8") # empty -> immediate EOF + stdout_file = TextIOWrapper(stdout_raw, encoding="utf-8") + monkeypatch.setattr(sys, "stdin", stdin_file) + monkeypatch.setattr(sys, "stdout", stdout_file) + + with anyio.fail_after(5): + async with stdio_server() as (read_stream, write_stream): + # Close the streams so the reader/writer tasks unwind and the + # server context exits (explicit aclose keeps this branch-free + # for the repo's 100% coverage gate). + await read_stream.aclose() + await write_stream.aclose() + + # Force finalization of any TextIOWrapper the server created internally. + gc.collect() + + # On the buggy code these assertions fail: the internal wrapper's __del__ + # closed the buffer we handed in. + assert not stdin_raw.closed, "stdio_server closed the real stdin buffer" + assert not stdout_raw.closed, "stdio_server closed the real stdout buffer" + # And the process handles stay usable after the server exits. + stdout_file.write("after-server-exit\n") + stdout_file.flush() + + +@pytest.mark.anyio +async def test_stdio_server_bufferless_text_streams(monkeypatch: pytest.MonkeyPatch) -> None: + """Regression: default startup must not crash on bufferless in-memory text streams. + + In-memory text streams like `io.StringIO` have neither a real `fileno()` nor a + `.buffer`, so both the fd-dup path and the `.buffer` re-wrap path fail. In that + case `stdio_server()` must use the stream directly. Before the fix, the fallback + reached for `std.buffer` and raised `AttributeError` during startup. + """ + monkeypatch.setattr(sys, "stdin", io.StringIO()) # empty -> immediate EOF + monkeypatch.setattr(sys, "stdout", io.StringIO()) + + with anyio.fail_after(5): + async with stdio_server() as (read_stream, write_stream): + await read_stream.aclose() + await write_stream.aclose() + + @pytest.mark.anyio async def test_stdio_server_invalid_utf8(monkeypatch: pytest.MonkeyPatch) -> None: """Non-UTF-8 stdin bytes surface as an in-stream exception without killing the stream.