Fixes to stdio_client to support Windows more robustly#372
Fixes to stdio_client to support Windows more robustly#372dsp-ant merged 9 commits intomodelcontextprotocol:mainfrom
Conversation
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/.
src/mcp/client/stdio.py
Outdated
| 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.
|
@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.