Skip to content

Commit ffed951

Browse files
committed
fix(client/stdio): allow FIFO cleanup of multiple transports on asyncio
stdio_client wrapped its reader/writer in anyio.create_task_group() inside the async context manager. The task group binds its cancel scope to the task that opened the transport, and anyio then requires that task to close transports LIFO. Independent AsyncExitStacks, multi-client managers, and unordered pytest fixtures close FIFO and hit: RuntimeError: Attempted to exit cancel scope in a different task On asyncio, spawn the pipe tasks with asyncio.ensure_future so they are not stacked on the caller's cancel-scope. Shutdown order is unchanged. Trio still uses a task group and still requires LIFO. Regression tests pin oldest-first and newest-first teardown, and that an unhandled pipe-task failure still surfaces. Fixes #577
1 parent 56af447 commit ffed951

2 files changed

Lines changed: 124 additions & 10 deletions

File tree

src/mcp/client/stdio.py

Lines changed: 67 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,14 @@
88
process nor hang on one.
99
"""
1010

11+
import asyncio
1112
import logging
1213
import os
1314
import sys
14-
from collections.abc import AsyncGenerator
15+
from collections.abc import AsyncGenerator, Callable, Coroutine
1516
from contextlib import asynccontextmanager, suppress
1617
from pathlib import Path
17-
from typing import Literal, TextIO
18+
from typing import Any, Literal, TextIO
1819

1920
import anyio
2021
import anyio.lowlevel
@@ -130,7 +131,7 @@ async def stdio_client(
130131
cwd=server.cwd,
131132
)
132133

133-
# The spawn succeeded; no awaits until the task group is entered, or a
134+
# The spawn succeeded; no awaits until the pipe tasks are running, or a
134135
# cancellation delivered in the gap would leak the live process.
135136
read_stream_writer, read_stream = anyio.create_memory_object_stream[SessionMessage | Exception](0)
136137
write_stream, write_stream_reader = anyio.create_memory_object_stream[SessionMessage](0)
@@ -197,9 +198,7 @@ async def shutdown() -> None:
197198
# One pass so unblocked tasks exit via their except paths before the cancel.
198199
await anyio.lowlevel.checkpoint()
199200

200-
async with anyio.create_task_group() as tg:
201-
tg.start_soon(stdout_reader)
202-
tg.start_soon(stdin_writer)
201+
async with _run_pipe_tasks(stdout_reader, stdin_writer) as cancel_pipe_tasks:
203202
try:
204203
yield read_stream, write_stream
205204
finally:
@@ -210,11 +209,72 @@ async def shutdown() -> None:
210209
with anyio.CancelScope(shield=True):
211210
await shutdown()
212211
# Unstick pipe tasks a kill survivor's open pipe end could still block.
213-
tg.cancel_scope.cancel()
212+
cancel_pipe_tasks()
214213
# The cancel lands via throw(); one yield resyncs 3.11 coverage (gh-106749).
215214
await anyio.lowlevel.cancel_shielded_checkpoint()
216215

217216

217+
@asynccontextmanager
218+
async def _run_pipe_tasks(
219+
*pipes: Callable[[], Coroutine[Any, Any, None]],
220+
) -> AsyncGenerator[Callable[[], None], None]:
221+
"""Runs the pipe tasks for the duration of the body, yielding their canceller.
222+
223+
On asyncio they are plain asyncio tasks rather than an anyio task group: a task
224+
group binds its cancel scope to the task that opened the transport, and anyio then
225+
requires that task to close its transports in the reverse of the order it opened
226+
them. Callers holding several servers (a multi-server manager, independent exit
227+
stacks, pytest fixtures) legitimately close them in other orders, and got a
228+
"cancel scope" RuntimeError instead of a clean teardown -- see #577. Trio cannot
229+
spawn a task outside a nursery, so it keeps the task group and still requires
230+
LIFO closing.
231+
"""
232+
if not _on_asyncio():
233+
async with anyio.create_task_group() as tg:
234+
for pipe in pipes:
235+
tg.start_soon(pipe)
236+
yield tg.cancel_scope.cancel
237+
return
238+
239+
tasks = [asyncio.ensure_future(pipe()) for pipe in pipes]
240+
241+
def cancel_pipe_tasks() -> None:
242+
for task in tasks:
243+
task.cancel()
244+
245+
errors: list[Exception] = []
246+
try:
247+
yield cancel_pipe_tasks
248+
finally:
249+
cancel_pipe_tasks()
250+
# Shielded, as the task group's own reaping was: a cancelled caller must
251+
# still leave no pipe task behind. Every task is awaited before anything is
252+
# re-raised, so a second failure cannot surface as an unretrieved exception.
253+
with anyio.CancelScope(shield=True):
254+
for task in tasks:
255+
try:
256+
await task
257+
except asyncio.CancelledError:
258+
pass # our own cancellation above, not a failure
259+
except Exception as exc: # the pipe tasks' top-level handler
260+
errors.append(exc)
261+
# Outside the finally: a body that raised keeps its own exception.
262+
if errors:
263+
raise errors[0]
264+
265+
266+
def _on_asyncio() -> bool:
267+
"""Whether the caller is running on anyio's asyncio backend.
268+
269+
True exactly when an asyncio task is executing, which is when spawning further
270+
asyncio tasks is meaningful; on trio there is no running loop to ask.
271+
"""
272+
try:
273+
return asyncio.current_task() is not None
274+
except RuntimeError:
275+
return False
276+
277+
218278
def _parse_line(line: str) -> SessionMessage | Exception:
219279
"""Parses one stdout line, returning parse errors as values for the session to surface."""
220280
try:

tests/client/test_stdio.py

Lines changed: 57 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -189,15 +189,16 @@ def pending_stdout_chunks(self) -> int:
189189

190190

191191
def install_fake_process(
192-
monkeypatch: pytest.MonkeyPatch, process: FakeProcess, *, grace_period: float | None = 0.2
192+
monkeypatch: pytest.MonkeyPatch, *processes: FakeProcess, grace_period: float | None = 0.2
193193
) -> list[FakeProcess]:
194-
"""Route stdio_client's spawn and terminate seams to `process`.
194+
"""Route stdio_client's spawn and terminate seams to `processes`, one per spawn.
195195
196196
Returns the list of processes the (fake) tree termination was invoked on.
197197
`grace_period=None` keeps the production stdin-close grace (affordable only on a
198198
virtual clock).
199199
"""
200200
terminated: list[FakeProcess] = []
201+
to_spawn = iter(processes)
201202

