From 0b627ba9f4d63548926a230b310a38ce7f3fe2ee Mon Sep 17 00:00:00 2001 From: jakkdl Date: Tue, 22 Apr 2025 15:32:16 +0200 Subject: [PATCH 01/15] draft for adding CancelReason --- src/trio/_core/_exceptions.py | 29 +++++++- src/trio/_core/_run.py | 79 ++++++++++++++++++++- src/trio/_core/_tests/test_cancel.py | 101 +++++++++++++++++++++++++++ src/trio/_core/_tests/test_run.py | 48 +++++++++---- src/trio/_tests/test_threads.py | 12 +++- src/trio/_tests/test_util.py | 8 ++- 6 files changed, 254 insertions(+), 23 deletions(-) create mode 100644 src/trio/_core/_tests/test_cancel.py diff --git a/src/trio/_core/_exceptions.py b/src/trio/_core/_exceptions.py index 4a76a674ac..0c85576702 100644 --- a/src/trio/_core/_exceptions.py +++ b/src/trio/_core/_exceptions.py @@ -1,6 +1,9 @@ from __future__ import annotations -from typing import TYPE_CHECKING +from functools import partial +from typing import TYPE_CHECKING, Literal + +import attrs from trio._util import NoPublicConstructor, final @@ -34,6 +37,7 @@ class WouldBlock(Exception): @final +@attrs.define(eq=False, kw_only=True) class Cancelled(BaseException, metaclass=NoPublicConstructor): """Raised by blocking calls if the surrounding scope has been cancelled. @@ -67,11 +71,30 @@ class Cancelled(BaseException, metaclass=NoPublicConstructor): """ + source: Literal["deadline", "nursery", "explicit"] + source_task: str | None = None + reason: str | None = None + def __str__(self) -> str: - return "Cancelled" + return ( + f"Cancelled due to {self.source}" + + (f" with reason {self.reason!r}" if self.reason is not None else "") + + f" from task {self.source_task}" + ) def __reduce__(self) -> tuple[Callable[[], Cancelled], tuple[()]]: - return (Cancelled._create, ()) + # the `__reduce__` tuple does not support kwargs, so we must use partial + # for non-default args + # or switch to allow posarg (?) + return ( + partial( + Cancelled._create, + source=self.source, + source_task=self.source_task, + reason=self.reason, + ), + (), + ) class BusyResourceError(Exception): diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index c2410150e1..771041007d 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -18,6 +18,7 @@ TYPE_CHECKING, Any, Final, + Literal, NoReturn, Protocol, cast, @@ -314,6 +315,14 @@ def expire(self, now: float) -> bool: return did_something +@attrs.define +class CancelReason: + # TODO: loren ipsum + source: Literal["deadline", "nursery", "explicit", "KeyboardInterrupt"] + source_task: str | None = None + reason: str | None = None + + @attrs.define(eq=False) class CancelStatus: """Tracks the cancellation status for a contiguous extent @@ -381,6 +390,10 @@ class CancelStatus: # recovery to show a useful traceback). abandoned_by_misnesting: bool = attrs.field(default=False, init=False, repr=False) + _cancel_reason: CancelReason | None = attrs.field( + default=None, init=False, repr=True, alias="cancel_reason" + ) + def __attrs_post_init__(self) -> None: if self._parent is not None: self._parent._children.add(self) @@ -468,10 +481,25 @@ def recalculate(self) -> None: or current.parent_cancellation_is_visible_to_us ) if new_state != current.effectively_cancelled: + if ( + current._scope._cancel_reason is not None + and current._cancel_reason is None + ): + current._cancel_reason = current._scope._cancel_reason + if ( + current._cancel_reason is None + and current.parent_cancellation_is_visible_to_us + ): + assert current._parent is not None + current._cancel_reason = current._parent._cancel_reason current.effectively_cancelled = new_state if new_state: for task in current._tasks: task._attempt_delivery_of_any_pending_cancel() + # current._cancel_reason will not be None, unless there's misnesting + for child in current._children: + if child._cancel_reason is None: + child._cancel_reason = current._cancel_reason todo.extend(current._children) def _mark_abandoned(self) -> None: @@ -558,6 +586,12 @@ class CancelScope: _cancel_called: bool = attrs.field(default=False, init=False) cancelled_caught: bool = attrs.field(default=False, init=False) + # necessary as cancel_status might be None + # TODO: but maybe cancel_status doesn't need it? + _cancel_reason: CancelReason | None = attrs.field( + default=None, init=False, repr=True + ) + # Constructor arguments: _relative_deadline: float = attrs.field( default=inf, @@ -594,9 +628,12 @@ def __enter__(self) -> Self: self._relative_deadline = inf if current_time() >= self._deadline: + self._cancel_reason = CancelReason(source="deadline") self.cancel() with self._might_change_registered_deadline(): self._cancel_status = CancelStatus(scope=self, parent=task._cancel_status) + if self._cancel_reason is not None: + self._cancel_status._cancel_reason = self._cancel_reason task._activate_cancel_status(self._cancel_status) return self @@ -883,7 +920,7 @@ def shield(self, new_value: bool) -> None: self._cancel_status.recalculate() @enable_ki_protection - def cancel(self) -> None: + def cancel(self, *, reason: str | None = None) -> None: """Cancels this scope immediately. This method is idempotent, i.e., if the scope was already @@ -893,7 +930,17 @@ def cancel(self) -> None: return with self._might_change_registered_deadline(): self._cancel_called = True + if self._cancel_reason is None: + try: + current_task = repr(_core.current_task()) + except RuntimeError: + current_task = None + self._cancel_reason = CancelReason( + reason=reason, source="explicit", source_task=current_task + ) if self._cancel_status is not None: + # idk if strictly required + self._cancel_status._cancel_reason = self._cancel_reason self._cancel_status.recalculate() @property @@ -1210,6 +1257,10 @@ def _child_finished( ) -> None: self._children.remove(task) if isinstance(outcome, Error): + if self.cancel_scope._cancel_reason is None: + self.cancel_scope._cancel_reason = CancelReason( + source="nursery", source_task=repr(task) + ) self._add_exc(outcome.error) self._check_nursery_closed() @@ -1576,7 +1627,18 @@ def _attempt_delivery_of_any_pending_cancel(self) -> None: return def raise_cancel() -> NoReturn: - raise Cancelled._create() + if ( + self._cancel_status is None + or self._cancel_status._cancel_reason is None + ): + # self._cancel_status is None when using abandon_on_cancel=True (TODO: why?) + # _cancel_status._cancel_reason is None when misnesting + raise Cancelled._create(source="unknown") + raise Cancelled._create( + source=self._cancel_status._cancel_reason.source, + reason=self._cancel_status._cancel_reason.reason, + source_task=self._cancel_status._cancel_reason.source_task, + ) self._attempt_abort(raise_cancel) @@ -2926,7 +2988,18 @@ async def checkpoint() -> None: if task._cancel_status.effectively_cancelled or ( task is task._runner.main_task and task._runner.ki_pending ): - with CancelScope(deadline=-inf): + cs = CancelScope(deadline=inf) + if ( + task._cancel_status._cancel_reason is None + and task is task._runner.main_task + and task._runner.ki_pending + ): + task._cancel_status._cancel_reason = CancelReason( + source="KeyboardInterrupt" + ) + assert task._cancel_status._cancel_reason is not None + cs._cancel_reason = task._cancel_status._cancel_reason + with cs: await _core.wait_task_rescheduled(lambda _: _core.Abort.SUCCEEDED) diff --git a/src/trio/_core/_tests/test_cancel.py b/src/trio/_core/_tests/test_cancel.py new file mode 100644 index 0000000000..eb87ce1d16 --- /dev/null +++ b/src/trio/_core/_tests/test_cancel.py @@ -0,0 +1,101 @@ +# there's tons of cancellation testing in test_run, but that file is 3k lines long already... + +from math import inf + +import pytest + +import trio +from trio import Cancelled +from trio.lowlevel import current_task +from trio.testing import RaisesGroup + + +async def test_cancel_reason() -> None: + with trio.CancelScope() as cs: + cs.cancel(reason="hello") + with pytest.raises( + Cancelled, + match=rf"^Cancelled due to explicit with reason 'hello' from task {current_task()!r}$", + ): + await trio.lowlevel.checkpoint() + + with trio.CancelScope(deadline=-inf) as cs: + with pytest.raises(Cancelled, match=r"^Cancelled due to deadline"): + await trio.lowlevel.checkpoint() + + with trio.CancelScope() as cs: + cs.deadline = -inf + with pytest.raises( + Cancelled, + # FIXME: ?? + match=r"^Cancelled due to explicit from task None", + ): + await trio.lowlevel.checkpoint() + + +async def test_cancel_reason_nursery() -> None: + async def failing_task(task_status: trio.TaskStatus[trio.lowlevel.Task]) -> None: + task_status.started(current_task()) + raise ValueError + + async def cancelled_task( + fail_task: trio.lowlevel.Task, task_status: trio.TaskStatus + ) -> None: + task_status.started() + with pytest.raises( + Cancelled, match=rf"^Cancelled due to nursery from task {fail_task!r}$" + ): + await trio.sleep_forever() + raise TypeError + + with RaisesGroup(ValueError, TypeError): + async with trio.open_nursery() as nursery: + fail_task = await nursery.start(failing_task) + await nursery.start(cancelled_task, fail_task) + + +async def test_cancel_reason_nursery2() -> None: + async def failing_task( + event: trio.Event, task_status: trio.TaskStatus[trio.lowlevel.Task] + ) -> None: + task_status.started(current_task()) + await event.wait() + raise ValueError + + async def cancelled_task( + fail_task: trio.lowlevel.Task, task_status: trio.TaskStatus + ) -> None: + task_status.started() + with pytest.raises( + Cancelled, match=rf"^Cancelled due to nursery from task {fail_task!r}$" + ): + await trio.sleep_forever() + raise TypeError + + with RaisesGroup(ValueError, TypeError): + async with trio.open_nursery() as nursery: + event = trio.Event() + fail_task = await nursery.start(failing_task, event) + await nursery.start(cancelled_task, fail_task) + event.set() + + +async def test_cancel_reason_not_overwritten() -> None: + with trio.CancelScope() as cs: + cs.cancel() + with pytest.raises(Cancelled, match=r"^Cancelled due to explicit"): + await trio.lowlevel.checkpoint() + cs.deadline = -inf + with pytest.raises(Cancelled, match=r"^Cancelled due to explicit"): + await trio.lowlevel.checkpoint() + + +async def test_cancel_reason_not_overwritten_2() -> None: + # TODO: broken, see earlier test + with trio.CancelScope() as cs: + cs.deadline = -inf + with pytest.raises(Cancelled, match=r"^Cancelled due to explicit"): + await trio.lowlevel.checkpoint() + cs.cancel() + with pytest.raises(Cancelled, match=r"^Cancelled due to explicit"): + await trio.lowlevel.checkpoint() diff --git a/src/trio/_core/_tests/test_run.py b/src/trio/_core/_tests/test_run.py index c02d185d45..075a521444 100644 --- a/src/trio/_core/_tests/test_run.py +++ b/src/trio/_core/_tests/test_run.py @@ -4,6 +4,7 @@ import functools import gc import pickle +import re import sys import threading import time @@ -781,7 +782,9 @@ async def task1() -> None: # Even if inside another cancel scope async def task2() -> None: with _core.CancelScope(): - with pytest.raises(_core.Cancelled): + with pytest.raises( + _core.Cancelled, match=r"^Cancelled due to unknown from task None$" + ): await sleep_forever() with ExitStack() as stack: @@ -2216,19 +2219,40 @@ def test_Nursery_subclass() -> None: def test_Cancelled_init() -> None: - with pytest.raises(TypeError): - raise _core.Cancelled + with pytest.raises(TypeError, match=r"^trio.Cancelled has no public constructor$"): + raise _core.Cancelled # type: ignore[call-arg] - with pytest.raises(TypeError): - _core.Cancelled() + with pytest.raises(TypeError, match=r"^trio.Cancelled has no public constructor$"): + _core.Cancelled(source="explicit") # private constructor should not raise - _core.Cancelled._create() - - -def test_Cancelled_str() -> None: - cancelled = _core.Cancelled._create() - assert str(cancelled) == "Cancelled" + _core.Cancelled._create(source="explicit") + + +async def test_Cancelled_str() -> None: + cancelled = _core.Cancelled._create(source="foo") + assert str(cancelled) == "Cancelled due to foo from task None" + assert re.fullmatch( + r"Cancelled due to bar from task " + r"", + str( + _core.Cancelled._create( + source="bar", + source_task=_core.current_task(), + ) + ), + ) + assert re.fullmatch( + r"Cancelled due to bar with reason 'pigs flying' from task " + r"", + str( + _core.Cancelled._create( + source="bar", + source_task=_core.current_task(), + reason="pigs flying", + ) + ), + ) def test_Cancelled_subclass() -> None: @@ -2238,7 +2262,7 @@ def test_Cancelled_subclass() -> None: # https://github.com/python-trio/trio/issues/3248 def test_Cancelled_pickle() -> None: - cancelled = _core.Cancelled._create() + cancelled = _core.Cancelled._create(source="foo") cancelled = pickle.loads(pickle.dumps(cancelled)) assert isinstance(cancelled, _core.Cancelled) diff --git a/src/trio/_tests/test_threads.py b/src/trio/_tests/test_threads.py index 380da3833b..b57f5702e1 100644 --- a/src/trio/_tests/test_threads.py +++ b/src/trio/_tests/test_threads.py @@ -994,7 +994,7 @@ async def async_time_bomb() -> None: async def test_from_thread_check_cancelled() -> None: - q: stdlib_queue.Queue[str] = stdlib_queue.Queue() + q: stdlib_queue.Queue[str | BaseException] = stdlib_queue.Queue() async def child(abandon_on_cancel: bool, scope: CancelScope) -> None: with scope: @@ -1053,6 +1053,10 @@ def f() -> None: # type: ignore[no-redef] # noqa: F811 from_thread_check_cancelled() except _core.Cancelled: q.put("Cancelled") + except BaseException as e: # pragma: no cover, test failure path + # abandon_on_cancel=True will eat exceptions, so we pass it + # through the queue in order to be able to debug any exceptions + q.put(e) else: # pragma: no cover, test failure path q.put("Not Cancelled") @@ -1068,7 +1072,11 @@ def f() -> None: # type: ignore[no-redef] # noqa: F811 assert scope.cancelled_caught assert "cancel" in record assert record[-1] == "exit" - assert q.get(timeout=1) == "Cancelled" + res = q.get(timeout=1) + if isinstance(res, BaseException): # pragma: no cover # for debugging + raise res + else: + assert res == "Cancelled" def test_from_thread_check_cancelled_raises_in_foreign_threads() -> None: diff --git a/src/trio/_tests/test_util.py b/src/trio/_tests/test_util.py index c0b0a3108e..e907da9c10 100644 --- a/src/trio/_tests/test_util.py +++ b/src/trio/_tests/test_util.py @@ -282,7 +282,7 @@ async def test_raise_single_exception_from_group() -> None: context = TypeError("context") exc.__cause__ = cause exc.__context__ = context - cancelled = trio.Cancelled._create() + cancelled = trio.Cancelled._create(source="foo") with pytest.raises(ValueError, match="foo") as excinfo: raise_single_exception_from_group(ExceptionGroup("", [exc])) @@ -346,9 +346,11 @@ async def test_raise_single_exception_from_group() -> None: assert excinfo.value.__context__ is None # if we only got cancelled, first one is reraised - with pytest.raises(trio.Cancelled, match=r"^Cancelled$") as excinfo: + with pytest.raises( + trio.Cancelled, match=r"^Cancelled due to foo from task None$" + ) as excinfo: raise_single_exception_from_group( - BaseExceptionGroup("", [cancelled, trio.Cancelled._create()]) + BaseExceptionGroup("", [cancelled, trio.Cancelled._create(source="bar")]) ) assert excinfo.value is cancelled assert excinfo.value.__cause__ is None From c7c46a43922a305f5e39a6786e8a5248b202c91f Mon Sep 17 00:00:00 2001 From: jakkdl Date: Fri, 25 Apr 2025 18:02:50 +0200 Subject: [PATCH 02/15] fix some issues, but currently causing cyclic garbage --- src/trio/_channel.py | 2 +- src/trio/_core/_asyncgens.py | 4 +- src/trio/_core/_exceptions.py | 29 ++++++++++-- src/trio/_core/_run.py | 64 +++++++++++++------------- src/trio/_core/_tests/test_cancel.py | 26 ++++++----- src/trio/_core/_tests/test_run.py | 29 +++++++----- src/trio/_highlevel_generic.py | 2 +- src/trio/_highlevel_open_tcp_stream.py | 2 +- src/trio/_subprocess.py | 1 + src/trio/_tests/test_threads.py | 33 +++++++++---- src/trio/_tests/test_util.py | 10 ++-- 11 files changed, 123 insertions(+), 79 deletions(-) diff --git a/src/trio/_channel.py b/src/trio/_channel.py index 1ed5945798..2768ecd44a 100644 --- a/src/trio/_channel.py +++ b/src/trio/_channel.py @@ -547,7 +547,7 @@ async def context_manager( yield wrapped_recv_chan # User has exited context manager, cancel to immediately close the # abandoned generator if it's still alive. - nursery.cancel_scope.cancel() + nursery.cancel_scope.cancel(reason="user exited context manager") except BaseExceptionGroup as eg: try: raise_single_exception_from_group(eg) diff --git a/src/trio/_core/_asyncgens.py b/src/trio/_core/_asyncgens.py index b3b6895753..cd01df548d 100644 --- a/src/trio/_core/_asyncgens.py +++ b/src/trio/_core/_asyncgens.py @@ -230,7 +230,9 @@ async def _finalize_one( # with an exception, not even a Cancelled. The inside # is cancelled so there's no deadlock risk. with _core.CancelScope(shield=True) as cancel_scope: - cancel_scope.cancel() + cancel_scope.cancel( + reason="internal trio cancellation during finalization" + ) await agen.aclose() except BaseException: ASYNCGEN_LOGGER.exception( diff --git a/src/trio/_core/_exceptions.py b/src/trio/_core/_exceptions.py index 0c85576702..db3a01b3e2 100644 --- a/src/trio/_core/_exceptions.py +++ b/src/trio/_core/_exceptions.py @@ -10,6 +10,8 @@ if TYPE_CHECKING: from collections.abc import Callable + from typing_extensions import Self + class TrioInternalError(Exception): """Raised by :func:`run` if we encounter a bug in Trio, or (possibly) a @@ -71,15 +73,21 @@ class Cancelled(BaseException, metaclass=NoPublicConstructor): """ - source: Literal["deadline", "nursery", "explicit"] + source: Literal["deadline", "nursery", "explicit", "unknown", "KeyboardInterrupt"] + # TODO: this should probably be a Task? source_task: str | None = None reason: str | None = None def __str__(self) -> str: + def repr_if_not_none(lead: str, s: str | None, trail: str = "") -> str: + if s is None: + return "" + return lead + s + trail + return ( - f"Cancelled due to {self.source}" - + (f" with reason {self.reason!r}" if self.reason is not None else "") - + f" from task {self.source_task}" + f"cancelled due to {self.source}" + + repr_if_not_none(" with reason '", self.reason, "'") + + repr_if_not_none(" from task ", self.source_task) ) def __reduce__(self) -> tuple[Callable[[], Cancelled], tuple[()]]: @@ -96,6 +104,19 @@ def __reduce__(self) -> tuple[Callable[[], Cancelled], tuple[()]]: (), ) + if TYPE_CHECKING: + # for type checking on internal code + @classmethod + def _create( + cls, + *, + source: Literal[ + "deadline", "nursery", "explicit", "unknown", "KeyboardInterrupt" + ], + source_task: str | None = None, + reason: str | None = None, + ) -> Self: ... + class BusyResourceError(Exception): """Raised when a task attempts to use a resource that some other task is diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index 771041007d..5ccb087ba0 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -306,6 +306,7 @@ def expire(self, now: float) -> bool: did_something = True # This implicitly calls self.remove(), so we don't need to # decrement _active here + cancel_scope._cancel_reason = CancelReason(source="deadline") cancel_scope.cancel() # If we've accumulated too many stale entries, then prune the heap to # keep it under control. (We only do this occasionally in a batch, to @@ -390,10 +391,6 @@ class CancelStatus: # recovery to show a useful traceback). abandoned_by_misnesting: bool = attrs.field(default=False, init=False, repr=False) - _cancel_reason: CancelReason | None = attrs.field( - default=None, init=False, repr=True, alias="cancel_reason" - ) - def __attrs_post_init__(self) -> None: if self._parent is not None: self._parent._children.add(self) @@ -482,24 +479,21 @@ def recalculate(self) -> None: ) if new_state != current.effectively_cancelled: if ( - current._scope._cancel_reason is not None - and current._cancel_reason is None - ): - current._cancel_reason = current._scope._cancel_reason - if ( - current._cancel_reason is None + current._scope._cancel_reason is None and current.parent_cancellation_is_visible_to_us ): assert current._parent is not None - current._cancel_reason = current._parent._cancel_reason + current._scope._cancel_reason = ( + current._parent._scope._cancel_reason + ) current.effectively_cancelled = new_state if new_state: for task in current._tasks: task._attempt_delivery_of_any_pending_cancel() # current._cancel_reason will not be None, unless there's misnesting for child in current._children: - if child._cancel_reason is None: - child._cancel_reason = current._cancel_reason + if child._scope._cancel_reason is None: + child._scope._cancel_reason = current._scope._cancel_reason todo.extend(current._children) def _mark_abandoned(self) -> None: @@ -632,8 +626,6 @@ def __enter__(self) -> Self: self.cancel() with self._might_change_registered_deadline(): self._cancel_status = CancelStatus(scope=self, parent=task._cancel_status) - if self._cancel_reason is not None: - self._cancel_status._cancel_reason = self._cancel_reason task._activate_cancel_status(self._cancel_status) return self @@ -939,8 +931,6 @@ def cancel(self, *, reason: str | None = None) -> None: reason=reason, source="explicit", source_task=current_task ) if self._cancel_status is not None: - # idk if strictly required - self._cancel_status._cancel_reason = self._cancel_reason self._cancel_status.recalculate() @property @@ -971,6 +961,7 @@ def cancel_called(self) -> bool: # but it makes the value returned by cancel_called more # closely match expectations. if not self._cancel_called and current_time() >= self._deadline: + self._cancel_reason = CancelReason(source="deadline") self.cancel() return self._cancel_called @@ -1241,6 +1232,7 @@ def parent_task(self) -> Task: def _add_exc(self, exc: BaseException) -> None: self._pending_excs.append(exc) + # TODO: source/reason? self.cancel_scope.cancel() def _check_nursery_closed(self) -> None: @@ -1626,20 +1618,24 @@ def _attempt_delivery_of_any_pending_cancel(self) -> None: if not self._cancel_status.effectively_cancelled: return - def raise_cancel() -> NoReturn: - if ( - self._cancel_status is None - or self._cancel_status._cancel_reason is None - ): - # self._cancel_status is None when using abandon_on_cancel=True (TODO: why?) - # _cancel_status._cancel_reason is None when misnesting - raise Cancelled._create(source="unknown") - raise Cancelled._create( - source=self._cancel_status._cancel_reason.source, - reason=self._cancel_status._cancel_reason.reason, - source_task=self._cancel_status._cancel_reason.source_task, + # FIXME: I haven't quite figured out how to get the cancel reason to stay + # alive for passing into other threads, but also not cause any cyclic garbage. + if ( + self._cancel_status is None + or self._cancel_status._scope._cancel_reason is None + ): + # _cancel_status._cancel_reason is None when misnesting + cancelled = Cancelled._create(source="unknown", reason="misnesting") + else: + cancelled = Cancelled._create( + source=self._cancel_status._scope._cancel_reason.source, + reason=self._cancel_status._scope._cancel_reason.reason, + source_task=self._cancel_status._scope._cancel_reason.source_task, ) + def raise_cancel() -> NoReturn: + raise cancelled + self._attempt_abort(raise_cancel) def _attempt_delivery_of_pending_ki(self) -> None: @@ -2137,6 +2133,7 @@ async def init( ) # Main task is done; start shutting down system tasks + # TODO: source/reason? self.system_nursery.cancel_scope.cancel() # System nursery is closed; finalize remaining async generators @@ -2145,6 +2142,7 @@ async def init( # There are no more asyncgens, which means no more user-provided # code except possibly run_sync_soon callbacks. It's finally safe # to stop the run_sync_soon task and exit run(). + # TODO: source/reason? run_sync_soon_nursery.cancel_scope.cancel() ################ @@ -2990,15 +2988,15 @@ async def checkpoint() -> None: ): cs = CancelScope(deadline=inf) if ( - task._cancel_status._cancel_reason is None + task._cancel_status._scope._cancel_reason is None and task is task._runner.main_task and task._runner.ki_pending ): - task._cancel_status._cancel_reason = CancelReason( + task._cancel_status._scope._cancel_reason = CancelReason( source="KeyboardInterrupt" ) - assert task._cancel_status._cancel_reason is not None - cs._cancel_reason = task._cancel_status._cancel_reason + assert task._cancel_status._scope._cancel_reason is not None + cs._cancel_reason = task._cancel_status._scope._cancel_reason with cs: await _core.wait_task_rescheduled(lambda _: _core.Abort.SUCCEEDED) diff --git a/src/trio/_core/_tests/test_cancel.py b/src/trio/_core/_tests/test_cancel.py index eb87ce1d16..66f0951c95 100644 --- a/src/trio/_core/_tests/test_cancel.py +++ b/src/trio/_core/_tests/test_cancel.py @@ -15,20 +15,19 @@ async def test_cancel_reason() -> None: cs.cancel(reason="hello") with pytest.raises( Cancelled, - match=rf"^Cancelled due to explicit with reason 'hello' from task {current_task()!r}$", + match=rf"^cancelled due to explicit with reason 'hello' from task {current_task()!r}$", ): await trio.lowlevel.checkpoint() with trio.CancelScope(deadline=-inf) as cs: - with pytest.raises(Cancelled, match=r"^Cancelled due to deadline"): + with pytest.raises(Cancelled, match=r"^cancelled due to deadline"): await trio.lowlevel.checkpoint() with trio.CancelScope() as cs: cs.deadline = -inf with pytest.raises( Cancelled, - # FIXME: ?? - match=r"^Cancelled due to explicit from task None", + match=r"^cancelled due to deadline", ): await trio.lowlevel.checkpoint() @@ -43,7 +42,7 @@ async def cancelled_task( ) -> None: task_status.started() with pytest.raises( - Cancelled, match=rf"^Cancelled due to nursery from task {fail_task!r}$" + Cancelled, match=rf"^cancelled due to nursery from task {fail_task!r}$" ): await trio.sleep_forever() raise TypeError @@ -67,7 +66,7 @@ async def cancelled_task( ) -> None: task_status.started() with pytest.raises( - Cancelled, match=rf"^Cancelled due to nursery from task {fail_task!r}$" + Cancelled, match=rf"^cancelled due to nursery from task {fail_task!r}$" ): await trio.sleep_forever() raise TypeError @@ -83,19 +82,24 @@ async def cancelled_task( async def test_cancel_reason_not_overwritten() -> None: with trio.CancelScope() as cs: cs.cancel() - with pytest.raises(Cancelled, match=r"^Cancelled due to explicit"): + with pytest.raises( + Cancelled, + match=rf"^cancelled due to explicit from task {current_task()!r}$", + ): await trio.lowlevel.checkpoint() cs.deadline = -inf - with pytest.raises(Cancelled, match=r"^Cancelled due to explicit"): + with pytest.raises( + Cancelled, + match=rf"^cancelled due to explicit from task {current_task()!r}$", + ): await trio.lowlevel.checkpoint() async def test_cancel_reason_not_overwritten_2() -> None: - # TODO: broken, see earlier test with trio.CancelScope() as cs: cs.deadline = -inf - with pytest.raises(Cancelled, match=r"^Cancelled due to explicit"): + with pytest.raises(Cancelled, match=r"^cancelled due to deadline$"): await trio.lowlevel.checkpoint() cs.cancel() - with pytest.raises(Cancelled, match=r"^Cancelled due to explicit"): + with pytest.raises(Cancelled, match=r"^cancelled due to deadline$"): await trio.lowlevel.checkpoint() diff --git a/src/trio/_core/_tests/test_run.py b/src/trio/_core/_tests/test_run.py index 075a521444..d68418c4ee 100644 --- a/src/trio/_core/_tests/test_run.py +++ b/src/trio/_core/_tests/test_run.py @@ -783,7 +783,8 @@ async def task1() -> None: async def task2() -> None: with _core.CancelScope(): with pytest.raises( - _core.Cancelled, match=r"^Cancelled due to unknown from task None$" + _core.Cancelled, + match=r"^cancelled due to unknown with reason 'misnesting'$", ): await sleep_forever() @@ -2230,25 +2231,26 @@ def test_Cancelled_init() -> None: async def test_Cancelled_str() -> None: - cancelled = _core.Cancelled._create(source="foo") - assert str(cancelled) == "Cancelled due to foo from task None" + cancelled = _core.Cancelled._create(source="explicit") + assert str(cancelled) == "cancelled due to explicit" assert re.fullmatch( - r"Cancelled due to bar from task " + r"cancelled due to deadline from task " r"", str( _core.Cancelled._create( - source="bar", - source_task=_core.current_task(), + source="deadline", + source_task=repr(_core.current_task()), ) ), ) + assert re.fullmatch( - r"Cancelled due to bar with reason 'pigs flying' from task " + r"cancelled due to nursery with reason 'pigs flying' from task " r"", str( _core.Cancelled._create( - source="bar", - source_task=_core.current_task(), + source="nursery", + source_task=repr(_core.current_task()), reason="pigs flying", ) ), @@ -2262,9 +2264,12 @@ def test_Cancelled_subclass() -> None: # https://github.com/python-trio/trio/issues/3248 def test_Cancelled_pickle() -> None: - cancelled = _core.Cancelled._create(source="foo") - cancelled = pickle.loads(pickle.dumps(cancelled)) - assert isinstance(cancelled, _core.Cancelled) + cancelled = _core.Cancelled._create(source="KeyboardInterrupt") + pickled_cancelled = pickle.loads(pickle.dumps(cancelled)) + assert isinstance(pickled_cancelled, _core.Cancelled) + assert cancelled.source == pickled_cancelled.source + assert cancelled.source_task == pickled_cancelled.source_task + assert cancelled.reason == pickled_cancelled.reason def test_CancelScope_subclass() -> None: diff --git a/src/trio/_highlevel_generic.py b/src/trio/_highlevel_generic.py index 041a684c62..9bd8822c9e 100644 --- a/src/trio/_highlevel_generic.py +++ b/src/trio/_highlevel_generic.py @@ -43,7 +43,7 @@ async def aclose_forcefully(resource: AsyncResource) -> None: """ with trio.CancelScope() as cs: - cs.cancel() + cs.cancel(reason="cancelled during aclose_forcefully") await resource.aclose() diff --git a/src/trio/_highlevel_open_tcp_stream.py b/src/trio/_highlevel_open_tcp_stream.py index 5723180e46..11460689b4 100644 --- a/src/trio/_highlevel_open_tcp_stream.py +++ b/src/trio/_highlevel_open_tcp_stream.py @@ -357,7 +357,7 @@ async def attempt_connect( # Success! Save the winning socket and cancel all outstanding # connection attempts. winning_socket = sock - nursery.cancel_scope.cancel() + nursery.cancel_scope.cancel(reason="successfully found a socket") except OSError as exc: # This connection attempt failed, but the next one might # succeed. Save the error for later so we can report it if diff --git a/src/trio/_subprocess.py b/src/trio/_subprocess.py index 823c50ea63..f93cd82fcc 100644 --- a/src/trio/_subprocess.py +++ b/src/trio/_subprocess.py @@ -766,6 +766,7 @@ async def killer() -> None: nursery.start_soon(killer) await proc.wait() + # TODO: source/reason? killer_cscope.cancel() raise diff --git a/src/trio/_tests/test_threads.py b/src/trio/_tests/test_threads.py index b57f5702e1..7b29ecdb32 100644 --- a/src/trio/_tests/test_threads.py +++ b/src/trio/_tests/test_threads.py @@ -1001,8 +1001,8 @@ async def child(abandon_on_cancel: bool, scope: CancelScope) -> None: record.append("start") try: return await to_thread_run_sync(f, abandon_on_cancel=abandon_on_cancel) - except _core.Cancelled: - record.append("cancel") + except _core.Cancelled as e: + record.append(str(e)) raise finally: record.append("exit") @@ -1010,8 +1010,8 @@ async def child(abandon_on_cancel: bool, scope: CancelScope) -> None: def f() -> None: try: from_thread_check_cancelled() - except _core.Cancelled: # pragma: no cover, test failure path - q.put("Cancelled") + except _core.Cancelled as e: # pragma: no cover, test failure path + q.put(str(e)) else: q.put("Not Cancelled") ev.wait() @@ -1042,8 +1042,13 @@ def f() -> None: scope.cancel() ev.set() assert scope.cancelled_caught - assert "cancel" in record - assert record[-1] == "exit" + assert re.fullmatch( + r"cancelled due to explicit from task " + r"", + record[1], + ), record[1] + assert record[2] == "exit" + assert len(record) == 3 # abandon_on_cancel=True case: slightly different thread behavior needed # check thread is cancelled "soon" after abandonment @@ -1051,8 +1056,8 @@ def f() -> None: # type: ignore[no-redef] # noqa: F811 ev.wait() try: from_thread_check_cancelled() - except _core.Cancelled: - q.put("Cancelled") + except _core.Cancelled as e: + q.put(str(e)) except BaseException as e: # pragma: no cover, test failure path # abandon_on_cancel=True will eat exceptions, so we pass it # through the queue in order to be able to debug any exceptions @@ -1070,13 +1075,21 @@ def f() -> None: # type: ignore[no-redef] # noqa: F811 scope.cancel() ev.set() assert scope.cancelled_caught - assert "cancel" in record + assert re.fullmatch( + r"cancelled due to explicit from task " + r"", + record[1], + ), record[1] assert record[-1] == "exit" res = q.get(timeout=1) if isinstance(res, BaseException): # pragma: no cover # for debugging raise res else: - assert res == "Cancelled" + assert re.fullmatch( + r"cancelled due to explicit from task " + r"", + res, + ), res def test_from_thread_check_cancelled_raises_in_foreign_threads() -> None: diff --git a/src/trio/_tests/test_util.py b/src/trio/_tests/test_util.py index e907da9c10..4182ba5db4 100644 --- a/src/trio/_tests/test_util.py +++ b/src/trio/_tests/test_util.py @@ -282,7 +282,7 @@ async def test_raise_single_exception_from_group() -> None: context = TypeError("context") exc.__cause__ = cause exc.__context__ = context - cancelled = trio.Cancelled._create(source="foo") + cancelled = trio.Cancelled._create(source="deadline") with pytest.raises(ValueError, match="foo") as excinfo: raise_single_exception_from_group(ExceptionGroup("", [exc])) @@ -346,11 +346,11 @@ async def test_raise_single_exception_from_group() -> None: assert excinfo.value.__context__ is None # if we only got cancelled, first one is reraised - with pytest.raises( - trio.Cancelled, match=r"^Cancelled due to foo from task None$" - ) as excinfo: + with pytest.raises(trio.Cancelled, match=r"^cancelled due to deadline$") as excinfo: raise_single_exception_from_group( - BaseExceptionGroup("", [cancelled, trio.Cancelled._create(source="bar")]) + BaseExceptionGroup( + "", [cancelled, trio.Cancelled._create(source="explicit")] + ) ) assert excinfo.value is cancelled assert excinfo.value.__cause__ is None From af964cf0a74c9f0a670c8deb1b3535bb37a04211 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Mon, 28 Apr 2025 17:22:53 +0200 Subject: [PATCH 03/15] fixes some errors from review --- src/trio/_core/_exceptions.py | 22 +++++++---- src/trio/_core/_run.py | 56 +++++++++++++++++++++------- src/trio/_core/_tests/test_cancel.py | 50 ++++++++++++++++++++++++- src/trio/_subprocess.py | 3 +- 4 files changed, 106 insertions(+), 25 deletions(-) diff --git a/src/trio/_core/_exceptions.py b/src/trio/_core/_exceptions.py index db3a01b3e2..e2af1b00a2 100644 --- a/src/trio/_core/_exceptions.py +++ b/src/trio/_core/_exceptions.py @@ -73,27 +73,30 @@ class Cancelled(BaseException, metaclass=NoPublicConstructor): """ - source: Literal["deadline", "nursery", "explicit", "unknown", "KeyboardInterrupt"] - # TODO: this should probably be a Task? + source: Literal[ + "KeyboardInterrupt", "deadline", "explicit", "nursery", "shutdown", "unknown" + ] + # repr(Task), so as to avoid gc troubles from holding a reference source_task: str | None = None reason: str | None = None def __str__(self) -> str: - def repr_if_not_none(lead: str, s: str | None, trail: str = "") -> str: + def repr_if_not_none(lead: str, s: str | None, do_repr: bool = False) -> str: if s is None: return "" - return lead + s + trail + if do_repr: + return lead + repr(s) + return lead + s return ( f"cancelled due to {self.source}" - + repr_if_not_none(" with reason '", self.reason, "'") + + repr_if_not_none(" with reason ", self.reason, True) + repr_if_not_none(" from task ", self.source_task) ) def __reduce__(self) -> tuple[Callable[[], Cancelled], tuple[()]]: # the `__reduce__` tuple does not support kwargs, so we must use partial # for non-default args - # or switch to allow posarg (?) return ( partial( Cancelled._create, @@ -111,7 +114,12 @@ def _create( cls, *, source: Literal[ - "deadline", "nursery", "explicit", "unknown", "KeyboardInterrupt" + "KeyboardInterrupt", + "deadline", + "explicit", + "nursery", + "shutdown", + "unknown", ], source_task: str | None = None, reason: str | None = None, diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index 5ccb087ba0..36183e4e8f 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -318,8 +318,14 @@ def expire(self, now: float) -> bool: @attrs.define class CancelReason: - # TODO: loren ipsum - source: Literal["deadline", "nursery", "explicit", "KeyboardInterrupt"] + """Attached to a :class:`CancelScope` upon cancellation with details of the source of the + cancellation, which is then used to construct the string in a :exc:`Cancelled`. + Users can pass a ``reason`` str to :meth:`CancelScope.cancel` to set it. + """ + + source: Literal[ + "KeyboardInterrupt", "deadline", "explicit", "nursery", "shutdown", "unknown" + ] source_task: str | None = None reason: str | None = None @@ -580,8 +586,6 @@ class CancelScope: _cancel_called: bool = attrs.field(default=False, init=False) cancelled_caught: bool = attrs.field(default=False, init=False) - # necessary as cancel_status might be None - # TODO: but maybe cancel_status doesn't need it? _cancel_reason: CancelReason | None = attrs.field( default=None, init=False, repr=True ) @@ -1230,9 +1234,10 @@ def parent_task(self) -> Task: "(`~trio.lowlevel.Task`): The Task that opened this nursery." return self._parent_task - def _add_exc(self, exc: BaseException) -> None: + def _add_exc(self, exc: BaseException, reason: CancelReason | None) -> None: self._pending_excs.append(exc) - # TODO: source/reason? + if self.cancel_scope._cancel_reason is None: + self.cancel_scope._cancel_reason = reason self.cancel_scope.cancel() def _check_nursery_closed(self) -> None: @@ -1249,11 +1254,14 @@ def _child_finished( ) -> None: self._children.remove(task) if isinstance(outcome, Error): - if self.cancel_scope._cancel_reason is None: - self.cancel_scope._cancel_reason = CancelReason( - source="nursery", source_task=repr(task) - ) - self._add_exc(outcome.error) + self._add_exc( + outcome.error, + CancelReason( + source="nursery", + source_task=repr(task), + reason=f"child task raised exception {outcome.error!r}", + ), + ) self._check_nursery_closed() async def _nested_child_finished( @@ -1263,7 +1271,14 @@ async def _nested_child_finished( # Returns ExceptionGroup instance (or any exception if the nursery is in loose mode # and there is just one contained exception) if there are pending exceptions if nested_child_exc is not None: - self._add_exc(nested_child_exc) + self._add_exc( + nested_child_exc, + reason=CancelReason( + source="nursery", + source_task=repr(self._parent_task), + reason=f"Code block inside nursery contextmanager raised exception {nested_child_exc!r}", + ), + ) self._nested_child_running = False self._check_nursery_closed() @@ -1274,7 +1289,13 @@ async def _nested_child_finished( def aborted(raise_cancel: _core.RaiseCancelT) -> Abort: exn = capture(raise_cancel).error if not isinstance(exn, Cancelled): - self._add_exc(exn) + self._add_exc( + exn, + CancelReason( + source="KeyboardInterrupt", + source_task=repr(self._parent_task), + ), + ) # see test_cancel_scope_exit_doesnt_create_cyclic_garbage del exn # prevent cyclic garbage creation return Abort.FAILED @@ -1288,7 +1309,8 @@ def aborted(raise_cancel: _core.RaiseCancelT) -> Abort: try: await cancel_shielded_checkpoint() except BaseException as exc: - self._add_exc(exc) + # there's no children to cancel, so don't need to supply cancel reason + self._add_exc(exc, reason=None) popped = self._parent_task._child_nurseries.pop() assert popped is self @@ -2134,6 +2156,12 @@ async def init( # Main task is done; start shutting down system tasks # TODO: source/reason? + self.system_nursery.cancel_scope._cancel_reason = CancelReason( + source="shutdown", + reason="main task done, shutting down system tasks", + source_task=repr(self.init_task), + ) + self.system_nursery.cancel_scope.cancel() # System nursery is closed; finalize remaining async generators diff --git a/src/trio/_core/_tests/test_cancel.py b/src/trio/_core/_tests/test_cancel.py index 66f0951c95..5fa8bf741e 100644 --- a/src/trio/_core/_tests/test_cancel.py +++ b/src/trio/_core/_tests/test_cancel.py @@ -9,6 +9,8 @@ from trio.lowlevel import current_task from trio.testing import RaisesGroup +from .test_ki import ki_self + async def test_cancel_reason() -> None: with trio.CancelScope() as cs: @@ -42,7 +44,8 @@ async def cancelled_task( ) -> None: task_status.started() with pytest.raises( - Cancelled, match=rf"^cancelled due to nursery from task {fail_task!r}$" + Cancelled, + match=rf"^cancelled due to nursery with reason 'child task raised exception ValueError\(\)' from task {fail_task!r}$", ): await trio.sleep_forever() raise TypeError @@ -66,7 +69,8 @@ async def cancelled_task( ) -> None: task_status.started() with pytest.raises( - Cancelled, match=rf"^cancelled due to nursery from task {fail_task!r}$" + Cancelled, + match=rf"^cancelled due to nursery with reason 'child task raised exception ValueError\(\)' from task {fail_task!r}$", ): await trio.sleep_forever() raise TypeError @@ -103,3 +107,45 @@ async def test_cancel_reason_not_overwritten_2() -> None: cs.cancel() with pytest.raises(Cancelled, match=r"^cancelled due to deadline$"): await trio.lowlevel.checkpoint() + + +async def test_nested_child_source() -> None: + ev = trio.Event() + parent_task = current_task() + + async def child() -> None: + ev.set() + with pytest.raises( + Cancelled, + match=rf"^cancelled due to nursery with reason 'Code block inside nursery contextmanager raised exception ValueError\(\)' from task {parent_task!r}$", + ): + await trio.sleep_forever() + + with RaisesGroup(ValueError): + async with trio.open_nursery() as nursery: + nursery.start_soon(child) + await ev.wait() + raise ValueError + + +async def test_reason_delayed_ki() -> None: + # simplified version of test_ki.test_ki_protection_works check #2 + parent_task = current_task() + + async def sleeper(name: str) -> None: + with pytest.raises( + Cancelled, + match=rf"^cancelled due to KeyboardInterrupt from task {parent_task!r}$", + ): + while True: + await trio.lowlevel.checkpoint() + + async def raiser(name: str) -> None: + ki_self() + + with RaisesGroup(KeyboardInterrupt): + async with trio.open_nursery() as nursery: + nursery.start_soon(sleeper, "s1") + nursery.start_soon(sleeper, "s2") + nursery.start_soon(trio.lowlevel.enable_ki_protection(raiser), "r1") + # __aexit__ blocks, and then receives the KI diff --git a/src/trio/_subprocess.py b/src/trio/_subprocess.py index f93cd82fcc..c1b162e308 100644 --- a/src/trio/_subprocess.py +++ b/src/trio/_subprocess.py @@ -766,8 +766,7 @@ async def killer() -> None: nursery.start_soon(killer) await proc.wait() - # TODO: source/reason? - killer_cscope.cancel() + killer_cscope.cancel(reason="trio internal implementation detail") raise stdout = b"".join(stdout_chunks) if capture_stdout else None From 69c398bc9b1332f95fcb7fbedc9ffb123f86d1fb Mon Sep 17 00:00:00 2001 From: jakkdl Date: Tue, 29 Apr 2025 12:28:48 +0200 Subject: [PATCH 04/15] resolved threads+gc problem, though not sure if in a good way --- src/trio/_core/_run.py | 38 +++++++++++++++++++------------------- src/trio/_threads.py | 4 +++- 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index 36183e4e8f..66cf57ca8c 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -1640,25 +1640,7 @@ def _attempt_delivery_of_any_pending_cancel(self) -> None: if not self._cancel_status.effectively_cancelled: return - # FIXME: I haven't quite figured out how to get the cancel reason to stay - # alive for passing into other threads, but also not cause any cyclic garbage. - if ( - self._cancel_status is None - or self._cancel_status._scope._cancel_reason is None - ): - # _cancel_status._cancel_reason is None when misnesting - cancelled = Cancelled._create(source="unknown", reason="misnesting") - else: - cancelled = Cancelled._create( - source=self._cancel_status._scope._cancel_reason.source, - reason=self._cancel_status._scope._cancel_reason.reason, - source_task=self._cancel_status._scope._cancel_reason.source_task, - ) - - def raise_cancel() -> NoReturn: - raise cancelled - - self._attempt_abort(raise_cancel) + self._attempt_abort(RaiseCancel(self._cancel_status._scope._cancel_reason)) def _attempt_delivery_of_pending_ki(self) -> None: assert self._runner.ki_pending @@ -1672,6 +1654,24 @@ def raise_cancel() -> NoReturn: self._attempt_abort(raise_cancel) +class RaiseCancel: + def __init__(self, reason: CancelReason | None) -> None: + if reason is None: + self.cancelled = Cancelled._create(source="unknown", reason="misnesting") + else: + self.cancelled = Cancelled._create( + source=reason.source, + reason=reason.reason, + source_task=reason.source_task, + ) + + def __call__(self) -> NoReturn: + try: + raise self.cancelled + finally: + del self.cancelled + + ################################################################ # The central Runner object ################################################################ diff --git a/src/trio/_threads.py b/src/trio/_threads.py index 394e5b06ac..4419bdb84a 100644 --- a/src/trio/_threads.py +++ b/src/trio/_threads.py @@ -430,7 +430,9 @@ def deliver_worker_fn_result(result: outcome.Outcome[RetT]) -> None: def abort(raise_cancel: RaiseCancelT) -> trio.lowlevel.Abort: # fill so from_thread_check_cancelled can raise - cancel_register[0] = raise_cancel + import copy + + cancel_register[0] = copy.copy(raise_cancel) if abandon_bool: # empty so report_back_in_trio_thread_fn cannot reschedule task_register[0] = None From 357b7978eba8cdd2eb5e7db76eac0af65f3ef1dd Mon Sep 17 00:00:00 2001 From: jakkdl Date: Tue, 29 Apr 2025 13:03:57 +0200 Subject: [PATCH 05/15] final fixes after review, add newsfragment --- src/trio/_channel.py | 10 ++++++++-- src/trio/_core/_asyncgens.py | 2 +- src/trio/_core/_run.py | 8 +++++++- src/trio/_threads.py | 5 +++-- 4 files changed, 19 insertions(+), 6 deletions(-) diff --git a/src/trio/_channel.py b/src/trio/_channel.py index 2768ecd44a..7ecfd6906c 100644 --- a/src/trio/_channel.py +++ b/src/trio/_channel.py @@ -17,7 +17,8 @@ import trio from ._abc import ReceiveChannel, ReceiveType, SendChannel, SendType, T -from ._core import Abort, RaiseCancelT, Task, enable_ki_protection +from ._core import Abort, RaiseCancelT, Task, current_task, enable_ki_protection +from ._core._run import CancelReason from ._util import ( MultipleExceptionError, NoPublicConstructor, @@ -547,7 +548,12 @@ async def context_manager( yield wrapped_recv_chan # User has exited context manager, cancel to immediately close the # abandoned generator if it's still alive. - nursery.cancel_scope.cancel(reason="user exited context manager") + nursery.cancel_scope._cancel_reason = CancelReason( + reason="exited _as_safe_channel context manager", + source="shutdown", + source_task=repr(current_task), + ) + nursery.cancel_scope.cancel() except BaseExceptionGroup as eg: try: raise_single_exception_from_group(eg) diff --git a/src/trio/_core/_asyncgens.py b/src/trio/_core/_asyncgens.py index cd01df548d..fea41e0e4d 100644 --- a/src/trio/_core/_asyncgens.py +++ b/src/trio/_core/_asyncgens.py @@ -231,7 +231,7 @@ async def _finalize_one( # is cancelled so there's no deadlock risk. with _core.CancelScope(shield=True) as cancel_scope: cancel_scope.cancel( - reason="internal trio cancellation during finalization" + reason="disallow async work when closing async generators during trio shutdown" ) await agen.aclose() except BaseException: diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index 66cf57ca8c..c52c679522 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -321,6 +321,8 @@ class CancelReason: """Attached to a :class:`CancelScope` upon cancellation with details of the source of the cancellation, which is then used to construct the string in a :exc:`Cancelled`. Users can pass a ``reason`` str to :meth:`CancelScope.cancel` to set it. + + Not publicly exported or documented. """ source: Literal[ @@ -916,9 +918,13 @@ def shield(self, new_value: bool) -> None: self._cancel_status.recalculate() @enable_ki_protection - def cancel(self, *, reason: str | None = None) -> None: + def cancel(self, reason: str | None = None) -> None: """Cancels this scope immediately. + The optional ``reason`` argument accepts a string, which will be attached to + any resulting :exc:`Cancelled` exception to help you understand where that + cancellation is coming from and why it happened. + This method is idempotent, i.e., if the scope was already cancelled then this method silently does nothing. """ diff --git a/src/trio/_threads.py b/src/trio/_threads.py index 4419bdb84a..e8046ba860 100644 --- a/src/trio/_threads.py +++ b/src/trio/_threads.py @@ -2,6 +2,7 @@ import contextlib import contextvars +import copy import inspect import queue as stdlib_queue import threading @@ -430,8 +431,8 @@ def deliver_worker_fn_result(result: outcome.Outcome[RetT]) -> None: def abort(raise_cancel: RaiseCancelT) -> trio.lowlevel.Abort: # fill so from_thread_check_cancelled can raise - import copy - + # 'raise_cancel' will immediately delete its reason object, so we make + # a copy in each thread cancel_register[0] = copy.copy(raise_cancel) if abandon_bool: # empty so report_back_in_trio_thread_fn cannot reschedule From 1bc89b284a12f0dbe1558f2c3a32018b782af893 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Tue, 29 Apr 2025 13:10:17 +0200 Subject: [PATCH 06/15] actually add the newsfragment --- newsfragments/3232.feature.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 newsfragments/3232.feature.rst diff --git a/newsfragments/3232.feature.rst b/newsfragments/3232.feature.rst new file mode 100644 index 0000000000..9da76cb370 --- /dev/null +++ b/newsfragments/3232.feature.rst @@ -0,0 +1 @@ +:exc:`Cancelled` strings can now display the source and reason for a cancellation. Trio-internal sources of cancellation will set this string, and :meth:`CancelScope.cancel` now has a ``reason`` string parameter that can be used to attach info to any :exc:`Cancelled` to help in debugging. From 7def83f454417ff2308546ce0091cddb5209bca5 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Thu, 1 May 2025 12:34:51 +0200 Subject: [PATCH 07/15] add _cancel, other fixes after review --- src/trio/_channel.py | 11 ++--- src/trio/_core/_exceptions.py | 16 +++---- src/trio/_core/_run.py | 78 +++++++++++++++++------------------ src/trio/_util.py | 3 +- 4 files changed, 51 insertions(+), 57 deletions(-) diff --git a/src/trio/_channel.py b/src/trio/_channel.py index 7ecfd6906c..12d4a275cd 100644 --- a/src/trio/_channel.py +++ b/src/trio/_channel.py @@ -548,12 +548,13 @@ async def context_manager( yield wrapped_recv_chan # User has exited context manager, cancel to immediately close the # abandoned generator if it's still alive. - nursery.cancel_scope._cancel_reason = CancelReason( - reason="exited _as_safe_channel context manager", - source="shutdown", - source_task=repr(current_task), + nursery.cancel_scope._cancel( + CancelReason( + reason="exited trio.as_safe_channel context manager", + source="shutdown", + source_task=repr(current_task), + ) ) - nursery.cancel_scope.cancel() except BaseExceptionGroup as eg: try: raise_single_exception_from_group(eg) diff --git a/src/trio/_core/_exceptions.py b/src/trio/_core/_exceptions.py index e2af1b00a2..859ca595d4 100644 --- a/src/trio/_core/_exceptions.py +++ b/src/trio/_core/_exceptions.py @@ -81,22 +81,16 @@ class Cancelled(BaseException, metaclass=NoPublicConstructor): reason: str | None = None def __str__(self) -> str: - def repr_if_not_none(lead: str, s: str | None, do_repr: bool = False) -> str: - if s is None: - return "" - if do_repr: - return lead + repr(s) - return lead + s - return ( f"cancelled due to {self.source}" - + repr_if_not_none(" with reason ", self.reason, True) - + repr_if_not_none(" from task ", self.source_task) + + ("" if self.reason is None else f" with reason {self.reason!r}") + + ("" if self.source_task is None else f" from task {self.source_task}") ) def __reduce__(self) -> tuple[Callable[[], Cancelled], tuple[()]]: - # the `__reduce__` tuple does not support kwargs, so we must use partial - # for non-default args + # The `__reduce__` tuple does not support directly passing kwargs, and the + # kwargs are required so we can't use the third item for adding to __dict__, + # so we use partial. return ( partial( Cancelled._create, diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index c52c679522..347a49bbef 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -306,8 +306,7 @@ def expire(self, now: float) -> bool: did_something = True # This implicitly calls self.remove(), so we don't need to # decrement _active here - cancel_scope._cancel_reason = CancelReason(source="deadline") - cancel_scope.cancel() + cancel_scope._cancel(CancelReason(source="deadline")) # If we've accumulated too many stale entries, then prune the heap to # keep it under control. (We only do this occasionally in a batch, to # keep the amortized cost down) @@ -498,10 +497,6 @@ def recalculate(self) -> None: if new_state: for task in current._tasks: task._attempt_delivery_of_any_pending_cancel() - # current._cancel_reason will not be None, unless there's misnesting - for child in current._children: - if child._scope._cancel_reason is None: - child._scope._cancel_reason = current._scope._cancel_reason todo.extend(current._children) def _mark_abandoned(self) -> None: @@ -588,9 +583,7 @@ class CancelScope: _cancel_called: bool = attrs.field(default=False, init=False) cancelled_caught: bool = attrs.field(default=False, init=False) - _cancel_reason: CancelReason | None = attrs.field( - default=None, init=False, repr=True - ) + _cancel_reason: CancelReason | None = attrs.field(default=None, init=False) # Constructor arguments: _relative_deadline: float = attrs.field( @@ -628,8 +621,7 @@ def __enter__(self) -> Self: self._relative_deadline = inf if current_time() >= self._deadline: - self._cancel_reason = CancelReason(source="deadline") - self.cancel() + self._cancel(CancelReason(source="deadline")) with self._might_change_registered_deadline(): self._cancel_status = CancelStatus(scope=self, parent=task._cancel_status) task._activate_cancel_status(self._cancel_status) @@ -917,6 +909,20 @@ def shield(self, new_value: bool) -> None: if self._cancel_status is not None: self._cancel_status.recalculate() + @enable_ki_protection + def _cancel(self, cancel_reason: CancelReason | None) -> None: + if self._cancel_called: + return + + if self._cancel_reason is None: + self._cancel_reason = cancel_reason + + with self._might_change_registered_deadline(): + self._cancel_called = True + + if self._cancel_status is not None: + self._cancel_status.recalculate() + @enable_ki_protection def cancel(self, reason: str | None = None) -> None: """Cancels this scope immediately. @@ -928,20 +934,13 @@ def cancel(self, reason: str | None = None) -> None: This method is idempotent, i.e., if the scope was already cancelled then this method silently does nothing. """ - if self._cancel_called: - return - with self._might_change_registered_deadline(): - self._cancel_called = True - if self._cancel_reason is None: - try: - current_task = repr(_core.current_task()) - except RuntimeError: - current_task = None - self._cancel_reason = CancelReason( - reason=reason, source="explicit", source_task=current_task - ) - if self._cancel_status is not None: - self._cancel_status.recalculate() + try: + current_task = repr(_core.current_task()) + except RuntimeError: + current_task = None + self._cancel( + CancelReason(reason=reason, source="explicit", source_task=current_task) + ) @property def cancel_called(self) -> bool: @@ -971,8 +970,7 @@ def cancel_called(self) -> bool: # but it makes the value returned by cancel_called more # closely match expectations. if not self._cancel_called and current_time() >= self._deadline: - self._cancel_reason = CancelReason(source="deadline") - self.cancel() + self._cancel(CancelReason(source="deadline")) return self._cancel_called @@ -1242,9 +1240,7 @@ def parent_task(self) -> Task: def _add_exc(self, exc: BaseException, reason: CancelReason | None) -> None: self._pending_excs.append(exc) - if self.cancel_scope._cancel_reason is None: - self.cancel_scope._cancel_reason = reason - self.cancel_scope.cancel() + self.cancel_scope._cancel(reason) def _check_nursery_closed(self) -> None: if not any([self._nested_child_running, self._children, self._pending_starts]): @@ -2161,23 +2157,27 @@ async def init( ) # Main task is done; start shutting down system tasks - # TODO: source/reason? - self.system_nursery.cancel_scope._cancel_reason = CancelReason( - source="shutdown", - reason="main task done, shutting down system tasks", - source_task=repr(self.init_task), + self.system_nursery.cancel_scope._cancel( + CancelReason( + source="shutdown", + reason="main task done, shutting down system tasks", + source_task=repr(self.init_task), + ) ) - self.system_nursery.cancel_scope.cancel() - # System nursery is closed; finalize remaining async generators await self.asyncgens.finalize_remaining(self) # There are no more asyncgens, which means no more user-provided # code except possibly run_sync_soon callbacks. It's finally safe # to stop the run_sync_soon task and exit run(). - # TODO: source/reason? - run_sync_soon_nursery.cancel_scope.cancel() + run_sync_soon_nursery.cancel_scope._cancel( + CancelReason( + source="shutdown", + reason="main task done, shut down run_sync_soon callbacks", + source_task=repr(self.init_task), + ) + ) ################ # Outside context problems diff --git a/src/trio/_util.py b/src/trio/_util.py index 6656749111..106423e2aa 100644 --- a/src/trio/_util.py +++ b/src/trio/_util.py @@ -29,12 +29,11 @@ import sys from types import AsyncGeneratorType, TracebackType - from typing_extensions import ParamSpec, Self, TypeVarTuple, Unpack + from typing_extensions import Self, TypeVarTuple, Unpack if sys.version_info < (3, 11): from exceptiongroup import BaseExceptionGroup - ArgsT = ParamSpec("ArgsT") PosArgsT = TypeVarTuple("PosArgsT") From ebec334f1bec4e36802f75c7e9cecaa80bd817c0 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Thu, 1 May 2025 12:43:52 +0200 Subject: [PATCH 08/15] make as_safe_channel use normal cancel, add docstring --- src/trio/_channel.py | 11 +++-------- src/trio/_core/_run.py | 5 +++++ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/trio/_channel.py b/src/trio/_channel.py index 12d4a275cd..54b5ea4bea 100644 --- a/src/trio/_channel.py +++ b/src/trio/_channel.py @@ -17,8 +17,7 @@ import trio from ._abc import ReceiveChannel, ReceiveType, SendChannel, SendType, T -from ._core import Abort, RaiseCancelT, Task, current_task, enable_ki_protection -from ._core._run import CancelReason +from ._core import Abort, RaiseCancelT, Task, enable_ki_protection from ._util import ( MultipleExceptionError, NoPublicConstructor, @@ -548,12 +547,8 @@ async def context_manager( yield wrapped_recv_chan # User has exited context manager, cancel to immediately close the # abandoned generator if it's still alive. - nursery.cancel_scope._cancel( - CancelReason( - reason="exited trio.as_safe_channel context manager", - source="shutdown", - source_task=repr(current_task), - ) + nursery.cancel_scope.cancel( + "exited trio.as_safe_channel context manager" ) except BaseExceptionGroup as eg: try: diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index 347a49bbef..bcee507114 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -911,6 +911,11 @@ def shield(self, new_value: bool) -> None: @enable_ki_protection def _cancel(self, cancel_reason: CancelReason | None) -> None: + """Internal sources of cancellation should use this instead of :meth:`cancel` + in order to set a more detailed :class:`CancelReason` + Helper or high-level functions can use `cancel`. + """ + if self._cancel_called: return From 87a8e07a1d6eb193fc2698a9a18bc5f81b88a14e Mon Sep 17 00:00:00 2001 From: jakkdl Date: Fri, 2 May 2025 13:23:14 +0200 Subject: [PATCH 09/15] add CancelReasonLiteral typealias --- src/trio/_core/_exceptions.py | 24 ++++++++++++------------ src/trio/_core/_run.py | 13 +++++++------ 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/src/trio/_core/_exceptions.py b/src/trio/_core/_exceptions.py index 859ca595d4..0974d8eb5e 100644 --- a/src/trio/_core/_exceptions.py +++ b/src/trio/_core/_exceptions.py @@ -10,7 +10,16 @@ if TYPE_CHECKING: from collections.abc import Callable - from typing_extensions import Self + from typing_extensions import Self, TypeAlias + +CancelReasonLiteral: TypeAlias = Literal[ + "KeyboardInterrupt", + "deadline", + "explicit", + "nursery", + "shutdown", + "unknown", +] class TrioInternalError(Exception): @@ -73,9 +82,7 @@ class Cancelled(BaseException, metaclass=NoPublicConstructor): """ - source: Literal[ - "KeyboardInterrupt", "deadline", "explicit", "nursery", "shutdown", "unknown" - ] + source: CancelReasonLiteral # repr(Task), so as to avoid gc troubles from holding a reference source_task: str | None = None reason: str | None = None @@ -107,14 +114,7 @@ def __reduce__(self) -> tuple[Callable[[], Cancelled], tuple[()]]: def _create( cls, *, - source: Literal[ - "KeyboardInterrupt", - "deadline", - "explicit", - "nursery", - "shutdown", - "unknown", - ], + source: CancelReasonLiteral, source_task: str | None = None, reason: str | None = None, ) -> Self: ... diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index bcee507114..c4f77d9ac7 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -18,7 +18,6 @@ TYPE_CHECKING, Any, Final, - Literal, NoReturn, Protocol, cast, @@ -37,7 +36,12 @@ from ._asyncgens import AsyncGenerators from ._concat_tb import concat_tb from ._entry_queue import EntryQueue, TrioToken -from ._exceptions import Cancelled, RunFinishedError, TrioInternalError +from ._exceptions import ( + Cancelled, + CancelReasonLiteral, + RunFinishedError, + TrioInternalError, +) from ._instrumentation import Instruments from ._ki import KIManager, enable_ki_protection from ._parking_lot import GLOBAL_PARKING_LOT_BREAKER @@ -324,9 +328,7 @@ class CancelReason: Not publicly exported or documented. """ - source: Literal[ - "KeyboardInterrupt", "deadline", "explicit", "nursery", "shutdown", "unknown" - ] + source: CancelReasonLiteral source_task: str | None = None reason: str | None = None @@ -915,7 +917,6 @@ def _cancel(self, cancel_reason: CancelReason | None) -> None: in order to set a more detailed :class:`CancelReason` Helper or high-level functions can use `cancel`. """ - if self._cancel_called: return From 36cf22274359509e1ea223daa4b40fda89db47fb Mon Sep 17 00:00:00 2001 From: jakkdl Date: Mon, 5 May 2025 17:13:39 +0200 Subject: [PATCH 10/15] rename test_cancel to test_cancelled, move cancelled tests to it. Add test. minor fixes --- src/trio/_core/_run.py | 2 +- .../{test_cancel.py => test_cancelled.py} | 131 ++++++++++++++---- src/trio/_core/_tests/test_run.py | 55 -------- 3 files changed, 102 insertions(+), 86 deletions(-) rename src/trio/_core/_tests/{test_cancel.py => test_cancelled.py} (51%) diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index c4f77d9ac7..541e1f4fd0 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -2180,7 +2180,7 @@ async def init( run_sync_soon_nursery.cancel_scope._cancel( CancelReason( source="shutdown", - reason="main task done, shut down run_sync_soon callbacks", + reason="main task done, shutting down run_sync_soon callbacks", source_task=repr(self.init_task), ) ) diff --git a/src/trio/_core/_tests/test_cancel.py b/src/trio/_core/_tests/test_cancelled.py similarity index 51% rename from src/trio/_core/_tests/test_cancel.py rename to src/trio/_core/_tests/test_cancelled.py index 5fa8bf741e..18af4a367e 100644 --- a/src/trio/_core/_tests/test_cancel.py +++ b/src/trio/_core/_tests/test_cancelled.py @@ -1,5 +1,5 @@ -# there's tons of cancellation testing in test_run, but that file is 3k lines long already... - +import pickle +import re from math import inf import pytest @@ -7,19 +7,75 @@ import trio from trio import Cancelled from trio.lowlevel import current_task -from trio.testing import RaisesGroup +from trio.testing import RaisesGroup, wait_all_tasks_blocked from .test_ki import ki_self +def test_Cancelled_init() -> None: + with pytest.raises(TypeError, match=r"^trio.Cancelled has no public constructor$"): + raise Cancelled # type: ignore[call-arg] + + with pytest.raises(TypeError, match=r"^trio.Cancelled has no public constructor$"): + Cancelled(source="explicit") + + # private constructor should not raise + Cancelled._create(source="explicit") + + +async def test_Cancelled_str() -> None: + cancelled = Cancelled._create(source="explicit") + assert str(cancelled) == "cancelled due to explicit" + # note: repr(current_task()) is often fairly verbose + assert re.fullmatch( + r"cancelled due to deadline from task " + r"", + str( + Cancelled._create( + source="deadline", + source_task=repr(current_task()), + ) + ), + ) + + assert re.fullmatch( + rf"cancelled due to nursery with reason 'pigs flying' from task {current_task()!r}", + str( + Cancelled._create( + source="nursery", + source_task=repr(current_task()), + reason="pigs flying", + ) + ), + ) + + +def test_Cancelled_subclass() -> None: + with pytest.raises(TypeError): + type("Subclass", (Cancelled,), {}) + + +# https://github.com/python-trio/trio/issues/3248 +def test_Cancelled_pickle() -> None: + cancelled = Cancelled._create(source="KeyboardInterrupt") + pickled_cancelled = pickle.loads(pickle.dumps(cancelled)) + assert isinstance(pickled_cancelled, Cancelled) + assert cancelled.source == pickled_cancelled.source + assert cancelled.source_task == pickled_cancelled.source_task + assert cancelled.reason == pickled_cancelled.reason + + async def test_cancel_reason() -> None: with trio.CancelScope() as cs: cs.cancel(reason="hello") with pytest.raises( Cancelled, match=rf"^cancelled due to explicit with reason 'hello' from task {current_task()!r}$", - ): + ) as excinfo: await trio.lowlevel.checkpoint() + assert excinfo.value.source == "explicit" + assert excinfo.value.reason == "hello" + assert excinfo.value.source_task == repr(current_task()) with trio.CancelScope(deadline=-inf) as cs: with pytest.raises(Cancelled, match=r"^cancelled due to deadline"): @@ -34,53 +90,68 @@ async def test_cancel_reason() -> None: await trio.lowlevel.checkpoint() +match_str = r"^cancelled due to nursery with reason 'child task raised exception ValueError\(\)' from task {0!r}$" + + +async def cancelled_task( + fail_task: trio.lowlevel.Task, task_status: trio.TaskStatus +) -> None: + task_status.started() + with pytest.raises(Cancelled, match=match_str.format(fail_task)): + await trio.sleep_forever() + raise TypeError + + +# failing_task raises before cancelled_task is started async def test_cancel_reason_nursery() -> None: async def failing_task(task_status: trio.TaskStatus[trio.lowlevel.Task]) -> None: task_status.started(current_task()) raise ValueError - async def cancelled_task( - fail_task: trio.lowlevel.Task, task_status: trio.TaskStatus - ) -> None: - task_status.started() - with pytest.raises( - Cancelled, - match=rf"^cancelled due to nursery with reason 'child task raised exception ValueError\(\)' from task {fail_task!r}$", - ): - await trio.sleep_forever() - raise TypeError - with RaisesGroup(ValueError, TypeError): async with trio.open_nursery() as nursery: fail_task = await nursery.start(failing_task) + with pytest.raises(Cancelled, match=match_str.format(fail_task)): + await wait_all_tasks_blocked() await nursery.start(cancelled_task, fail_task) +# wait until both tasks are running before failing_task raises async def test_cancel_reason_nursery2() -> None: - async def failing_task( - event: trio.Event, task_status: trio.TaskStatus[trio.lowlevel.Task] - ) -> None: + async def failing_task(task_status: trio.TaskStatus[trio.lowlevel.Task]) -> None: task_status.started(current_task()) - await event.wait() + await wait_all_tasks_blocked() + raise ValueError + + with RaisesGroup(ValueError, TypeError): + async with trio.open_nursery() as nursery: + fail_task = await nursery.start(failing_task) + await nursery.start(cancelled_task, fail_task) + + +# failing_task raises before calling task_status.started() +async def test_cancel_reason_nursery3() -> None: + async def failing_task(task_status: trio.TaskStatus[None]) -> None: raise ValueError - async def cancelled_task( - fail_task: trio.lowlevel.Task, task_status: trio.TaskStatus - ) -> None: - task_status.started() + parent_task = current_task() + + async def cancelled_task() -> None: + # We don't have a way of distinguishing that the nursery code block failed + # because it failed to `start()` a task. with pytest.raises( Cancelled, - match=rf"^cancelled due to nursery with reason 'child task raised exception ValueError\(\)' from task {fail_task!r}$", + match=re.escape( + rf"cancelled due to nursery with reason 'Code block inside nursery contextmanager raised exception ValueError()' from task {parent_task!r}" + ), ): await trio.sleep_forever() - raise TypeError - with RaisesGroup(ValueError, TypeError): + with RaisesGroup(ValueError): async with trio.open_nursery() as nursery: - event = trio.Event() - fail_task = await nursery.start(failing_task, event) - await nursery.start(cancelled_task, fail_task) - event.set() + nursery.start_soon(cancelled_task) + await wait_all_tasks_blocked() + await nursery.start(failing_task) async def test_cancel_reason_not_overwritten() -> None: diff --git a/src/trio/_core/_tests/test_run.py b/src/trio/_core/_tests/test_run.py index d68418c4ee..24693eab89 100644 --- a/src/trio/_core/_tests/test_run.py +++ b/src/trio/_core/_tests/test_run.py @@ -3,8 +3,6 @@ import contextvars import functools import gc -import pickle -import re import sys import threading import time @@ -2219,59 +2217,6 @@ def test_Nursery_subclass() -> None: type("Subclass", (_core._run.Nursery,), {}) -def test_Cancelled_init() -> None: - with pytest.raises(TypeError, match=r"^trio.Cancelled has no public constructor$"): - raise _core.Cancelled # type: ignore[call-arg] - - with pytest.raises(TypeError, match=r"^trio.Cancelled has no public constructor$"): - _core.Cancelled(source="explicit") - - # private constructor should not raise - _core.Cancelled._create(source="explicit") - - -async def test_Cancelled_str() -> None: - cancelled = _core.Cancelled._create(source="explicit") - assert str(cancelled) == "cancelled due to explicit" - assert re.fullmatch( - r"cancelled due to deadline from task " - r"", - str( - _core.Cancelled._create( - source="deadline", - source_task=repr(_core.current_task()), - ) - ), - ) - - assert re.fullmatch( - r"cancelled due to nursery with reason 'pigs flying' from task " - r"", - str( - _core.Cancelled._create( - source="nursery", - source_task=repr(_core.current_task()), - reason="pigs flying", - ) - ), - ) - - -def test_Cancelled_subclass() -> None: - with pytest.raises(TypeError): - type("Subclass", (_core.Cancelled,), {}) - - -# https://github.com/python-trio/trio/issues/3248 -def test_Cancelled_pickle() -> None: - cancelled = _core.Cancelled._create(source="KeyboardInterrupt") - pickled_cancelled = pickle.loads(pickle.dumps(cancelled)) - assert isinstance(pickled_cancelled, _core.Cancelled) - assert cancelled.source == pickled_cancelled.source - assert cancelled.source_task == pickled_cancelled.source_task - assert cancelled.reason == pickled_cancelled.reason - - def test_CancelScope_subclass() -> None: with pytest.raises(TypeError): type("Subclass", (_core.CancelScope,), {}) From 494428274fc290dea40bfa87b309bb5629c6cfbf Mon Sep 17 00:00:00 2001 From: jakkdl Date: Wed, 7 May 2025 12:14:30 +0200 Subject: [PATCH 11/15] use a5rocks better implementation of raise_cancel. remove copy.copy. undo accidental -inf => inf flip --- src/trio/_core/_run.py | 33 +++++++++++++-------------------- src/trio/_threads.py | 3 +-- 2 files changed, 14 insertions(+), 22 deletions(-) diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index 541e1f4fd0..8fc74b26b4 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -1648,7 +1648,18 @@ def _attempt_delivery_of_any_pending_cancel(self) -> None: if not self._cancel_status.effectively_cancelled: return - self._attempt_abort(RaiseCancel(self._cancel_status._scope._cancel_reason)) + def raise_cancel() -> NoReturn: + reason = self._cancel_status._scope._cancel_reason + if reason is None: + raise Cancelled._create(source="unknown", reason="misnesting") + + raise Cancelled._create( + source=reason.source, + reason=reason.reason, + source_task=reason.source_task, + ) + + self._attempt_abort(raise_cancel) def _attempt_delivery_of_pending_ki(self) -> None: assert self._runner.ki_pending @@ -1662,24 +1673,6 @@ def raise_cancel() -> NoReturn: self._attempt_abort(raise_cancel) -class RaiseCancel: - def __init__(self, reason: CancelReason | None) -> None: - if reason is None: - self.cancelled = Cancelled._create(source="unknown", reason="misnesting") - else: - self.cancelled = Cancelled._create( - source=reason.source, - reason=reason.reason, - source_task=reason.source_task, - ) - - def __call__(self) -> NoReturn: - try: - raise self.cancelled - finally: - del self.cancelled - - ################################################################ # The central Runner object ################################################################ @@ -3026,7 +3019,7 @@ async def checkpoint() -> None: if task._cancel_status.effectively_cancelled or ( task is task._runner.main_task and task._runner.ki_pending ): - cs = CancelScope(deadline=inf) + cs = CancelScope(deadline=-inf) if ( task._cancel_status._scope._cancel_reason is None and task is task._runner.main_task diff --git a/src/trio/_threads.py b/src/trio/_threads.py index e8046ba860..4b1e54f540 100644 --- a/src/trio/_threads.py +++ b/src/trio/_threads.py @@ -2,7 +2,6 @@ import contextlib import contextvars -import copy import inspect import queue as stdlib_queue import threading @@ -433,7 +432,7 @@ def abort(raise_cancel: RaiseCancelT) -> trio.lowlevel.Abort: # fill so from_thread_check_cancelled can raise # 'raise_cancel' will immediately delete its reason object, so we make # a copy in each thread - cancel_register[0] = copy.copy(raise_cancel) + cancel_register[0] = raise_cancel if abandon_bool: # empty so report_back_in_trio_thread_fn cannot reschedule task_register[0] = None From 67b1370961ed380afb2cb75e035874f14c8a2978 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Fri, 9 May 2025 11:56:53 +0200 Subject: [PATCH 12/15] revert RaiseCancel change, improve tests --- src/trio/_core/_run.py | 31 ++++++----- src/trio/_tests/test_threads.py | 94 ++++++++++++++++++--------------- src/trio/_threads.py | 3 +- 3 files changed, 73 insertions(+), 55 deletions(-) diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index 8fc74b26b4..ab0096ffb1 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -1648,18 +1648,7 @@ def _attempt_delivery_of_any_pending_cancel(self) -> None: if not self._cancel_status.effectively_cancelled: return - def raise_cancel() -> NoReturn: - reason = self._cancel_status._scope._cancel_reason - if reason is None: - raise Cancelled._create(source="unknown", reason="misnesting") - - raise Cancelled._create( - source=reason.source, - reason=reason.reason, - source_task=reason.source_task, - ) - - self._attempt_abort(raise_cancel) + self._attempt_abort(RaiseCancel(self._cancel_status._scope._cancel_reason)) def _attempt_delivery_of_pending_ki(self) -> None: assert self._runner.ki_pending @@ -1673,6 +1662,24 @@ def raise_cancel() -> NoReturn: self._attempt_abort(raise_cancel) +class RaiseCancel: + def __init__(self, reason: CancelReason | None) -> None: + if reason is None: + self.cancelled = Cancelled._create(source="unknown", reason="misnesting") + else: + self.cancelled = Cancelled._create( + source=reason.source, + reason=reason.reason, + source_task=reason.source_task, + ) + + def __call__(self) -> NoReturn: + try: + raise self.cancelled + finally: + del self.cancelled + + ################################################################ # The central Runner object ################################################################ diff --git a/src/trio/_tests/test_threads.py b/src/trio/_tests/test_threads.py index 7b29ecdb32..ba2c366faa 100644 --- a/src/trio/_tests/test_threads.py +++ b/src/trio/_tests/test_threads.py @@ -993,19 +993,26 @@ async def async_time_bomb() -> None: assert cancel_scope.cancelled_caught -async def test_from_thread_check_cancelled() -> None: - q: stdlib_queue.Queue[str | BaseException] = stdlib_queue.Queue() +async def child( + abandon_on_cancel: bool, + scope: CancelScope, + record: list[str], + f: Callable[[], None], +) -> None: + with scope: + record.append("start") + try: + return await to_thread_run_sync(f, abandon_on_cancel=abandon_on_cancel) + except _core.Cancelled as e: + record.append(str(e)) + raise + finally: + record.append("exit") - async def child(abandon_on_cancel: bool, scope: CancelScope) -> None: - with scope: - record.append("start") - try: - return await to_thread_run_sync(f, abandon_on_cancel=abandon_on_cancel) - except _core.Cancelled as e: - record.append(str(e)) - raise - finally: - record.append("exit") + +@pytest.mark.parametrize("cancel_the_scope", [False, True]) +async def test_from_thread_check_cancelled_no_abandon(cancel_the_scope: bool) -> None: + q: stdlib_queue.Queue[str | BaseException] = stdlib_queue.Queue() def f() -> None: try: @@ -1017,42 +1024,42 @@ def f() -> None: ev.wait() return from_thread_check_cancelled() - # Base case: nothing cancelled so we shouldn't see cancels anywhere record: list[str] = [] ev = threading.Event() - async with _core.open_nursery() as nursery: - nursery.start_soon(child, False, _core.CancelScope()) - await wait_all_tasks_blocked() - assert record[0] == "start" - assert q.get(timeout=1) == "Not Cancelled" - ev.set() - # implicit assertion, Cancelled not raised via nursery - assert record[1] == "exit" - - # abandon_on_cancel=False case: a cancel will pop out but be handled by - # the appropriate cancel scope - record = [] - ev = threading.Event() scope = _core.CancelScope() # Nursery cancel scope gives false positives + async with _core.open_nursery() as nursery: - nursery.start_soon(child, False, scope) + nursery.start_soon(child, False, scope, record, f) await wait_all_tasks_blocked() assert record[0] == "start" assert q.get(timeout=1) == "Not Cancelled" - scope.cancel() + if cancel_the_scope: + scope.cancel() ev.set() - assert scope.cancelled_caught - assert re.fullmatch( - r"cancelled due to explicit from task " - r"", - record[1], - ), record[1] - assert record[2] == "exit" - assert len(record) == 3 + # Base case: nothing cancelled so we shouldn't see cancels anywhere + if not cancel_the_scope: + # implicit assertion, Cancelled not raised via nursery + assert record[1] == "exit" + else: + # abandon_on_cancel=False case: a cancel will pop out but be handled by + # the appropriate cancel scope + + assert scope.cancelled_caught + assert re.fullmatch( + r"cancelled due to explicit from task " + r"", + record[1], + ), record[1] + assert record[2] == "exit" + assert len(record) == 3 + +async def test_from_thread_check_cancelled_abandon_on_cancel() -> None: + q: stdlib_queue.Queue[str | BaseException] = stdlib_queue.Queue() # abandon_on_cancel=True case: slightly different thread behavior needed # check thread is cancelled "soon" after abandonment - def f() -> None: # type: ignore[no-redef] # noqa: F811 + + def f() -> None: ev.wait() try: from_thread_check_cancelled() @@ -1065,19 +1072,22 @@ def f() -> None: # type: ignore[no-redef] # noqa: F811 else: # pragma: no cover, test failure path q.put("Not Cancelled") - record = [] + record: list[str] = [] ev = threading.Event() scope = _core.CancelScope() async with _core.open_nursery() as nursery: - nursery.start_soon(child, True, scope) + nursery.start_soon(child, True, scope, record, f) await wait_all_tasks_blocked() assert record[0] == "start" scope.cancel() - ev.set() + # In the worst case the nursery fully exits before the threaded function + # checks for cancellation. + ev.set() + assert scope.cancelled_caught assert re.fullmatch( r"cancelled due to explicit from task " - r"", + r"", record[1], ), record[1] assert record[-1] == "exit" @@ -1087,7 +1097,7 @@ def f() -> None: # type: ignore[no-redef] # noqa: F811 else: assert re.fullmatch( r"cancelled due to explicit from task " - r"", + r"", res, ), res diff --git a/src/trio/_threads.py b/src/trio/_threads.py index 4b1e54f540..e8046ba860 100644 --- a/src/trio/_threads.py +++ b/src/trio/_threads.py @@ -2,6 +2,7 @@ import contextlib import contextvars +import copy import inspect import queue as stdlib_queue import threading @@ -432,7 +433,7 @@ def abort(raise_cancel: RaiseCancelT) -> trio.lowlevel.Abort: # fill so from_thread_check_cancelled can raise # 'raise_cancel' will immediately delete its reason object, so we make # a copy in each thread - cancel_register[0] = raise_cancel + cancel_register[0] = copy.copy(raise_cancel) if abandon_bool: # empty so report_back_in_trio_thread_fn cannot reschedule task_register[0] = None From bcf001413b1159b7e6a2edb4c98103f5290e27b1 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Mon, 12 May 2025 11:25:17 +0200 Subject: [PATCH 13/15] move logic out of RaiseCancel --- src/trio/_core/_run.py | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index ab0096ffb1..53eab108b1 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -1648,7 +1648,19 @@ def _attempt_delivery_of_any_pending_cancel(self) -> None: if not self._cancel_status.effectively_cancelled: return - self._attempt_abort(RaiseCancel(self._cancel_status._scope._cancel_reason)) + if (reason := self._cancel_status._scope._cancel_reason) is not None: + cancelled = Cancelled._create( + source=reason.source, + reason=reason.reason, + source_task=reason.source_task, + ) + else: + cancelled = Cancelled._create(source="unknown", reason="misnesting") + + self._attempt_abort(RaiseCancel(cancelled)) + # Clear reference to pass gc tests. `RaiseCancel` keeps the `Cancelled` + # alive until it's used. + del cancelled def _attempt_delivery_of_pending_ki(self) -> None: assert self._runner.ki_pending @@ -1663,15 +1675,8 @@ def raise_cancel() -> NoReturn: class RaiseCancel: - def __init__(self, reason: CancelReason | None) -> None: - if reason is None: - self.cancelled = Cancelled._create(source="unknown", reason="misnesting") - else: - self.cancelled = Cancelled._create( - source=reason.source, - reason=reason.reason, - source_task=reason.source_task, - ) + def __init__(self, cancelled: Cancelled) -> None: + self.cancelled = cancelled def __call__(self) -> NoReturn: try: From 680306fab5d505ac8243f6105dd9d42141a2183f Mon Sep 17 00:00:00 2001 From: jakkdl Date: Thu, 15 May 2025 12:44:26 +0200 Subject: [PATCH 14/15] a5rocks version --- src/trio/_core/_run.py | 35 ++++++++++++----------------------- 1 file changed, 12 insertions(+), 23 deletions(-) diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index 53eab108b1..5fff0b772f 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -1648,19 +1648,19 @@ def _attempt_delivery_of_any_pending_cancel(self) -> None: if not self._cancel_status.effectively_cancelled: return - if (reason := self._cancel_status._scope._cancel_reason) is not None: - cancelled = Cancelled._create( - source=reason.source, - reason=reason.reason, - source_task=reason.source_task, - ) - else: - cancelled = Cancelled._create(source="unknown", reason="misnesting") + reason = self._cancel_status._scope._cancel_reason + + def raise_cancel() -> NoReturn: + if reason is None: + raise Cancelled._create(source="unknown", reason="misnesting") + else: + raise Cancelled._create( + source=reason.source, + reason=reason.reason, + source_task=reason.source_task, + ) - self._attempt_abort(RaiseCancel(cancelled)) - # Clear reference to pass gc tests. `RaiseCancel` keeps the `Cancelled` - # alive until it's used. - del cancelled + self._attempt_abort(raise_cancel) def _attempt_delivery_of_pending_ki(self) -> None: assert self._runner.ki_pending @@ -1674,17 +1674,6 @@ def raise_cancel() -> NoReturn: self._attempt_abort(raise_cancel) -class RaiseCancel: - def __init__(self, cancelled: Cancelled) -> None: - self.cancelled = cancelled - - def __call__(self) -> NoReturn: - try: - raise self.cancelled - finally: - del self.cancelled - - ################################################################ # The central Runner object ################################################################ From 2fe4fca3f50c0182a36116664694f1372ae55af8 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Thu, 15 May 2025 12:47:50 +0200 Subject: [PATCH 15/15] remove copy --- src/trio/_threads.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/trio/_threads.py b/src/trio/_threads.py index e8046ba860..4b1e54f540 100644 --- a/src/trio/_threads.py +++ b/src/trio/_threads.py @@ -2,7 +2,6 @@ import contextlib import contextvars -import copy import inspect import queue as stdlib_queue import threading @@ -433,7 +432,7 @@ def abort(raise_cancel: RaiseCancelT) -> trio.lowlevel.Abort: # fill so from_thread_check_cancelled can raise # 'raise_cancel' will immediately delete its reason object, so we make # a copy in each thread - cancel_register[0] = copy.copy(raise_cancel) + cancel_register[0] = raise_cancel if abandon_bool: # empty so report_back_in_trio_thread_fn cannot reschedule task_register[0] = None