Skip to content
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

Fixes to stdio_client to support Windows more robustly #372

Merged
merged 9 commits into from
Mar 27, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 99 additions & 5 deletions src/mcp/client/stdio.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import os
import shutil
import subprocess
import sys
from contextlib import asynccontextmanager
from pathlib import Path
Expand Down Expand Up @@ -101,15 +103,19 @@ async def stdio_client(server: StdioServerParameters, errlog: TextIO = sys.stder
read_stream_writer, read_stream = anyio.create_memory_object_stream(0)
write_stream, write_stream_reader = anyio.create_memory_object_stream(0)

process = await anyio.open_process(
[server.command, *server.args],
command = _get_executable_command(server.command)

# Open process with stderr piped for capture
process = await _create_platform_compatible_process(
command=command,
args=server.args,
env=(
{**get_default_environment(), **server.env}
if server.env is not None
else get_default_environment()
),
stderr=errlog,
cwd=server.cwd,
errlog=errlog,
cwd=server.cwd
)

async def stdout_reader():
Expand Down Expand Up @@ -159,4 +165,92 @@ async def stdin_writer():
):
tg.start_soon(stdout_reader)
tg.start_soon(stdin_writer)
yield read_stream, write_stream
try:
yield read_stream, write_stream
finally:
# Clean up process
try:
process.terminate()
if sys.platform == "win32":
try:
with anyio.fail_after(2.0):
await process.wait()
except TimeoutError:
# Force kill if it doesn't terminate
process.kill()
except Exception:
pass


def _get_executable_command(command: str) -> str:
"""
Get the correct executable command normalized for the current platform.

Args:
command: Base command (e.g., 'uvx', 'npx')

Returns:
List[str]: Platform-appropriate command
"""

try:
if sys.platform != "win32":
return command
else:
# For Windows, we need more sophisticated path resolution
# First check if command exists in PATH as-is
command_path = shutil.which(command)
if command_path:
return command_path

# Check for Windows-specific extensions
for ext in [".cmd", ".bat", ".exe", ".ps1"]:
ext_version = f"{command}{ext}"
ext_path = shutil.which(ext_version)
if ext_path:
return ext_path

# For regular commands or if we couldn't find special versions
return command
except Exception:
return command
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exception could happen here? Can we add at least a comment or be more specific about the exception?

Copy link
Contributor Author

@saqadri saqadri Mar 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main one I was worried about is any exceptions thrown by shutil.which(). I never hit this clause, so I can also remove it if you prefer



async def _create_platform_compatible_process(
command: str,
args: list[str],
env: dict[str, str] | None = None,
errlog: int | TextIO = subprocess.PIPE,
cwd: Path | str | None = None,
):
"""
Creates a subprocess in a platform-compatible way.
Returns a process handle.
"""

process = None

if sys.platform == "win32":
try:
process = await anyio.open_process(
[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,
stderr=errlog,
cwd=cwd,
)

return process
except Exception:
# Don't raise, let's try to create the process using the default method
process = None

# Default method for creating the process
process = await anyio.open_process(
[command, *args], env=env, stderr=errlog, cwd=cwd
)

return process
6 changes: 3 additions & 3 deletions src/mcp/server/fastmcp/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -652,9 +652,9 @@ async def read_resource(self, uri: str | AnyUrl) -> Iterable[ReadResourceContent
Returns:
The resource content as either text or bytes
"""
assert self._fastmcp is not None, (
"Context is not available outside of a request"
)
assert (
self._fastmcp is not None
), "Context is not available outside of a request"
return await self._fastmcp.read_resource(uri)

async def log(
Expand Down
4 changes: 1 addition & 3 deletions tests/shared/test_sse.py
Original file line number Diff line number Diff line change
Expand Up @@ -138,9 +138,7 @@ def server(server_port: int) -> Generator[None, None, None]:
time.sleep(0.1)
attempt += 1
else:
raise RuntimeError(
f"Server failed to start after {max_attempts} attempts"
)
raise RuntimeError(f"Server failed to start after {max_attempts} attempts")

yield

Expand Down
4 changes: 1 addition & 3 deletions tests/shared/test_ws.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,9 +134,7 @@ def server(server_port: int) -> Generator[None, None, None]:
time.sleep(0.1)
attempt += 1
else:
raise RuntimeError(
f"Server failed to start after {max_attempts} attempts"
)
raise RuntimeError(f"Server failed to start after {max_attempts} attempts")

yield

Expand Down