From d00e115fbbf2b7da12152601c77a75e83998c2f1 Mon Sep 17 00:00:00 2001 From: Ganden Schaffner Date: Mon, 24 Apr 2023 00:00:00 -0700 Subject: [PATCH 1/4] Test that TaskStatus.started() never returns success upon failure --- src/trio/_core/_tests/test_run.py | 60 +++++++++++++++---------------- 1 file changed, 29 insertions(+), 31 deletions(-) diff --git a/src/trio/_core/_tests/test_run.py b/src/trio/_core/_tests/test_run.py index 5bd98f0e91..a6f53eb18f 100644 --- a/src/trio/_core/_tests/test_run.py +++ b/src/trio/_core/_tests/test_run.py @@ -1804,48 +1804,46 @@ async def nothing( await nursery.start(nothing) assert "exited without calling" in str(excinfo1.value) - # if the call to start() is cancelled, then the call to started() does - # nothing -- the child keeps executing under start(). The value it passed - # is ignored; start() raises Cancelled. - async def just_started( - task_status: _core.TaskStatus[str] = _core.TASK_STATUS_IGNORED, + # if the call to start() is effectively cancelled when the child checkpoints + # (before calling started()), the child raises Cancelled up out of start(). + async def checkpoint_before_started( + task_status: _core.TaskStatus[None] = _core.TASK_STATUS_IGNORED, ) -> None: - task_status.started("hi") await _core.checkpoint() + raise AssertionError() # pragma: no cover async with _core.open_nursery() as nursery: with _core.CancelScope() as cs: cs.cancel() with pytest.raises(_core.Cancelled): - await nursery.start(just_started) - - # but if the task does not execute any checkpoints, and exits, then start() - # doesn't raise Cancelled, since the task completed successfully. - async def started_with_no_checkpoint( - *, task_status: _core.TaskStatus[None] = _core.TASK_STATUS_IGNORED - ) -> None: - task_status.started(None) + await nursery.start(checkpoint_before_started) - async with _core.open_nursery() as nursery: - with _core.CancelScope() as cs: - cs.cancel() - await nursery.start(started_with_no_checkpoint) - assert not cs.cancelled_caught - - # and since starting in a cancelled context makes started() a no-op, if - # the child crashes after calling started(), the error can *still* come - # out of start() - async def raise_keyerror_after_started( - *, task_status: _core.TaskStatus[None] = _core.TASK_STATUS_IGNORED + # but if the call to start() is effectively cancelled when the child calls + # started(), it is too late to deliver the start() cancellation to the + # child, so the start() call returns and the child continues in the nursery. + async def no_checkpoint_before_started( + eventual_parent_nursery: _core.Nursery, + *, + task_status: _core.TaskStatus[None] = _core.TASK_STATUS_IGNORED, ) -> None: + assert _core.current_task().eventual_parent_nursery is eventual_parent_nursery task_status.started() - raise KeyError("whoopsiedaisy") + assert _core.current_task().eventual_parent_nursery is None + assert _core.current_task().parent_nursery is eventual_parent_nursery + # This task is no longer in an effectively cancelled scope, so it should + # be able to pass through a checkpoint. + await _core.checkpoint() + raise KeyError(f"I should come out of {eventual_parent_nursery!r}") - async with _core.open_nursery() as nursery: - with _core.CancelScope() as cs: - cs.cancel() - with pytest.raises(KeyError): - await nursery.start(raise_keyerror_after_started) + with pytest.raises(KeyError): + async with _core.open_nursery() as nursery: + with _core.CancelScope() as cs: + cs.cancel() + try: + await nursery.start(no_checkpoint_before_started, nursery) + except KeyError as exc: + raise AssertionError() from exc # pragma: no cover + assert not cs.cancelled_caught # trying to start in a closed nursery raises an error immediately async with _core.open_nursery() as closed_nursery: From 39bc92a79b11d785ec60314324d3bebb215a7cd1 Mon Sep 17 00:00:00 2001 From: Ganden Schaffner Date: Mon, 24 Apr 2023 00:00:00 -0700 Subject: [PATCH 2/4] Test that TaskStatus.started() does not allow teleporting Cancelled(s) around the tree --- src/trio/_core/_tests/test_run.py | 55 ++++++++++++++++++++++++++++++- test-requirements.in | 2 +- test-requirements.txt | 2 +- 3 files changed, 56 insertions(+), 3 deletions(-) diff --git a/src/trio/_core/_tests/test_run.py b/src/trio/_core/_tests/test_run.py index a6f53eb18f..1f9ee0e6f5 100644 --- a/src/trio/_core/_tests/test_run.py +++ b/src/trio/_core/_tests/test_run.py @@ -40,6 +40,7 @@ Callable, Generator, ) +from exceptiongroup import catch if sys.version_info < (3, 11): from exceptiongroup import BaseExceptionGroup, ExceptionGroup @@ -1845,7 +1846,59 @@ async def no_checkpoint_before_started( raise AssertionError() from exc # pragma: no cover assert not cs.cancelled_caught - # trying to start in a closed nursery raises an error immediately + # calling started() while handling a Cancelled raises an error immediately. + async def started_while_handling_cancelled( + task_status: _core.TaskStatus[None] = _core.TASK_STATUS_IGNORED, + ) -> None: + try: + await _core.checkpoint() + except _core.Cancelled: + task_status.started() + raise AssertionError() # pragma: no cover + + async with _core.open_nursery() as nursery: + with _core.CancelScope() as cs: + cs.cancel() + with pytest.raises(RuntimeError): + await nursery.start(started_while_handling_cancelled) + + # calling started() while handling multiple Cancelleds raises an error + # immediately. + async def started_while_handling_multiple_cancelled( + task_status: _core.TaskStatus[None] = _core.TASK_STATUS_IGNORED, + ) -> None: + with catch({_core.Cancelled: lambda _: task_status.started()}): + async with _core.open_nursery() as nursery: + nursery.start_soon(_core.checkpoint) + nursery.start_soon(_core.checkpoint) + raise AssertionError() # pragma: no cover + + async with _core.open_nursery() as nursery: + with _core.CancelScope() as cs: + cs.cancel() + with pytest.raises(RuntimeError): + await nursery.start(started_while_handling_multiple_cancelled) + + # calling started() while handling an exception while handling Cancelled(s) raises an error immediately. + async def started_while_handling_exc_while_handling_cancelled( + task_status: _core.TaskStatus[None] = _core.TASK_STATUS_IGNORED, + ) -> None: + try: + await _core.checkpoint() + except _core.Cancelled: + try: + raise ValueError + except ValueError: + task_status.started() + raise AssertionError() # pragma: no cover + + async with _core.open_nursery() as nursery: + with _core.CancelScope() as cs: + cs.cancel() + with pytest.raises(RuntimeError): + await nursery.start(started_while_handling_exc_while_handling_cancelled) + + # trying to start in a closed nursery raises an error immediately. async with _core.open_nursery() as closed_nursery: pass t0 = _core.current_time() diff --git a/test-requirements.in b/test-requirements.in index 683eeb21db..e9bfb47e7d 100644 --- a/test-requirements.in +++ b/test-requirements.in @@ -30,4 +30,4 @@ sortedcontainers idna outcome sniffio -exceptiongroup >= 1.0.0rc9; python_version < "3.11" +exceptiongroup >= 1.0.0rc9 diff --git a/test-requirements.txt b/test-requirements.txt index 0dc9d07a15..630e7afcc6 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -36,7 +36,7 @@ cryptography==41.0.5 # types-pyopenssl dill==0.3.7 # via pylint -exceptiongroup==1.1.3 ; python_version < "3.11" +exceptiongroup==1.1.3 # via # -r test-requirements.in # pytest From b038de53950cc023fbee580bde2e2ba50cf2ad97 Mon Sep 17 00:00:00 2001 From: Ganden Schaffner Date: Mon, 24 Apr 2023 00:00:00 -0700 Subject: [PATCH 3/4] Make TaskStatus.started() never return success upon failure Specifically, make started() reparent the task even if the start() call is in an effectively cancelled scope when started() is called, and check sys.exc_info() to prevent calling task_status.started() while handling active Cancelled(s). --- newsfragments/2895.bugfix.rst | 13 ++++++++++++ src/trio/_core/_run.py | 37 +++++++++++++++++++++++++++-------- 2 files changed, 42 insertions(+), 8 deletions(-) create mode 100644 newsfragments/2895.bugfix.rst diff --git a/newsfragments/2895.bugfix.rst b/newsfragments/2895.bugfix.rst new file mode 100644 index 0000000000..8181308f84 --- /dev/null +++ b/newsfragments/2895.bugfix.rst @@ -0,0 +1,13 @@ +When :func:`trio.TaskStatus.started` is called in a cancelled scope, it no +longer returns (indicating that the task has been moved and is now running as a +child of the nursery) without actually moving the task. Previously, +:func:`trio.TaskStatus.started` would not move the task in this scenario, +causing the call to :func:`trio.Nursery.start` to incorrectly wait until the +started task finished, which caused deadlocks and other issues. + +:func:`trio.TaskStatus.started` is no longer allowed to be called by an +exception handler that is handling one or more :exc:`Cancelled` exceptions; +attempting to do so will now raise a :exc:`RuntimeError`. Note that any code +that did this prior to this ban was already buggy, because it was attempting to +teleport :exc:`Cancelled` exception(s) to a place in the task tree without the +corresponding :class:`CancelScope`(s) to catch them. diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index 6e8a2e261e..ddd040f879 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -856,20 +856,35 @@ def started(self: _TaskStatus[StatusT], value: StatusT) -> None: def started(self, value: StatusT | None = None) -> None: if self._value is not _NoStatus: raise RuntimeError("called 'started' twice on the same task status") - self._value = cast(StatusT, value) # If None, StatusT == None - # If the old nursery is cancelled, then quietly quit now; the child - # will eventually exit on its own, and we don't want to risk moving - # children that might have propagating Cancelled exceptions into - # a place with no cancelled cancel scopes to catch them. - assert self._old_nursery._cancel_status is not None - if self._old_nursery._cancel_status.effectively_cancelled: - return + # Make sure we don't move a task with propagating Cancelled exception(s) + # to a place in the tree without the corresponding cancel scope(s). + # + # N.B.: This check is limited to the task that calls started(). If the + # user uses lowlevel.current_task().parent_nursery to add other tasks to + # the private implementation-detail nursery of start(), this won't be + # able to check those tasks. See #1599. + _, exc, _ = sys.exc_info() + while exc is not None: + handling_cancelled = False + if isinstance(exc, Cancelled): + handling_cancelled = True + elif isinstance(exc, BaseExceptionGroup): + matched, _ = exc.split(Cancelled) + if matched: + handling_cancelled = True + if handling_cancelled: + raise RuntimeError( + "task_status.started() cannot be called while handling Cancelled(s)" + ) + exc = exc.__context__ # Can't be closed, b/c we checked in start() and then _pending_starts # should keep it open. assert not self._new_nursery._closed + self._value = cast(StatusT, value) # If None, StatusT == None + # Move tasks from the old nursery to the new tasks = self._old_nursery._children self._old_nursery._children = set() @@ -1209,6 +1224,12 @@ async def async_fn(arg1, arg2, *, task_status=trio.TASK_STATUS_IGNORED): If the child task passes a value to :meth:`task_status.started(value) `, then :meth:`start` returns this value. Otherwise, it returns ``None``. + + :meth:`task_status.started() ` cannot be called by + an exception handler (or other cleanup code, like ``finally`` blocks, + ``__aexit__`` handlers, and so on) that is handling one or more + :exc:`Cancelled` exceptions. (It'll raise a :exc:`RuntimeError` if you + violate this rule.) """ if self._closed: raise RuntimeError("Nursery is closed to new arrivals") From 8933cee72ac4df8fcd052f86452b684026cd885a Mon Sep 17 00:00:00 2001 From: CoolCat467 <52022020+CoolCat467@users.noreply.github.com> Date: Wed, 13 Dec 2023 15:20:07 -0600 Subject: [PATCH 4/4] Fix ruff issue --- src/trio/_core/_tests/test_run.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/trio/_core/_tests/test_run.py b/src/trio/_core/_tests/test_run.py index 63270dc336..8c2e56c526 100644 --- a/src/trio/_core/_tests/test_run.py +++ b/src/trio/_core/_tests/test_run.py @@ -1850,7 +1850,7 @@ async def no_checkpoint_before_started( await _core.checkpoint() raise KeyError(f"I should come out of {eventual_parent_nursery!r}") - with pytest.raises(KeyError): + with pytest.raises(KeyError): # noqa: PT012 async with _core.open_nursery() as nursery: with _core.CancelScope() as cs: cs.cancel()