Uh oh!
There was an error while loading. Please reload this page.
Fixes to stdio_client to support Windows more robustly - #372
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
dsp-ant
left a comment
There was a problem hiding this comment.
I really appreciate the work to make windows work. I have too little understanding of windows to have a strong opinion. Some minor comments. Throwing it back at you, but happy to merge with the minor changes that I requested.
In general it might be worthwhile to start a src/mcp/client/stdio/ folder and move stdio.py to src/mcp/client/stdio/__init__.py or whatever, so that we can have a src/mcp/client/stdio/win32.py, or even just have a helper src/mcp/platform/win32/.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| except Exception: | ||
| return command |
There was a problem hiding this comment.
What exception could happen here? Can we add at least a comment or be more specific about the exception?
There was a problem hiding this comment.
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
dsp-ant
left a comment
There was a problem hiding this comment.
sorry meant to request changes to get it back to your queue.
saqadri
commented
Mar 27, 2025
@dsp-ant thanks for the prompt review! I believe I've addressed everything |
| if sys.platform == "win32": | ||
| await terminate_windows_process(process) | ||
| else: | ||
| process.terminate() |
There was a problem hiding this comment.
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.
just as a note the fix in cristipufu's pr works on windows 11, the merged fix still hangs.
This change should fix most cases of #359, with issues spawning stdio MCP servers on Windows.
Motivation and Context
There is a somewhat longstanding issue with
stdio_cleintthat it doesn't work on Windows.Example:
Servers running with "npx" have problems because of the way "npx" command is handled in Windows (it is typically stored as a ps1 script):
Replacing "npx" with "npx.cmd" usually mitigates this, but that makes the command platform-specific.
This change adds some of that handling in the stdio_client itself, and adds helpers for processing the command and spawning a process in platform-specific ways.
How Has This Been Tested?
Tested on Windows VM in Azure using all mcp-agent examples. All of them use stdio_client, and all of them now work.
Note: for using with streamlit, I had to add an additional change which I didn't add to this PR, since it may not belong to the SDK:
In addition:
Breaking Changes
None
Types of changes
Checklist
Additional context
Open to feedback on how to make this more elegant.