-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Fix Windows subprocess NotImplementedError (STDIO clients) #596
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 12 commits
9f5eecd
306b28d
2f26efd
5b98439
02b3d3c
a550b02
5815e6c
dddff2b
9e38ab5
249d81e
d5e46c6
0629a3f
c8eaeba
d9863c3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -6,10 +6,12 @@ | |||||
import subprocess | ||||||
import sys | ||||||
from pathlib import Path | ||||||
from typing import TextIO | ||||||
from typing import BinaryIO, TextIO, cast | ||||||
|
||||||
import anyio | ||||||
from anyio import to_thread | ||||||
from anyio.abc import Process | ||||||
from anyio.streams.file import FileReadStream, FileWriteStream | ||||||
|
||||||
|
||||||
def get_windows_executable_command(command: str) -> str: | ||||||
|
@@ -44,49 +46,122 @@ def get_windows_executable_command(command: str) -> str: | |||||
return command | ||||||
|
||||||
|
||||||
class DummyProcess: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll fix the naming in a follow up |
||||||
""" | ||||||
A fallback process wrapper for Windows to handle async I/O | ||||||
when using subprocess.Popen, which provides sync-only FileIO objects. | ||||||
|
||||||
This wraps stdin and stdout into async-compatible | ||||||
streams (FileReadStream, FileWriteStream), | ||||||
so that MCP clients expecting async streams can work properly. | ||||||
""" | ||||||
|
||||||
def __init__(self, popen_obj: subprocess.Popen[bytes]): | ||||||
self.popen: subprocess.Popen[bytes] = popen_obj | ||||||
self.stdin_raw = popen_obj.stdin # type: ignore[assignment] | ||||||
self.stdout_raw = popen_obj.stdout # type: ignore[assignment] | ||||||
self.stderr = popen_obj.stderr # type: ignore[assignment] | ||||||
|
||||||
self.stdin = FileWriteStream(cast(BinaryIO, self.stdin_raw)) if self.stdin_raw else None | ||||||
self.stdout = FileReadStream(cast(BinaryIO, self.stdout_raw)) if self.stdout_raw else None | ||||||
|
||||||
async def __aenter__(self): | ||||||
"""Support async context manager entry.""" | ||||||
return self | ||||||
|
||||||
async def __aexit__( | ||||||
self, | ||||||
exc_type: BaseException | None, | ||||||
exc_val: BaseException | None, | ||||||
exc_tb: object | None, | ||||||
) -> None: | ||||||
"""Terminate and wait on process exit inside a thread.""" | ||||||
self.popen.terminate() | ||||||
await to_thread.run_sync(self.popen.wait) | ||||||
|
||||||
# Close the file handles to prevent ResourceWarning | ||||||
if self.stdin: | ||||||
await self.stdin.aclose() | ||||||
if self.stdout: | ||||||
await self.stdout.aclose() | ||||||
if self.stdin_raw: | ||||||
self.stdin_raw.close() | ||||||
if self.stdout_raw: | ||||||
self.stdout_raw.close() | ||||||
if self.stderr: | ||||||
self.stderr.close() | ||||||
|
||||||
async def wait(self): | ||||||
"""Async wait for process completion.""" | ||||||
return await to_thread.run_sync(self.popen.wait) | ||||||
|
||||||
def terminate(self): | ||||||
"""Terminate the subprocess immediately.""" | ||||||
return self.popen.terminate() | ||||||
|
||||||
def kill(self) -> None: | ||||||
"""Kill the subprocess immediately (alias for terminate).""" | ||||||
self.terminate() | ||||||
|
||||||
|
||||||
# ------------------------ | ||||||
# Updated function | ||||||
# ------------------------ | ||||||
|
||||||
|
||||||
async def create_windows_process( | ||||||
command: str, | ||||||
args: list[str], | ||||||
env: dict[str, str] | None = None, | ||||||
errlog: TextIO = sys.stderr, | ||||||
errlog: TextIO | None = sys.stderr, | ||||||
cwd: Path | str | None = None, | ||||||
): | ||||||
) -> DummyProcess: | ||||||
""" | ||||||
Creates a subprocess in a Windows-compatible way. | ||||||
|
||||||
Windows processes need special handling for console windows and | ||||||
process creation flags. | ||||||
On Windows, asyncio.create_subprocess_exec has incomplete support | ||||||
(NotImplementedError when trying to open subprocesses). | ||||||
Therefore, we fallback to subprocess.Popen and wrap it for async usage. | ||||||
|
||||||
Args: | ||||||
command: The command to execute | ||||||
args: Command line arguments | ||||||
env: Environment variables | ||||||
errlog: Where to send stderr output | ||||||
cwd: Working directory for the process | ||||||
command (str): The executable to run | ||||||
args (list[str]): List of command line arguments | ||||||
env (dict[str, str] | None): Environment variables | ||||||
errlog (TextIO | None): Where to send stderr output (defaults to sys.stderr) | ||||||
cwd (Path | str | None): Working directory for the subprocess | ||||||
|
||||||
Returns: | ||||||
A process handle | ||||||
DummyProcess: Async-compatible subprocess with stdin and stdout streams | ||||||
""" | ||||||
try: | ||||||
# Try with Windows-specific flags to hide console window | ||||||
process = await anyio.open_process( | ||||||
# Try launching with creationflags to avoid opening a new console window | ||||||
popen_obj = subprocess.Popen( | ||||||
[command, *args], | ||||||
env=env, | ||||||
# Ensure we don't create console windows for each process | ||||||
creationflags=subprocess.CREATE_NO_WINDOW # type: ignore | ||||||
if hasattr(subprocess, "CREATE_NO_WINDOW") | ||||||
else 0, | ||||||
stdin=subprocess.PIPE, | ||||||
stdout=subprocess.PIPE, | ||||||
stderr=errlog, | ||||||
env=env, | ||||||
cwd=cwd, | ||||||
bufsize=0, # Unbuffered output | ||||||
creationflags=getattr(subprocess, "CREATE_NO_WINDOW", 0), | ||||||
) | ||||||
return process | ||||||
return DummyProcess(popen_obj) | ||||||
|
||||||
except Exception: | ||||||
# Don't raise, let's try to create the process without creation flags | ||||||
process = await anyio.open_process([command, *args], env=env, stderr=errlog, cwd=cwd) | ||||||
return process | ||||||
# If creationflags failed, fallback without them | ||||||
popen_obj = subprocess.Popen( | ||||||
[command, *args], | ||||||
stdin=subprocess.PIPE, | ||||||
stdout=subprocess.PIPE, | ||||||
stderr=errlog, | ||||||
env=env, | ||||||
cwd=cwd, | ||||||
bufsize=0, | ||||||
) | ||||||
return DummyProcess(popen_obj) | ||||||
|
||||||
|
||||||
async def terminate_windows_process(process: Process): | ||||||
async def terminate_windows_process(process: Process | DummyProcess): | ||||||
""" | ||||||
Terminate a Windows process. | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,7 +57,12 @@ async def test_stdio_client(): | |
@pytest.mark.anyio | ||
async def test_stdio_client_bad_path(): | ||
"""Check that the connection doesn't hang if process errors.""" | ||
server_params = StdioServerParameters(command="python", args=["-c", "non-existent-file.py"]) | ||
|
||
# Removed `-c` to simulate a real "file not found" error instead of | ||
# executing a bad inline script. | ||
# This ensures the subprocess fails to launch, which better matches | ||
# the test's intent. | ||
server_params = StdioServerParameters(command="python", args=["non-existent-file.py"]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are the changes to this file necessary at all? The original test with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No this is not strictly needed. My understanding was that this is a bad path test and the intention was for non-existent-file.py to act as a file name that does not exist. The -c flag makes it into a literal command like a print statement so I thought to include this as well. I will remove and try to push without this. Thanks! |
||
async with stdio_client(server_params) as (read_stream, write_stream): | ||
async with ClientSession(read_stream, write_stream) as session: | ||
# The session should raise an error when the connection closes | ||
|
Uh oh!
There was an error while loading. Please reload this page.