-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Fixes to stdio_client to support Windows more robustly #372
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
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
a78d7fc
Fixes to stdio_client to support Windows more robustly
saqadri 3b974aa
subprocess.PIPE -> sys.stderr for errlog
saqadri c289efa
Minor update to docstring
saqadri 725dc61
PR fixes
saqadri 627704a
Address PR comments
saqadri 8918b67
Merge branch 'modelcontextprotocol:main' into main
saqadri c712f04
Undo uv.lock update
saqadri 03b5266
Move process termination to win32.py as well
saqadri 9720d99
Add missing await
saqadri File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,109 @@ | ||
""" | ||
Windows-specific functionality for stdio client operations. | ||
""" | ||
|
||
import shutil | ||
import subprocess | ||
import sys | ||
from pathlib import Path | ||
from typing import TextIO | ||
|
||
import anyio | ||
from anyio.abc import Process | ||
|
||
|
||
def get_windows_executable_command(command: str) -> str: | ||
""" | ||
Get the correct executable command normalized for Windows. | ||
|
||
On Windows, commands might exist with specific extensions (.exe, .cmd, etc.) | ||
that need to be located for proper execution. | ||
|
||
Args: | ||
command: Base command (e.g., 'uvx', 'npx') | ||
|
||
Returns: | ||
str: Windows-appropriate command path | ||
""" | ||
try: | ||
# First check if command exists in PATH as-is | ||
if command_path := shutil.which(command): | ||
return command_path | ||
|
||
# Check for Windows-specific extensions | ||
for ext in [".cmd", ".bat", ".exe", ".ps1"]: | ||
ext_version = f"{command}{ext}" | ||
if ext_path := shutil.which(ext_version): | ||
return ext_path | ||
|
||
# For regular commands or if we couldn't find special versions | ||
return command | ||
except OSError: | ||
# Handle file system errors during path resolution | ||
# (permissions, broken symlinks, etc.) | ||
return command | ||
|
||
|
||
async def create_windows_process( | ||
command: str, | ||
args: list[str], | ||
env: dict[str, str] | None = None, | ||
errlog: TextIO = sys.stderr, | ||
cwd: Path | str | None = None, | ||
): | ||
""" | ||
Creates a subprocess in a Windows-compatible way. | ||
|
||
Windows processes need special handling for console windows and | ||
process creation flags. | ||
|
||
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 | ||
|
||
Returns: | ||
A process handle | ||
""" | ||
try: | ||
# Try with Windows-specific flags to hide console window | ||
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 without creation flags | ||
process = await anyio.open_process( | ||
[command, *args], env=env, stderr=errlog, cwd=cwd | ||
) | ||
return process | ||
|
||
|
||
async def terminate_windows_process(process: Process): | ||
""" | ||
Terminate a Windows process. | ||
|
||
Note: On Windows, terminating a process with process.terminate() doesn't | ||
always guarantee immediate process termination. | ||
So we give it 2s to exit, or we call process.kill() | ||
which sends a SIGKILL equivalent signal. | ||
|
||
Args: | ||
process: The process to terminate | ||
""" | ||
try: | ||
process.terminate() | ||
with anyio.fail_after(2.0): | ||
await process.wait() | ||
except TimeoutError: | ||
# Force kill if it doesn't terminate | ||
process.kill() |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still hangs on linux if the invoked process expects SIGINT, eg: https://github.com/modelcontextprotocol/servers/blob/main/src/everything/index.ts#L13
Shouldn't we try to force kill, same as we do on Windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dsp-ant check this out please: #555
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just as a note the fix in cristipufu's pr works on windows 11, the merged fix still hangs.