Skip to content

Commit 506879f

Browse files
Simplify stdio cleanup: unified stream cleanup across platforms
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).
1 parent dced223 commit 506879f

File tree

3 files changed

+97
-24
lines changed

3 files changed

+97
-24
lines changed

src/mcp/client/stdio/__init__.py

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616
from .win32 import (
1717
create_windows_process,
1818
get_windows_executable_command,
19-
terminate_windows_process,
2019
)
2120

2221
# Environment variables to inherit by default
@@ -179,11 +178,10 @@ async def stdin_writer():
179178
yield read_stream, write_stream
180179
finally:
181180
# Clean up process to prevent any dangling orphaned processes
181+
# Unified cleanup across all platforms: simple terminate + stream cleanup
182+
# Stream cleanup (PR #559) is the key to preventing hanging behavior
182183
try:
183-
if sys.platform == "win32":
184-
await terminate_windows_process(process)
185-
else:
186-
process.terminate()
184+
process.terminate()
187185
except ProcessLookupError:
188186
# Process already exited, which is fine
189187
pass

src/mcp/client/stdio/win32.py

Lines changed: 2 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -161,22 +161,5 @@ async def create_windows_process(
161161
return FallbackProcess(popen_obj)
162162

163163

164-
async def terminate_windows_process(process: Process | FallbackProcess):
165-
"""
166-
Terminate a Windows process.
167-
168-
Note: On Windows, terminating a process with process.terminate() doesn't
169-
always guarantee immediate process termination.
170-
So we give it 2s to exit, or we call process.kill()
171-
which sends a SIGKILL equivalent signal.
172-
173-
Args:
174-
process: The process to terminate
175-
"""
176-
try:
177-
process.terminate()
178-
with anyio.fail_after(2.0):
179-
await process.wait()
180-
except TimeoutError:
181-
# Force kill if it doesn't terminate
182-
process.kill()
164+
# Windows-specific process termination function removed
165+
# Unified cleanup now uses simple process.terminate() + stream cleanup across all platforms

tests/client/test_stdio.py

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
import shutil
2+
import sys
3+
import time
24

35
import pytest
46

@@ -90,3 +92,93 @@ async def test_stdio_client_nonexistent_command():
9092
or "not found" in error_message.lower()
9193
or "cannot find the file" in error_message.lower() # Windows error message
9294
)
95+
96+
97+
@pytest.mark.anyio
98+
async def test_stdio_client_universal_timeout():
99+
"""
100+
Test that stdio_client completes cleanup within reasonable time
101+
even when connected to processes that exit slowly.
102+
"""
103+
104+
# Use a simple sleep command that's available on all platforms
105+
# This simulates a process that takes time to terminate
106+
if sys.platform == "win32":
107+
# Windows: use ping with timeout to simulate a running process
108+
server_params = StdioServerParameters(
109+
command="ping",
110+
args=["127.0.0.1", "-n", "10"], # Ping 10 times, takes ~10 seconds
111+
)
112+
else:
113+
# Unix: use sleep command
114+
server_params = StdioServerParameters(
115+
command="sleep",
116+
args=["10"], # Sleep for 10 seconds
117+
)
118+
119+
start_time = time.time()
120+
121+
try:
122+
async with stdio_client(server_params) as (read_stream, write_stream):
123+
# Immediately exit - this triggers cleanup while process is still running
124+
pass
125+
126+
end_time = time.time()
127+
elapsed = end_time - start_time
128+
129+
# Key assertion: Should complete quickly due to timeout mechanism
130+
# Before PR #555, Unix systems might hang for the full 10 seconds
131+
# After PR #555, all platforms should complete within ~2-3 seconds
132+
assert elapsed < 5.0, (
133+
f"stdio_client cleanup took {elapsed:.1f} seconds, expected < 5.0 seconds. "
134+
f"This suggests the timeout mechanism may not be working properly."
135+
)
136+
137+
except Exception as e:
138+
end_time = time.time()
139+
elapsed = end_time - start_time
140+
print(f"❌ Test failed after {elapsed:.1f} seconds: {e}")
141+
raise
142+
143+
144+
@pytest.mark.anyio
145+
async def test_stdio_client_immediate_completion():
146+
"""
147+
Test that stdio_client doesn't introduce unnecessary delays
148+
when processes exit normally and quickly.
149+
150+
This ensures PR #555's timeout mechanism doesn't slow down normal operation.
151+
"""
152+
153+
# Use a command that exits immediately
154+
if sys.platform == "win32":
155+
server_params = StdioServerParameters(
156+
command="cmd",
157+
args=["/c", "echo", "hello"], # Windows: echo and exit
158+
)
159+
else:
160+
server_params = StdioServerParameters(
161+
command="echo",
162+
args=["hello"], # Unix: echo and exit
163+
)
164+
165+
start_time = time.time()
166+
167+
try:
168+
async with stdio_client(server_params) as (read_stream, write_stream):
169+
pass
170+
171+
end_time = time.time()
172+
elapsed = end_time - start_time
173+
174+
# Should complete very quickly when process exits normally
175+
assert elapsed < 2.0, (
176+
f"stdio_client took {elapsed:.1f} seconds for fast-exiting process, "
177+
f"expected < 2.0 seconds. Timeout mechanism may be introducing delays."
178+
)
179+
180+
except Exception as e:
181+
end_time = time.time()
182+
elapsed = end_time - start_time
183+
print(f"❌ Test failed after {elapsed:.1f} seconds: {e}")
184+
raise

0 commit comments

Comments
 (0)