202203
async def fake_spawn(
203204
command: str,
@@ -206,7 +207,7 @@ async def fake_spawn(
206207
errlog: TextIO = sys.stderr,
207208
cwd: Path | str | None = None,
208209
) -> FakeProcess:
209-
return process
210+
return next(to_spawn)
210211

211212
async def fake_terminate_tree(proc: FakeProcess) -> None:
212213
terminated.append(proc)
@@ -480,6 +481,59 @@ async def run_client_until_cancelled() -> None:
480481
assert terminated == [process]
481482

482483

484+
@pytest.mark.anyio
485+
@pytest.mark.parametrize("close_order", [(0, 1), (1, 0)], ids=["oldest-first", "newest-first"])
486+
async def test_two_transports_held_by_one_task_close_in_either_order(
487+
monkeypatch: pytest.MonkeyPatch, close_order: tuple[int, int]
488+
) -> None:
489+
"""One task holding two transports may close them oldest-first, not only newest-first.
490+
491+
Pins issue #577: a multi-server manager, independent exit stacks, or unordered pytest
492+
fixtures got a cancel-scope RuntimeError out of the oldest-first teardown. Asyncio
493+
only (this module's backend): on trio the transport still borrows the caller's
494+
nursery, so newest-first stays the requirement there.
495+
"""
496+
first = FakeProcess(on_stdin_close=lambda: first.exit(0))
497+
second = FakeProcess(on_stdin_close=lambda: second.exit(0))
498+
terminated = install_fake_process(monkeypatch, first, second)
499+
500+
stacks = [AsyncExitStack(), AsyncExitStack()]
501+
502+
with anyio.fail_after(5):
503+
for stack in stacks:
504+
await stack.enter_async_context(stdio_client(FAKE_PARAMS))
505+
for index in close_order:
506+
await stacks[index].aclose()
507+
508+
# Both servers went through the full shutdown, so neither needed terminating.
509+
assert first.stdin_closed.is_set()
510+
assert second.stdin_closed.is_set()
511+
assert terminated == []
512+
513+
514+
@pytest.mark.anyio
515+
async def test_an_unhandled_pipe_task_failure_surfaces_out_of_the_context_manager(
516+
monkeypatch: pytest.MonkeyPatch,
517+
) -> None:
518+
"""A pipe task failing in a way the transport does not handle reaches the caller.
519+
520+
Undecodable server output crashes the reader (the encoding error handler defaults to
521+
`strict`); exiting still shuts the server down cleanly, but the error is re-raised
522+
rather than swallowed.
523+
"""
524+
process = FakeProcess(on_stdin_close=lambda: process.exit(0))
525+
terminated = install_fake_process(monkeypatch, process)
526+
527+
with pytest.raises(UnicodeDecodeError):
528+
with anyio.fail_after(5):
529+
async with stdio_client(FAKE_PARAMS):
530+
await process.feed(b"\xff\xfe not utf-8\n")
531+
# Wait until the reader has actually decoded the bytes and died.
532+
await anyio.wait_all_tasks_blocked()
533+
534+
assert terminated == []
535+
536+
483537
@pytest.mark.anyio
484538
async def test_writing_after_the_server_dies_reports_clean_closure(monkeypatch: pytest.MonkeyPatch) -> None:
485539
"""A send racing the server's death must not surface a raw backend exception.

0 commit comments

Comments
 (0)