Skip to content

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

Conversation

theailanguage
Copy link
Contributor

@theailanguage theailanguage commented Apr 28, 2025

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 the subprocess 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 during anyio.open_process(). This is because asyncio on Windows does not implement full support for subprocess transports.

This change provides a clean workaround:

  • Attempt async process creation first.
  • If not supported, fall back to classic subprocess.Popen with asyncio 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

  • Frameworks like Streamlit, which may run their own event loop internally
  • Python 3.13 and above (where SelectorEventLoop is often the default on Windows)
  • CI setups or test runners that don’t customize the loop policy
    (note - personally I have faced this with my Streamlit UI only)

How Has This Been Tested?

  • Tested manually on Windows 11, Python 3.13.
  • Ran streamlit_client_ui.py to launch the MCP client.
  • Successfully connected to STDIO MCP servers without errors.
  • Validated tool loading and interaction without subprocess creation issues.
  • Verified no new ruff or pyright issues for the updated file.

Breaking Changes

No breaking changes.

  • The fallback mechanism is backward compatible.
  • Linux/macOS users are unaffected.
  • Windows users gain compatibility without needing to change their code.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update (style improvements, minor doc updates)

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

This fix is especially important for making MCP tools and agents accessible on all platforms, including Windows, thereby helping broader adoption.

@theailanguage
Copy link
Contributor Author

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
[Kartik / theailanguage]

@theailanguage
Copy link
Contributor Author

@dsp-ant @jerome3o-anthropic would appreciate any kind of feedback on this if possible from your end. Thanks a ton!

Regards
[Kartik / theailanguage]

@BenMawnMahlauNBTC
Copy link

#555 solves this in a much cleaner way

@theailanguage
Copy link
Contributor Author

theailanguage commented May 7, 2025

#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:
raise NotImplementedError

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:
Falling back to a sync subprocess.Popen
Then manually wrapping stdin, stdout in FileWriteStream and FileReadStream
This makes it work regardless of the event loop or OS internals

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
Kartik / theailanguage

@BenMawnMahlauNBTC
Copy link

BenMawnMahlauNBTC commented May 7, 2025

I am on windows 11; also using it in an event loop. are you using windows proactor for the async loop?

edit:
I also have this change in the version i am using

mcp\client\session.py,

import asyncio
import os
if os.name == 'nt':
  asyncio.set_event_loop_policy(asyncio.WindowsProactorEventLoopPolicy())

@theailanguage
Copy link
Contributor Author

theailanguage commented May 7, 2025

I am on windows 11; also using it in an event loop. are you using windows proactor for the async loop?

edit: I also have this change in the version i am using

mcp\client\session.py,

import asyncio
import os
if os.name == 'nt':
  asyncio.set_event_loop_policy(asyncio.WindowsProactorEventLoopPolicy())

Aah that explains it :)

For me it is
<class 'asyncio.windows_events._WindowsSelectorEventLoop'>

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

  • Frameworks like Streamlit, which may run their own event loop internally
  • Python 3.13 and above (where SelectorEventLoop is often the default on Windows)
  • CI setups or test runners that don’t customize the loop policy

(note - personally I have faced this with my Streamlit UI only)

That said, I really appreciate the clean structure of the other solution.

Thanks
Kartik / theailanguage

@theailanguage
Copy link
Contributor Author

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
UI code - https://github.com/theailanguage/mcp_client/blob/main/streamlit_client_ui.py

This is a tutorial video for the same that I built for a course
https://youtu.be/Ln-Tgz8Pmek

@ihrpr ihrpr modified the milestones: r-05-25, stdio shutdown May 28, 2025
ihrpr
ihrpr previously requested changes May 28, 2025
Copy link
Contributor

@ihrpr ihrpr left a 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?

@theailanguage
Copy link
Contributor Author

Thanks for your reply. Yes, I'll check these.

@theailanguage
Copy link
Contributor Author

@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:

