revert RaiseCancel change, improve tests

Author: jakkdl

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
 ################################################################

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"<Task 'trio._tests.test_threads.test_from_thread_check_cancelled' at 0x\w*>",
-        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"<Task 'trio._tests.test_threads.test_from_thread_check_cancelled_no_abandon' at 0x\w*>",
+            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"<Task 'trio._tests.test_threads.test_from_thread_check_cancelled' at 0x\w*>",
+        r"<Task 'trio._tests.test_threads.test_from_thread_check_cancelled_abandon_on_cancel' at 0x\w*>",
         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"<Task 'trio._tests.test_threads.test_from_thread_check_cancelled' at 0x\w*>",
+            r"<Task 'trio._tests.test_threads.test_from_thread_check_cancelled_abandon_on_cancel' at 0x\w*>",
             res,
         ), res
 

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