-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Fix Windows subprocess NotImplementedError (STDIO clients) #596
Conversation
Hi team! This is my first pull request. PR fixes Windows compatibility for MCP STDIO clients by introducing a fallback to subprocess.Popen wrapped with async I/O streams. It ensures that Windows users can now use streamlit and similar STDIO clients without errors. Please let me know if you'd like any further changes. Thank you for your time and for maintaining such an awesome project! Regards |
@dsp-ant @jerome3o-anthropic would appreciate any kind of feedback on this if possible from your end. Thanks a ton! Regards |
#555 solves this in a much cleaner way |
Thanks for sharing 🙏 I just tested it on my setup (Windows 11 + Python 3.13), and I’m still hitting the same error: I think I have tested #555 well by updating the init and win32 python files in my .venv mcp path with the edits suggested here. Baically #555 still uses await anyio.open_process(...) in both primary and fallback paths, which internally relies on asyncio.create_subprocess_exec(). Unfortunately, that call raises NotImplementedError on my machine, so the fallback logic never helps. In my fork, I catch the failure and bypass the entire async subprocess path by: So for Windows users with newer Python versions or CI environments, my fallback is the only one that works when the async event loop doesn’t support subprocesses. Let me know what you think. Have you tested this in event loop scenarios like the one pointed out in this PR? Thanks |
I am on windows 11; also using it in an event loop. are you using windows proactor for the async loop? edit:
import asyncio
import os
if os.name == 'nt':
asyncio.set_event_loop_policy(asyncio.WindowsProactorEventLoopPolicy()) |
Aah that explains it :) For me it is This I think is the default. I actually tried to add your session.py code but cannot get the event loop to change for some reason (I guess due to Streamlit's own eventloop). I understand #555 might work perfectly under the ProactorEventLoop, but this patch "might" help to support environments where that’s not the default
(note - personally I have faced this with my Streamlit UI only) That said, I really appreciate the clean structure of the other solution. Thanks |
One more thing, in case you'd want to look at the implementation of the streamlit code - you can have a look at Repo - https://github.com/theailanguage/mcp_client This is a tutorial video for the same that I built for a course |
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.
Thank you for working on this!
I see all the windows tests are failing, please can this be checked?
Thanks for your reply. Yes, I'll check these. |
@ihrpr I checked the failing Windows checks. This doesn’t seem to be related to the code changes I made, but rather to the test setup itself. The four checks are all failing in 2 tests out of the several others that pass. Failed Test Details Both failing tests (test_stdio_context_manager_exiting and test_stdio_client) are raising:
Location Code -
**My understanding is - ** Possible Next Steps
If not, then I believe the test files need to be updated (assuming my understanding is correct). So that might need changes to the test itself. Any suggestion on the next course of action to address this would be appreciated. Thanks |
@ihrpr I have requested a re-review based on my last comment itself because if the test may be skipped then the checks can be passed with the existing PR code itself. No changes, kindly let me know what you think. Thanks a lot! Edit: was trying to ignore the tests for windows in the same PR but reverted. Will wait for your reply :) |
@ihrpr We'd appreciate if this is prioritized higher. MCP is not usable under windows. @theailanguage thanks for the contribution. |
This is a clean rebase of PR #596 (#596) by @theailanguage with merge conflicts resolved and unrelated changes removed. THIS IS FOR REVIEW ONLY - PR #596 should be the one that gets merged. Original fix by @theailanguage implements a fallback mechanism for Windows where asyncio.create_subprocess_exec raises NotImplementedError. Uses subprocess.Popen directly with async stream wrappers to maintain compatibility. The DummyProcess class wraps the synchronous Popen object and provides the same interface as anyio.Process for seamless integration. Resolves subprocess creation issues on Windows, particularly in environments with different event loop configurations like Streamlit.
This is a clean rebase of PR #596 (#596) by @theailanguage with merge conflicts resolved and unrelated changes removed. THIS IS FOR REVIEW ONLY - PR #596 should be the one that gets merged. Original fix by @theailanguage implements a fallback mechanism for Windows where asyncio.create_subprocess_exec raises NotImplementedError. Uses subprocess.Popen directly with async stream wrappers to maintain compatibility. The DummyProcess class wraps the synchronous Popen object and provides the same interface as anyio.Process for seamless integration. Resolves subprocess creation issues on Windows, particularly in environments with different event loop configurations like Streamlit.
This is a clean rebase of PR #596 (#596) by @theailanguage with merge conflicts resolved and unrelated changes removed. THIS IS FOR REVIEW ONLY - PR #596 should be the one that gets merged. Original fix by @theailanguage implements a fallback mechanism for Windows where asyncio.create_subprocess_exec raises NotImplementedError. Uses subprocess.Popen directly with async stream wrappers to maintain compatibility. The DummyProcess class wraps the synchronous Popen object and provides the same interface as anyio.Process for seamless integration. Resolves subprocess creation issues on Windows, particularly in environments with different event loop configurations like Streamlit.
On Windows, asyncio.create_subprocess_exec raises NotImplementedError due to incomplete subprocess transport support in ProactorEventLoop. This commit implements a fallback using subprocess.Popen directly with async wrappers. The fix introduces a DummyProcess class that wraps subprocess.Popen and provides async-compatible stdin/stdout streams using anyio's FileReadStream and FileWriteStream. This allows MCP STDIO clients to work on Windows without encountering the NotImplementedError. Based on PR #596 by @theailanguage Github-Issue: #596 Reported-by: theailanguage
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.
Hi @theailanguage, I'm looking into Windows issues with the Python SDK at the moment, thank you for working on this!
Your core change makes sense to me and seems especially safe given its scoped to affect Windows specific logic only, so we should get this landed asap to improve the DevX here.
I went ahead and tested your PR by rebasing on current main
and took a best effort in resolving conflicts (result just to illustrate what I landed on based on your changes here: #1009). Windows tests were still failing, but there seems to be a relatively straightforward fix (6a703a4 closing file handles in __aexit__
) after which things appear to work and pass CI.
I also left some comments on this PR, but I realize this PR has been in queue for a while so most of these are probably not your changes but simply the PR getting out of sync with main - in that case apologies!
If you have a moment, would greatly appreciate if you could rebase your PR here to resolve conflicts, push an update, and I'd be happy to review immediately to help unblock landing this.
tests/client/test_stdio.py
Outdated
@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"] | ||
) | ||
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 | ||
with pytest.raises(McpError) as exc_info: | ||
await session.initialize() | ||
|
||
# Check that we got a connection closed error | ||
assert exc_info.value.error.code == CONNECTION_CLOSED | ||
assert "Connection closed" in exc_info.value.error.message | ||
|
||
|
||
@pytest.mark.anyio | ||
async def test_stdio_client_nonexistent_command(): | ||
"""Test that stdio_client raises an error for non-existent commands.""" | ||
# Create a server with a non-existent command | ||
server_params = StdioServerParameters( | ||
command="/path/to/nonexistent/command", | ||
args=["--help"], | ||
) | ||
|
||
# Should raise an error when trying to start the process | ||
with pytest.raises(Exception) as exc_info: | ||
async with stdio_client(server_params) as (_, _): | ||
pass | ||
|
||
# The error should indicate the command was not found | ||
error_message = str(exc_info.value) | ||
assert ( | ||
"nonexistent" in error_message | ||
or "not found" in error_message.lower() | ||
or "cannot find the file" in error_message.lower() # Windows error message | ||
) |
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.
these tests seem unrelated to this change (though I might be wrong), were they intentionally removed?
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.
Thanks for the comment. These are unrelated to the change. I have rebased to the original tests. Then I have made one change to the bad file path test
async def test_stdio_client_bad_path():
The intentional related change made this time is -
# 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"])
The args initially had a -c that instructs python to execute non-existent-file.py as a literal script with just the file name rather than simulate a bad path!
Thanks
Kartik / theailanguage
@felixweinberger thanks for the suggestions, will work on this now! Regards |
…ack to subprocess.Popen
…o_client on windows due to tee command issues
…est_stdio_client on windows due to tee command issues" This reverts commit fef614d.
a3164ae
to
5815e6c
Compare
… ResourceWarning by properly closing file handles in DummyProcess
…ng a bad inline script. This ensures the subprocess fails to launch, which better matches the test's intent.
…sLookupError. This can happen if the command failed to launch properly (as in this test case).
@felixweinberger I have rebased the code, included my original changes and made two additional changes as mentioned below. Kindly review and thanks a lot for your feedback! Regards
finally:
# Clean up process to prevent any dangling orphaned processes
if sys.platform == "win32":
await terminate_windows_process(process)
else:
- process.terminate()
+ # On Linux, terminating a process that has already exited raises ProcessLookupError.
+ # This can happen if the command failed to launch properly (as in this test case).
+ try:
+ process.terminate()
+ except ProcessLookupError:
+ # Process already exited — safe to ignore
+ pass
@pytest.mark.anyio
async def test_stdio_client_bad_path():
"""Check that the connection doesn't hang if process errors."""
+ # 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"])
- server_params = StdioServerParameters(command="python", args=["-c", "non-existent-file.py"])
async with stdio_client(server_params) as (read_stream, write_stream): |
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.
Thank you for rebasing and making these updates so quickly!
I just wanted to understand the changes outside win32.py
better.
Are these changes actually needed? I checked out a local copy of this PR again to run the tests and it seems to me like the only necessary changes are the ones in src/mcp/client/stdio/win32.py
. I ran the tests on on both Windows and macOS.
I also tried my rebased version of your fix (which only modifies win32.py
) on a Windows machine and all tests seem to pass, so curious if these other changes to stdio
are genuinely necessary or we can keep this PR isolated to win32.py
only.
Could you confirm if these are actually needed? If possible and it addresses the Windows issues for you I would strongly prefer to keep all fixes in this PR limited to the win32.py
file only.
tests/client/test_stdio.py
Outdated
|
||
# 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 comment
The 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 -c
works correctly and both versions test error handling adequately.
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.
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!
@felixweinberger I am updating the PR with just the win32.py changes - will confirm with a diff before pushing and then request to review. Thanks! |
Hi @felixweinberger, I’ve again pushed the code with only one file updated, the win32 client. You're right that all the CI tests (except one that fails for me) pass. For the one test that is failing for now (test_stdio_context_manager_exiting ) - it seems unrelated to my change (which only touches win32.py). And these tests were passing earlier. Could I request you to please see if this test fails at your end too? Please let me know if I should do anything about this one failing test! Thanks |
I triggered a rebuild and the tests pass, looks like it was just a flake (happens occasionally on the windows CI builds, known problem). |
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.
Huge thanks for all the iterations on this @theailanguage!
@felixweinberger Thanks for reviewing and the feedback! |
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
class DummyProcess: | |
class FallbackProcess: |
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.
I'll fix the naming in a follow up
ecc9165
into
modelcontextprotocol:main
Fix subprocess creation errors on Windows for STDIO client connections.
This updates
create_windows_process
to properly handle the case where asyncio cannot create subprocess transports on Windows by falling back to using thesubprocess
module manually.Additionally, minor import sorting and style issues have been fixed according to
ruff
linter checks.Motivation and Context
On Windows systems, MCP clients that connect via STDIO were failing with a
NotImplementedError
duringanyio.open_process()
. This is becauseasyncio
on Windows does not implement full support for subprocess transports.This change provides a clean workaround:
subprocess.Popen
withasyncio
wrapping.This allows MCP STDIO clients (e.g., streamlit_client_ui.py) to work successfully on Windows without changing user workflows.
edit:
the default event loop comes out as (Windows 11, Python 3.13) -
<class 'asyncio.windows_events._WindowsSelectorEventLoop'>
My understanding is that #555 might work under the ProactorEventLoop, but my patch "might" help to support environments where that’s not the default
(note - personally I have faced this with my Streamlit UI only)
How Has This Been Tested?
streamlit_client_ui.py
to launch the MCP client.ruff
orpyright
issues for the updated file.Breaking Changes
No breaking changes.
Types of changes
Checklist
Additional context
This fix is especially important for making MCP tools and agents accessible on all platforms, including Windows, thereby helping broader adoption.