Fix hanging on streams when stdio_client exiting #559

LionNatsu · 2025-04-22T07:51:00Z

Triggers a TaskGroup cancel after reaching the finally code block.

Motivation and Context

stdin_writer in 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

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

Triggers a TaskGroup cancel after reaching the finally code block.

ihrpr

Thank you, the clean up change looks good.
Please can you move the test to tests/client/test_stdio.py

tests/client/test_stdio_cm.py

Merged to test_stdio.py

LionNatsu · 2025-05-08T06:30:52Z

You're welcome, @ihrpr. Just so you know, all requested changes are resolved :)

src/mcp/client/stdio/__init__.py

Use async versions of .close() Co-authored-by: ihrpr <[email protected]>

LionNatsu · 2025-05-10T17:49:15Z

@ihrpr Resolved

ihrpr

Thank you!

This commit demonstrates that PR #559's stream cleanup alone is sufficient to prevent hanging, even with servers that ignore SIGTERM. Testing shows: 1. PR #559 stream cleanup alone handles all tested hanging scenarios 2. Timeout mechanism may be unnecessary additional complexity 3. Existing Unix behavior works fine without timeouts 4. Original timeout tests still pass, confirming no regression This simpler approach preserves Unix behavior while eliminating platform-specific timeout handling complexity. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>

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

Copy of #555 Testing shows PR #559 stream cleanup alone prevents hanging, even with servers that ignore SIGTERM and keep streams open. Removes all timeout logic and Windows-specific termination function in favor of unified `process.terminate()`.

NOTE: These tests FAIL without the changes introduced in #559. 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: 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()

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

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.

@cristipufu

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. Original PR: #555 by @cristipufu Github-Issue:#559 Github-Issue:#555

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

@cristipufu

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

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 <[email protected]>

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 <[email protected]>

LionNatsu added 6 commits April 22, 2025 14:46

Fix the TaskGroup hanging on stdin_writer when stdio_client exiting

22e2b3a

Triggers a TaskGroup cancel after reaching the finally code block.

Add test_stdio_cm for stdin_writer hanging

2774496

Test using simple tee instead of mcp-server-fetch

2591f80

Close write_stream_reader in finally block

474f072

Fix pyright warnings in test_stdio_cm.py

d2f1146

Fix read_stream stream leakage

12d4740

LionNatsu changed the title ~~Fix the TaskGroup hanging on stdin_writer when stdio_client exiting~~ Fix hanging on streams when stdio_client exiting Apr 22, 2025

ihrpr added this to the r-05-25 milestone Apr 29, 2025

ihrpr requested changes Apr 30, 2025

View reviewed changes

tests/client/test_stdio_cm.py Outdated Show resolved Hide resolved

tests/client/test_stdio_cm.py Outdated Show resolved Hide resolved

LionNatsu added 2 commits May 8, 2025 14:26

Update test_stdio.py

15b24eb

Delete tests/client/test_stdio_cm.py

a2e3328

Merged to test_stdio.py

ihrpr requested changes May 8, 2025

View reviewed changes

src/mcp/client/stdio/__init__.py Outdated Show resolved Hide resolved

Update src/mcp/client/stdio/__init__.py

a59995e

Use async versions of .close() Co-authored-by: ihrpr <[email protected]>

ihrpr approved these changes May 13, 2025

View reviewed changes

ihrpr merged commit c578212 into modelcontextprotocol:main May 13, 2025
6 checks passed

LionNatsu deleted the patch-1 branch May 13, 2025 18:55

felixweinberger mentioned this pull request Jun 26, 2025

Alternative: Stream cleanup only (no timeout mechanism) #1042

Closed

felixweinberger mentioned this pull request Jun 26, 2025

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

Open

9 tasks

felixweinberger mentioned this pull request Jun 26, 2025

Fix stdio_client kill process after timeout #555

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix hanging on streams when stdio_client exiting #559

Fix hanging on streams when stdio_client exiting #559

Uh oh!

LionNatsu commented Apr 22, 2025 •

edited

Loading

Uh oh!

ihrpr left a comment

Uh oh!

Uh oh!

Uh oh!

LionNatsu commented May 8, 2025

Uh oh!

Uh oh!

LionNatsu commented May 10, 2025

Uh oh!

ihrpr left a comment

Uh oh!

Uh oh!

Uh oh!

Fix hanging on streams when stdio_client exiting #559

Fix hanging on streams when stdio_client exiting #559

Uh oh!

Conversation

LionNatsu commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist

Uh oh!

ihrpr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

LionNatsu commented May 8, 2025

Uh oh!

Uh oh!

LionNatsu commented May 10, 2025

Uh oh!

ihrpr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

LionNatsu commented Apr 22, 2025 •

edited

Loading