diff --git a/newsfragments/3307.misc.rst b/newsfragments/3307.misc.rst new file mode 100644 index 0000000000..ab59183729 --- /dev/null +++ b/newsfragments/3307.misc.rst @@ -0,0 +1 @@ +When misnesting nurseries you now get a helpful :exc:`RuntimeError` instead of a catastrophic :exc:`TrioInternalError`. diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index aee025cb7a..5303dfe75d 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -678,6 +678,9 @@ def _close(self, exc: BaseException | None) -> BaseException | None: exc is not None and self._cancel_status.effectively_cancelled and not self._cancel_status.parent_cancellation_is_visible_to_us + ) or ( + scope_task._cancel_status is not self._cancel_status + and self._cancel_status.abandoned_by_misnesting ): if isinstance(exc, Cancelled): self.cancelled_caught = True @@ -1261,6 +1264,9 @@ def _child_finished( outcome: Outcome[object], ) -> None: self._children.remove(task) + if self._closed and not hasattr(self, "_pending_excs"): + # We're abandoned by misnested nurseries, the result of the task is lost. + return if isinstance(outcome, Error): self._add_exc( outcome.error, @@ -1321,7 +1327,7 @@ def aborted(raise_cancel: _core.RaiseCancelT) -> Abort: self._add_exc(exc, reason=None) popped = self._parent_task._child_nurseries.pop() - assert popped is self + assert popped is self, "Nursery misnesting detected!" if self._pending_excs: try: if not self._strict_exception_groups and len(self._pending_excs) == 1: @@ -2007,6 +2013,17 @@ async def python_wrapper(orig_coro: Awaitable[RetT]) -> RetT: return task def task_exited(self, task: Task, outcome: Outcome[object]) -> None: + if task._child_nurseries: + for nursery in task._child_nurseries: + nursery.cancel_scope._cancel( + CancelReason( + source="nursery", + reason="Parent Task exited prematurely, abandoning this nursery without exiting it properly.", + source_task=repr(task), + ) + ) + nursery._closed = True + # break parking lots associated with the exiting task if task in GLOBAL_PARKING_LOT_BREAKER: for lot in GLOBAL_PARKING_LOT_BREAKER[task]: @@ -2017,7 +2034,8 @@ def task_exited(self, task: Task, outcome: Outcome[object]) -> None: task._cancel_status is not None and task._cancel_status.abandoned_by_misnesting and task._cancel_status.parent is None - ): + ) or task._child_nurseries: + reason = "Nursery" if task._child_nurseries else "Cancel scope" # The cancel scope surrounding this task's nursery was closed # before the task exited. Force the task to exit with an error, # since the error might not have been caught elsewhere. See the @@ -2026,7 +2044,7 @@ def task_exited(self, task: Task, outcome: Outcome[object]) -> None: # Raise this, rather than just constructing it, to get a # traceback frame included raise RuntimeError( - "Cancel scope stack corrupted: cancel scope surrounding " + f"{reason} stack corrupted: {reason} surrounding " f"{task!r} was closed before the task exited\n{MISNESTING_ADVICE}", ) except RuntimeError as new_exc: diff --git a/src/trio/_core/_tests/test_run.py b/src/trio/_core/_tests/test_run.py index b317c8d6c9..111ba9e5ec 100644 --- a/src/trio/_core/_tests/test_run.py +++ b/src/trio/_core/_tests/test_run.py @@ -8,7 +8,13 @@ import time import types import weakref -from contextlib import ExitStack, contextmanager, suppress +from contextlib import ( + AsyncExitStack, + ExitStack, + asynccontextmanager, + contextmanager, + suppress, +) from math import inf, nan from typing import TYPE_CHECKING, NoReturn, TypeVar from unittest import mock @@ -761,7 +767,7 @@ async def enter_scope() -> None: assert scope.cancel_called # never become un-cancelled -async def test_cancel_scope_misnesting() -> None: +async def test_cancel_scope_misnesting_1() -> None: outer = _core.CancelScope() inner = _core.CancelScope() with ExitStack() as stack: @@ -771,6 +777,8 @@ async def test_cancel_scope_misnesting() -> None: stack.close() # No further error is raised when exiting the inner context + +async def test_cancel_scope_misnesting_2() -> None: # If there are other tasks inside the abandoned part of the cancel tree, # they get cancelled when the misnesting is detected async def task1() -> None: @@ -828,6 +836,8 @@ def no_context(exc: RuntimeError) -> bool: ) assert group.matches(exc_info.value.__context__) + +async def test_cancel_scope_misnesting_3() -> None: # Trying to exit a cancel scope from an unrelated task raises an error # without affecting any state async def task3(task_status: _core.TaskStatus[_core.CancelScope]) -> None: @@ -844,6 +854,130 @@ async def task3(task_status: _core.TaskStatus[_core.CancelScope]) -> None: scope.cancel() +# helper to check we're not outputting overly verbose tracebacks +def no_cause_or_context(e: BaseException) -> bool: + return e.__cause__ is None and e.__context__ is None + + +async def test_nursery_misnest() -> None: + # See https://github.com/python-trio/trio/issues/3298 + async def inner_func() -> None: + inner_nursery = await inner_cm.__aenter__() + inner_nursery.start_soon(sleep, 1) + + with pytest.RaisesGroup( + pytest.RaisesExc( + RuntimeError, match="Nursery stack corrupted", check=no_cause_or_context + ), + check=no_cause_or_context, + ): + async with _core.open_nursery() as outer_nursery: + inner_cm = _core.open_nursery() + outer_nursery.start_soon(inner_func) + + +def test_nursery_nested_child_misnest() -> None: + # Note that this example does *not* raise an exception group. + async def main() -> None: + async with _core.open_nursery(): + inner_cm = _core.open_nursery() + await inner_cm.__aenter__() + + with pytest.raises(RuntimeError, match="Nursery stack corrupted") as excinfo: + _core.run(main) + assert excinfo.value.__cause__ is None + # This AssertionError is kind of redundant, but I don't think we want to remove + # the assertion and don't think we care enough to suppress it in this specific case. + assert pytest.RaisesExc( + AssertionError, match="^Nursery misnesting detected!$" + ).matches(excinfo.value.__context__) + assert excinfo.value.__context__.__cause__ is None + assert excinfo.value.__context__.__context__ is None + + +async def test_asyncexitstack_nursery_misnest() -> None: + # This example is trickier than the above ones, and is the one that requires + # special logic of abandoned nurseries to avoid nasty internal errors that masks + # the RuntimeError. + @asynccontextmanager + async def asynccontextmanager_that_creates_a_nursery_internally() -> ( + AsyncGenerator[None] + ): + async with _core.open_nursery() as nursery: + await nursery.start(started_sleeper) + nursery.start_soon(unstarted_task) + yield + + async def started_sleeper(task_status: _core.TaskStatus[None]) -> None: + task_status.started() + await sleep_forever() + + async def unstarted_task() -> None: + await _core.checkpoint() + + with pytest.RaisesGroup( + pytest.RaisesGroup( + pytest.RaisesExc( + RuntimeError, match="Nursery stack corrupted", check=no_cause_or_context + ), + check=no_cause_or_context, + ), + check=no_cause_or_context, + ): + async with AsyncExitStack() as stack, _core.open_nursery() as nursery: + # The asynccontextmanager is going to create a nursery that outlives this nursery! + nursery.start_soon( + stack.enter_async_context, + asynccontextmanager_that_creates_a_nursery_internally(), + ) + + +def test_asyncexitstack_nursery_misnest_cleanup() -> None: + # We guarantee that abandoned tasks get to do cleanup *eventually*, but exceptions + # are lost. With more effort it's possible we could reschedule child tasks to exit + # promptly. + finally_entered = [] + + async def main() -> None: + async def unstarted_task() -> None: + try: + await _core.checkpoint() + finally: + finally_entered.append(True) + raise ValueError("this exception is lost") + + # rest of main() is ~identical to the above test + @asynccontextmanager + async def asynccontextmanager_that_creates_a_nursery_internally() -> ( + AsyncGenerator[None] + ): + async with _core.open_nursery() as nursery: + nursery.start_soon(unstarted_task) + yield + + with pytest.RaisesGroup( + pytest.RaisesGroup( + pytest.RaisesExc( + RuntimeError, + match="Nursery stack corrupted", + check=no_cause_or_context, + ), + check=no_cause_or_context, + ), + check=no_cause_or_context, + ): + async with AsyncExitStack() as stack, _core.open_nursery() as nursery: + # The asynccontextmanager is going to create a nursery that outlives this nursery! + nursery.start_soon( + stack.enter_async_context, + asynccontextmanager_that_creates_a_nursery_internally(), + ) + assert not finally_entered # abandoned task still hasn't been cleaned up + + _core.run(main) + assert finally_entered # now it has + + @slow async def test_timekeeping() -> None: # probably a good idea to use a real clock for *one* test anyway... diff --git a/test-requirements.in b/test-requirements.in index 331e87ca34..6756f4d259 100644 --- a/test-requirements.in +++ b/test-requirements.in @@ -1,5 +1,5 @@ # For tests -pytest >= 5.0 # for faulthandler in core +pytest >= 8.4 # for pytest.RaisesGroup coverage >= 7.2.5 async_generator >= 1.9 pyright