diff --git a/newsfragments/3329.bugfix.rst b/newsfragments/3329.bugfix.rst new file mode 100644 index 0000000000..30e2ea5c2e --- /dev/null +++ b/newsfragments/3329.bugfix.rst @@ -0,0 +1,3 @@ +Trio now reports an error when a task exits without closing a cancel scope it +entered, instead of allowing that scope to be exited by another task. Deadlines +associated with these unclosed scopes are also cleaned up. diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index ddd4ea7ee2..8d625de2cd 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -2026,11 +2026,21 @@ def task_exited(self, task: Task, outcome: Outcome[object]) -> None: lot.break_lot(task) del GLOBAL_PARKING_LOT_BREAKER[task] + unclosed_scope = ( + task._parent_nursery is not None + and task._cancel_status is not task._parent_nursery._cancel_status + and task._cancel_status is not None + and not task._cancel_status.abandoned_by_misnesting + ) if ( - task._cancel_status is not None - and task._cancel_status.abandoned_by_misnesting - and task._cancel_status.parent is None - ) or task._child_nurseries: + ( + task._cancel_status is not None + and task._cancel_status.abandoned_by_misnesting + and task._cancel_status.parent is None + ) + or task._child_nurseries + or unclosed_scope + ): 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, @@ -2039,6 +2049,11 @@ def task_exited(self, task: Task, outcome: Outcome[object]) -> None: try: # Raise this, rather than just constructing it, to get a # traceback frame included + if unclosed_scope and not task._child_nurseries: + raise RuntimeError( + f"Cancel scope stack corrupted: {task!r} exited without " + f"closing its cancel scope\n{MISNESTING_ADVICE}", + ) raise RuntimeError( f"{reason} stack corrupted: {reason} surrounding " f"{task!r} was closed before the task exited\n{MISNESTING_ADVICE}", @@ -2048,7 +2063,18 @@ def task_exited(self, task: Task, outcome: Outcome[object]) -> None: new_exc.__context__ = outcome.error outcome = Error(new_exc) + exited_status: CancelStatus | None = task._cancel_status task._activate_cancel_status(None) + if unclosed_scope and not task._child_nurseries: + assert task._parent_nursery is not None + while exited_status is not task._parent_nursery._cancel_status: + assert exited_status is not None + parent_status = exited_status.parent + scope = exited_status._scope + exited_status.close() + with scope._might_change_registered_deadline(): + scope._cancel_status = None + exited_status = parent_status self.tasks.remove(task) if task is self.init_task: # If the init task crashed, then something is very wrong and we diff --git a/src/trio/_core/_tests/test_guest_mode.py b/src/trio/_core/_tests/test_guest_mode.py index 743eddc846..68e1bd8d5e 100644 --- a/src/trio/_core/_tests/test_guest_mode.py +++ b/src/trio/_core/_tests/test_guest_mode.py @@ -39,6 +39,21 @@ InHost: TypeAlias = Callable[[Callable[[], object]], None] +def test_guest_unclosed_cancel_scope_deadline_cleanup() -> None: + async def main(in_host: InHost) -> None: + scope = trio.CancelScope(deadline=trio.current_time() + 100) + + async def child() -> None: + scope.__enter__() + + with pytest.RaisesGroup(RuntimeError): + async with trio.open_nursery() as nursery: + nursery.start_soon(child) + assert scope._registered_deadline == inf + + trivial_guest_run(main) + + # The simplest possible "host" loop. # Nice features: # - we can run code "outside" of trio using the schedule function passed to diff --git a/src/trio/_core/_tests/test_run.py b/src/trio/_core/_tests/test_run.py index e49e2e6ed2..27b9a3a673 100644 --- a/src/trio/_core/_tests/test_run.py +++ b/src/trio/_core/_tests/test_run.py @@ -852,6 +852,66 @@ async def task3(task_status: _core.TaskStatus[_core.CancelScope]) -> None: scope.cancel() +@pytest.mark.parametrize("depth", [1, 2]) +@pytest.mark.parametrize("cancelled", [False, True]) +@pytest.mark.parametrize("adopted", [False, True]) +async def test_cancel_scope_left_open_at_task_exit( + depth: int, cancelled: bool, adopted: bool +) -> None: + # A scope must not outlive the task that entered it (issue #3329). + scopes = [ + _core.CancelScope(deadline=_core.current_time() + 100) for _ in range(depth) + ] + + async def enter_scope( + *, task_status: _core.TaskStatus[None] = _core.TASK_STATUS_IGNORED + ) -> None: + for scope in scopes: + scope.__enter__() + task_status.started() + if cancelled: + scopes[-1].cancel() + await _core.checkpoint() + + with pytest.RaisesGroup(RuntimeError): + async with _core.open_nursery() as nursery: + if adopted: + await nursery.start(enter_scope) + else: + nursery.start_soon(enter_scope) + + assert all(scope._registered_deadline == inf for scope in scopes) + assert all(scope._cancel_status is None for scope in scopes) + + +async def test_unclosed_cancel_scope_preserves_error_and_parent() -> None: + original = ValueError("child failure") + child_scope = _core.CancelScope(deadline=_core.current_time() + 100) + + async def child() -> None: + child_scope.__enter__() + raise original + + with _core.CancelScope(deadline=_core.current_time() + 200) as parent_scope: + parent_status = parent_scope._cancel_status + parent_deadline = parent_scope._registered_deadline + with pytest.RaisesGroup( + pytest.RaisesExc( + RuntimeError, + match="exited without closing its cancel scope", + check=lambda exc: exc.__context__ is original, + ) + ): + async with _core.open_nursery() as nursery: + nursery.start_soon(child) + assert parent_scope._cancel_status is parent_status + assert parent_scope._registered_deadline == parent_deadline + assert not parent_scope.cancel_called + await _core.checkpoint() + assert child_scope._cancel_status is None + assert child_scope._registered_deadline == inf + + # 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