python-sdk icon indicating copy to clipboard operation
python-sdk copied to clipboard

Fix Windows subprocess NotImplementedError (STDIO clients)

Open theailanguage opened this issue 8 months ago • 10 comments

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 https://github.com/modelcontextprotocol/python-sdk/pull/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

  • [x] 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)
  • [x] Documentation update (style improvements, minor doc updates)

Checklist

  • [x] I have read the MCP Documentation
  • [x] My code follows the repository's style guidelines
  • [x] New and existing tests pass locally
  • [x] I have added appropriate error handling
  • [x] 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 avatar Apr 28 '25 13:04 theailanguage

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 avatar Apr 28 '25 14:04 theailanguage

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

Regards [Kartik / theailanguage]

theailanguage avatar Apr 29 '25 02:04 theailanguage

https://github.com/modelcontextprotocol/python-sdk/pull/555 solves this in a much cleaner way

BenMawnMahlauNBTC avatar May 07 '25 17:05 BenMawnMahlauNBTC

#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

theailanguage avatar May 07 '25 19:05 theailanguage

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())

BenMawnMahlauNBTC avatar May 07 '25 19:05 BenMawnMahlauNBTC

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 avatar May 07 '25 19:05 theailanguage

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

theailanguage avatar May 07 '25 20:05 theailanguage

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

theailanguage avatar May 28 '25 13:05 theailanguage

@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 avatar May 28 '25 19:05 theailanguage

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

theailanguage avatar May 28 '25 19:05 theailanguage

@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. https://github.com/google/adk-python/issues/1321

@theailanguage thanks for the contribution.

intval avatar Jun 12 '25 07:06 intval

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

Regards Kartik / theailanguage

theailanguage avatar Jun 24 '25 07:06 theailanguage

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

theailanguage avatar Jun 24 '25 11:06 theailanguage

@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 avatar Jun 24 '25 12:06 theailanguage

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

theailanguage avatar Jun 24 '25 13:06 theailanguage

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).

felixweinberger avatar Jun 24 '25 13:06 felixweinberger

@felixweinberger Thanks for reviewing and the feedback!

theailanguage avatar Jun 24 '25 13:06 theailanguage