Fix hanging on streams when stdio_client exiting#559
Merged
ihrpr merged 9 commits intomodelcontextprotocol:mainfrom May 13, 2025
Merged
Fix hanging on streams when stdio_client exiting#559ihrpr merged 9 commits intomodelcontextprotocol:mainfrom
ihrpr merged 9 commits intomodelcontextprotocol:mainfrom
Conversation
Triggers a TaskGroup cancel after reaching the finally code block.
LionNatsu
changed the title
ihrpr
requested changes
Apr 30, 2025
Contributor
ihrpr
left a comment
ihrpr
left a comment
There was a problem hiding this comment.
Thank you, the clean up change looks good.
Please can you move the test to tests/client/test_stdio.py
Merged to test_stdio.py
Contributor
Author
|
You're welcome, @ihrpr. Just so you know, all requested changes are resolved :) |
ihrpr
requested changes
May 8, 2025
Use async versions of .close() Co-authored-by: ihrpr <inna.hrpr@gmail.com>
Contributor
Author
|
@ihrpr Resolved |
felixweinberger
added a commit
that referenced
this pull request
Jun 26, 2025
This change demonstrates a simpler approach than PR #555's timeout mechanism: 1. **Evidence**: Extensive testing shows PR #559 stream cleanup alone prevents hanging, even with servers that ignore SIGTERM and keep streams open. 2. **Simplification**: Removes all timeout logic and Windows-specific termination function in favor of unified `process.terminate()` + stream cleanup. 3. **Benefits**: - Less code complexity (no timeout handling, no platform branching) - Preserves proven stream cleanup protection from PR #559 - Makes behavior consistent across all platforms - All existing timeout tests still pass 4. **Risk reduction**: Avoids changing process termination semantics while maintaining hanging protection through stream cleanup. The core insight: process hanging was caused by stream management issues (solved by PR #559), not termination timing issues (targeted by PR #555).
9 tasks
9 tasks
felixweinberger
added a commit
that referenced
this pull request
Jun 26, 2025
**NOTE: These tests FAIL without the changes introduced in #559** - test_stdio_client_universal_timeout - test_stdio_client_immediate_completion await read_stream.aclose() await write_stream.aclose() await read_stream_writer.aclose() await write_stream_reader.aclose() These tests verify that stdio_client completes cleanup within reasonable time for both slow-terminating and fast-exiting processes, preventing the hanging issues reported in #559. **NOTE: This test FAILS without the changes introduced in #555** - test_stdio_client_sigint_only_process try: process.terminate() with anyio.fail_after(2.0): await process.wait() except TimeoutError: # If process doesn't terminate in time, force kill it process.kill() This test verifies that on UNIX systems MCP servers that don't respect SIGTERM but e.g. SIGINT still get terminated after a grace period.
felixweinberger
added a commit
that referenced
this pull request
Jul 1, 2025
cherry-pick of #555 Add regression tests for stdio cleanup hanging **NOTE: These tests FAIL without the changes introduced in #559** - test_stdio_client_universal_timeout - test_stdio_client_immediate_completion await read_stream.aclose() await write_stream.aclose() await read_stream_writer.aclose() await write_stream_reader.aclose() These tests verify that stdio_client completes cleanup within reasonable time for both slow-terminating and fast-exiting processes, preventing the hanging issues reported in #559. **NOTE: This test FAILS without the changes introduced in #555** - test_stdio_client_sigint_only_process try: process.terminate() with anyio.fail_after(2.0): await process.wait() except TimeoutError: # If process doesn't terminate in time, force kill it process.kill() This test verifies that on UNIX systems MCP servers that don't respect SIGTERM but e.g. SIGINT still get terminated after a grace period.
felixweinberger
added a commit
that referenced
this pull request
Jul 1, 2025
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. 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 or perform lengthy cleanup operations. Also adds regression tests for #559. resolves #555 Co-authored-by: Cristian Pufu <cristian.pufu@uipath.com>
felixweinberger
added a commit
that referenced
this pull request
Jul 4, 2025
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. 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 or perform lengthy cleanup operations. Also adds regression tests for #559. resolves #555 Co-authored-by: Cristian Pufu <cristian.pufu@uipath.com>
felixweinberger
added a commit
that referenced
this pull request
Jul 4, 2025
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>
felixweinberger
added a commit
that referenced
this pull request
Jul 7, 2025
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>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Triggers a TaskGroup cancel after reaching the finally code block.
Motivation and Context
stdin_writerin the task group hangs when pressing Ctrl+c.Now we explicitly close the streams blocking.
How Has This Been Tested?
Tested in a new simple no-op test and a real Uvicorn Starlette application.
Types of changes
Checklist