fix: prevent stdio_server from closing process stdio handles - #2040
fix: prevent stdio_server from closing process stdio handles#2040adityuhkapoor wants to merge 7 commits into
Conversation
Sorry, something went wrong.
TextIOWrapper wraps sys.stdin.buffer/sys.stdout.buffer directly, so garbage-collecting the wrapper closes the real process stdio. Use os.dup() to give the wrapper its own file descriptor copy. Github-Issue:modelcontextprotocol#1933
| stdin_fd = os.dup(sys.stdin.buffer.fileno()) | ||
| stdin = anyio.wrap_file(TextIOWrapper(os.fdopen(stdin_fd, "rb"), encoding="utf-8")) | ||
| if not stdout: | ||
| stdout = anyio.wrap_file(TextIOWrapper(sys.stdout.buffer, encoding="utf-8")) | ||
| stdout_fd = os.dup(sys.stdout.buffer.fileno()) | ||
| stdout = anyio.wrap_file(TextIOWrapper(os.fdopen(stdout_fd, "wb"), encoding="utf-8")) |
There was a problem hiding this comment.
| stdin_fd = os.dup(sys.stdin.buffer.fileno()) | |
| stdin = anyio.wrap_file(TextIOWrapper(os.fdopen(stdin_fd, "rb"), encoding="utf-8")) | |
| if not stdout: | |
| stdout = anyio.wrap_file(TextIOWrapper(sys.stdout.buffer, encoding="utf-8")) | |
| stdout_fd = os.dup(sys.stdout.buffer.fileno()) | |
| stdout = anyio.wrap_file(TextIOWrapper(os.fdopen(stdout_fd, "wb"), encoding="utf-8")) | |
| stdin = anyio.wrap_file(TextIOWrapper(open(sys.stdin.fileno(), "rb", closefd=False), encoding="utf-8")) | |
| if not stdout: | |
| stdout = anyio.wrap_file(TextIOWrapper(open(sys.stdout.fileno(), "wb", closefd=False), encoding="utf-8")) |
closefd=False gets you the same isolation without having to allocate an fd, so there's nothing to clean up and the filterwarnings on the test can be deleted.
also maybe update the comment above to something like:
# Re-wrap the `fd` with `closefd=False` to force UTF-8 (Windows encoding is # platform-dependant) without taking ownership of process stdio.
Sorry, something went wrong.
There was a problem hiding this comment.
Applied, dropping import os and filter warnings, much appreciated!
Sorry, something went wrong.
|
Addressed the closefd=False suggestion and added wrapper cleanup on exit to avoid ResourceWarning. Ready for re-review whenever you get a chance @maxisbey |
Sorry, something went wrong.
|
One thing I noticed reading the cleanup path: the new try/finally around the task group awaits stdin.aclose() / stdout.aclose() without a shielded cancel scope. AsyncFile.aclose() is to_thread.run_sync(self._fp.close), which is cancellation-sensitive — if the caller's surrounding scope is already cancelled when stdio_server unwinds (e.g. SIGINT during a long write, or a parent cancel_scope.cancel() while a message is being framed), the close gets re-raised before TextIOWrapper.close() runs, the wrapper is leaked, and a ResourceWarning fires on GC. That's the same noise the closefd=False switch was meant to eliminate, just now reached via cancellation instead of fd ownership. test_1933_stdio_close.py only exercises clean exit (write end closed → EOF → normal __aexit__), so this path isn't covered. Wrapping the two aclose calls in with anyio.CancelScope(shield=True): inside the finally would close the gap, and a test that does async with stdio_server(): tg.cancel_scope.cancel() mid-iteration would lock it in. Happy to draft the test if useful. |
Sorry, something went wrong.
Reviewers
Requested changes must be addressed to merge this pull request.
Motivation and Context
Fixes #1933. stdio_server() wraps sys.stdin.buffer and sys.stdout.buffer in TextIOWrapper. When the wrapper gets garbage-collected, it closes the underlying buffer, killing process stdio. Any subsequent print() or stdin usage raises ValueError: I/O operation on closed file.
os.dup() gives the wrapper its own copy of the file descriptor, so closing it doesn't affect the original process stdio.
How Has This Been Tested?
Breaking Changes
No.
Types of changes
Checklist