PytestUnraisableExceptionWarning: Exception ignored in: <_io.FileIO [closed]>
ResourceWarning: unclosed file

Location
tests\client\test_stdio.py:12 and
tests\client\test_stdio.py:19 on win32.

Code -

@pytest.mark.anyio
@pytest.mark.skipif(tee is None, reason="could not find tee command")
async def test_stdio_context_manager_exiting():
    ...

@pytest.mark.anyio
@pytest.mark.skipif(tee is None, reason="could not find tee command")
async def test_stdio_client():
    ...

**My understanding is - **
It appears the issue is with the use of the tee command. I believe on Windows, tee equivalent may not behave reliably with subprocess handling. Note sure what "tee" the CI on GitHub uses but these may not close file handles cleanly, leading to the ResourceWarning.

Possible Next Steps
I just want to check if that is the intention - to skip on windows? One possible solution in this case could be to explicitly skip these tests on Windows using something like:

@pytest.mark.skipif(
    tee is None or sys.platform.startswith("win"),
    reason="tee command not available or platform is Windows"
)

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
Kartik / theailanguage

@theailanguage theailanguage requested a review from ihrpr May 28, 2025 19:56
@theailanguage
Copy link
Contributor Author

theailanguage commented May 28, 2025

@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 :)

@intval
Copy link

intval commented Jun 12, 2025

@ihrpr We'd appreciate if this is prioritized higher. MCP is not usable under windows.
Same issue occurs when using google agent development kit, not only streamlit.
google/adk-python#1321

@theailanguage thanks for the contribution.

felixweinberger added a commit that referenced this pull request Jun 23, 2025
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.
felixweinberger added a commit that referenced this pull request Jun 23, 2025
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.
felixweinberger added a commit that referenced this pull request Jun 23, 2025
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.
felixweinberger added a commit that referenced this pull request Jun 23, 2025
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
Copy link
Contributor

@felixweinberger felixweinberger left a 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.

Comment on lines 61 to 92
@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
)
Copy link
Contributor

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?

Copy link
Contributor Author

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

@theailanguage
Copy link
Contributor Author

@felixweinberger thanks for the suggestions, will work on this now!

Regards
Kartik / theailanguage

…est_stdio_client on windows due to tee command issues"

This reverts commit fef614d.
…ogic

Revert "Merge branch 'main' into fix/windows_stdio_subprocess"

This reverts commit d3e0975, reversing
changes made to 1c6c6fb.
@theailanguage theailanguage force-pushed the fix/windows_stdio_subprocess branch from a3164ae to 5815e6c Compare June 24, 2025 08:34
… 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).
@theailanguage
Copy link
Contributor Author

theailanguage commented Jun 24, 2025

@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
Kartik / theailanguage

  1. src\mcp\client\stdio\__init__.py
        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
  1. tests\client\test_stdio.py
@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):

Copy link
Contributor

@felixweinberger felixweinberger left a 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.

Comment on lines 60 to 65

# 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"])
Copy link
Contributor

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.

Copy link
Contributor Author

@theailanguage theailanguage Jun 24, 2025

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!

@theailanguage
Copy link
Contributor Author

@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!

@theailanguage
Copy link
Contributor Author

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
Kartik / theailanguage

@felixweinberger
Copy link
Contributor

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 Kartik / theailanguage

I triggered a rebuild and the tests pass, looks like it was just a flake (happens occasionally on the windows CI builds, known problem).

Copy link
Contributor

@felixweinberger felixweinberger left a 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!

@theailanguage
Copy link
Contributor Author

@felixweinberger Thanks for reviewing and the feedback!

@@ -44,49 +46,122 @@ def get_windows_executable_command(command: str) -> str:
return command


class DummyProcess:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
class DummyProcess:
class FallbackProcess:

Copy link
Contributor

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

@felixweinberger felixweinberger merged commit ecc9165 into modelcontextprotocol:main Jun 24, 2025
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants