FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Unify process termination on POSIX & Windows (+ tests) by felixweinberger · Pull Request #1044 · modelcontextprotocol/python-sdk · GitHub

/ python-sdk Public

Unify process termination on POSIX & Windows (+ tests) - #1044

Merged
felixweinberger merged 3 commits into
mainfrom
fweinberger/stream-cleanup-only-approach
Jul 8, 2025
ConversationCommits (3)ChecksFiles changed
Merged

Unify process termination on POSIX & Windows (+ tests)#1044
felixweinberger merged 3 commits into
mainfrom
fweinberger/stream-cleanup-only-approach

Conversation

felixweinberger commented Jun 26, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Motivation and Context

#555, #559 address important but different ways MCP servers can hang or fail to successfully terminate on Windows & POSIX systems.

This PR re-implements #555 by aligning process termination between Windows and Unix to make sure SIGTERM-ignoring processes don't inadvertently cause hangs.

How Has This Been Tested?

New regression tests added for #555 and #559. These were developed and tested on both POSIX and Windows systems.

Breaking Changes

None.

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

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

felixweinberger force-pushed the fweinberger/stream-cleanup-only-approach branch 2 times, most recently from 9f88ea7 to a27fcef Compare June 26, 2025 21:31
felixweinberger mentioned this pull request Jun 26, 2025
Closed
9 tasks
felixweinberger force-pushed the fweinberger/stream-cleanup-only-approach branch 6 times, most recently from caa5628 to 1d495ca Compare June 30, 2025 14:55
felixweinberger changed the title Add regression tests for #559 and #555 Add regression tests for #559, #555, #765 Jun 30, 2025
felixweinberger changed the title Add regression tests for #559, #555, #765 Add regression tests for #555, #559, #765 Jun 30, 2025
felixweinberger force-pushed the fweinberger/stream-cleanup-only-approach branch from 9e8d74b to 3f6c472 Compare June 30, 2025 15:39
felixweinberger mentioned this pull request Jun 30, 2025
Closed
9 tasks
felixweinberger changed the title Add regression tests for #555, #559, #765 [WIP] Add regression tests for #555, #559, #765, #729, #850 Jul 1, 2025
felixweinberger force-pushed the fweinberger/stream-cleanup-only-approach branch 15 times, most recently from cbaee2e to e8954dd Compare July 1, 2025 17:45
felixweinberger changed the title [WIP] Add regression tests for #555, #559, #765, #729, #850 Unify process termination on POSIX & Windows and add comprehensive regression testing Jul 1, 2025
felixweinberger force-pushed the fweinberger/stream-cleanup-only-approach branch 8 times, most recently from e817dfb to 7af9e65 Compare July 4, 2025 17:39

felixweinberger commented Jul 4, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Hi @Kludex, @agronholm thanks for taking a look!

For context, we actually don't spawn windows processes with anyio.open_process on Windows anymore since #596. We now use subprocess.Popen - the reason was that trying to spawn processes with anyio way was causing NotImplementedError on Windows when using the SelectorEventLoop (apparently the default for Python 3.13+ and e.g. in Streamlit, Google AgentKit google/adk-python#1321), which was blocking Windows development of MCP servers in those environments.

However, your comment made me realize that we should probably explicitly guard for that case and still use anyio by default if possible as a first try, so I added a commit on top of this PR to attempt with anyio first and only in the NotImplementedError case use the FallbackProcess wrapper.

If this could somehow be fixed upstream in anyio that would of course be fantastic - though we did get feedback from Microsoft that Python's asyncio is relatively underdeveloped on Windows, which might be a root cause why we ran into this issue in the first place and might make it difficult to address there?

felixweinberger marked this pull request as ready for review July 4, 2025 17:41
felixweinberger mentioned this pull request Jul 4, 2025
Merged
9 tasks

agronholm commented Jul 5, 2025

Copy link
Copy Markdown

Any idea why SelectorEventLoop would be the default there? It stopped being the Python default on Windows on Python 3.8.

felixweinberger and others added 2 commits July 7, 2025 14:59
Fix stdio cleanup hanging on slow-terminating processes
d15efab 
The stdio cleanup was hanging indefinitely when processes ignored
termination signals or took too long to exit. This caused the MCP
client to freeze during shutdown, especially with servers that don't
handle SIGTERM properly.

This was already being handled on Windows, but not Unix systems.
This Commit unifies the two approaches, removing special logic
for windows process termination.

The fix introduces a 2-second timeout for process termination. If a
process doesn't exit gracefully within this window, it's forcefully
killed. This ensures the client always completes cleanup in bounded
time while still giving well-behaved servers a chance to exit cleanly.

This resolves hanging issues reported when MCP servers ignore standard
termination signals.

resolves #555

Also adds regression tests for #559.

Co-authored-by: Cristian Pufu <cristian.pufu@uipath.com>
Try anyio.open_process first before Windows fallback
c9b43bc 
This re-establishes behavior before #596 in the default case.

- Attempt to use anyio's native open_process function on Windows
- Fall back to subprocess.Popen only if NotImplementedError is raised
- This improves compatibility with event loops that support async subprocesses
- Extract fallback logic into separate function for clarity
felixweinberger force-pushed the fweinberger/stream-cleanup-only-approach branch from 7af9e65 to c9b43bc Compare July 7, 2025 13:59

felixweinberger commented Jul 7, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Any idea why SelectorEventLoop would be the default there? It stopped being the Python default on Windows on Python 3.8.

@agronholm Doing some research based on what @theailanguage raised in #596:

  1. Streamlit:
  1. MCP Adapters:
  1. Jupyter Notebooks (outdated, but as examples):

It seems that while the ProactorEventLoop is the default on Python 3.8+ on Windows, that doesn't necessarily carry over to all applications that might set the event loop internally to SelectorEventLoop for some reason? In such cases this leads to a NotImplementedError

felixweinberger added a commit that referenced this pull request Jul 7, 2025
Add test demonstrating issue #1027 and verifying PR #1044 fixes it
72cfeca 
This test shows that MCP server cleanup code in lifespan doesn't run
when the process is terminated, but does run when stdin is closed first
(as implemented in PR #1044).

The test includes:
- Demonstration of current broken behavior (cleanup doesn't run)
- Verification that stdin closure allows graceful shutdown
- Windows-specific ResourceWarning handling
- Detailed documentation of the issue and solution

Github-Issue:#1027
ihrpr previously approved these changes Jul 7, 2025
View reviewed changes

ihrpr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

left a few comments

Comment thread tests/client/test_stdio.py Outdated
Comment thread src/mcp/client/stdio/win32.py
Comment thread src/mcp/client/stdio/__init__.py Outdated
Address @ihrpr review feedback
5771abc 
- Add SIGTERM_IGNORING_PROCESS_TIMEOUT constant in tests to document timeout behavior
- Add PROCESS_TERMINATION_TIMEOUT constant to replace magic number in stdio client
- Restore deprecated terminate_windows_process function with original functionality
  to maintain backward compatibility for external users

The deprecated function is marked using @deprecated decorator following the
codebase convention, while preserving its original terminate-wait-kill behavior.
felixweinberger force-pushed the fweinberger/stream-cleanup-only-approach branch from 9fd111e to 5771abc Compare July 8, 2025 10:50
felixweinberger requested a review from ihrpr July 8, 2025 11:28
felixweinberger merged commit cf72565 into main Jul 8, 2025
felixweinberger deleted the fweinberger/stream-cleanup-only-approach branch July 8, 2025 13:57
felixweinberger mentioned this pull request Jul 8, 2025
Merged
10 tasks
cristipufu mentioned this pull request Jul 12, 2025
Merged
eyurtsev mentioned this pull request Sep 10, 2025
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ihrpr ihrpr approved these changes
dsp-ant Awaiting requested review from dsp-ant
+1 more reviewer
agronholm agronholm left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL