Merge pull request #901 from oremanj/delaycancel

Delay deciding which cancel scope a Cancelled exception belongs to

newsfragments/860.bugfix.rst

@@ -0,0 +1,4 @@
+:exc:`trio.Cancelled` exceptions now always propagate until they reach
+the outermost unshielded cancelled scope, even if more cancellations
+occur or shielding is changed between when the :exc:`~trio.Cancelled`
+is delivered and when it is caught.

trio/_core/_exceptions.py

@@ -46,11 +46,9 @@ class Cancelled(BaseException):
 
     then this *won't* catch a :exc:`Cancelled` exception.
 
-    Attempting to raise :exc:`Cancelled` yourself will cause a
-    :exc:`RuntimeError`. It would not be associated with a cancel scope and thus
-    not be caught by Trio. Use
-    :meth:`cancel_scope.cancel() <trio.CancelScope.cancel>`
-    instead.
+    You cannot raise :exc:`Cancelled` yourself. Attempting to do so
+    will produce a :exc:`RuntimeError`. Use :meth:`cancel_scope.cancel()
+    <trio.CancelScope.cancel>` instead.
 
     .. note::
 
@@ -65,7 +63,6 @@ class Cancelled(BaseException):
        everywhere.
 
     """
-    _scope = None
     __marker = object()
 
     def __init__(self, _marker=None):

trio/_core/_run.py

@@ -342,23 +342,18 @@ def shield(self, new_value):
             for task in self._tasks:
                 task._attempt_delivery_of_any_pending_cancel()
 
-    def _cancel_no_notify(self):
-        # returns the affected tasks
-        if not self.cancel_called:
-            with self._might_change_effective_deadline():
-                self.cancel_called = True
-            return self._tasks
-        else:
-            return set()
-
     @enable_ki_protection
     def cancel(self):
         """Cancels this scope immediately.
 
         This method is idempotent, i.e., if the scope was already
         cancelled then this method silently does nothing.
         """
-        for task in self._cancel_no_notify():
+        if self.cancel_called:
+            return
+        with self._might_change_effective_deadline():
+            self.cancel_called = True
+        for task in self._tasks:
             task._attempt_delivery_of_any_pending_cancel()
 
     def _add_task(self, task):
@@ -379,24 +374,22 @@ def _tasks_removed_by_adoption(self, tasks):
     def _tasks_added_by_adoption(self, tasks):
         self._tasks.update(tasks)
 
-    def _make_exc(self):
-        exc = Cancelled._init()
-        exc._scope = self
-        return exc
-
     def _exc_filter(self, exc):
-        if isinstance(exc, Cancelled) and exc._scope is self:
+        if (
+            isinstance(exc, Cancelled) and self.cancel_called
+            and self._scope_task._pending_cancel_scope() is self
+        ):
             self.cancelled_caught = True
             return None
         return exc
 
     def _close(self, exc):
+        if exc is not None:
+            exc = MultiError.filter(self._exc_filter, exc)
         with self._might_change_effective_deadline():
             self._remove_task(self._scope_task)
         self._scope_task = None
-        if exc is not None:
-            return MultiError.filter(self._exc_filter, exc)
-        return None
+        return exc
 
 
 @deprecated("0.10.0", issue=607, instead="trio.CancelScope")
@@ -432,9 +425,8 @@ def started(self, value=None):
 
         # If the old nursery is cancelled, then quietly quit now; the child
         # will eventually exit on its own, and we don't want to risk moving
-        # the children into a different scope while they might have
-        # propagating Cancelled exceptions that assume they're under the old
-        # scope.
+        # children that might have propagating Cancelled exceptions into
+        # a place with no cancelled cancel scopes to catch them.
         if _pending_cancel_scope(self._old_nursery._cancel_stack) is not None:
             return
 
@@ -758,13 +750,11 @@ def _attempt_abort(self, raise_cancel):
     def _attempt_delivery_of_any_pending_cancel(self):
         if self._abort_func is None:
             return
-        pending_scope = self._pending_cancel_scope()
-        if pending_scope is None:
+        if self._pending_cancel_scope() is None:
             return
-        exc = pending_scope._make_exc()
 
         def raise_cancel():
-            raise exc
+            raise Cancelled._init()
 
         self._attempt_abort(raise_cancel)
 
@@ -1523,21 +1513,16 @@ def run_impl(runner, async_fn, args):
         if runner.instruments:
             runner.instrument("after_io_wait", timeout)
 
+        # Process cancellations due to deadline expiry
         now = runner.clock.current_time()
-        # We process all timeouts in a batch and then notify tasks at the end
-        # to ensure that if multiple timeouts occur at once, then it's the
-        # outermost one that gets delivered.
-        cancelled_tasks = set()
         while runner.deadlines:
             (deadline, _), cancel_scope = runner.deadlines.peekitem(0)
             if deadline <= now:
                 # This removes the given scope from runner.deadlines:
-                cancelled_tasks.update(cancel_scope._cancel_no_notify())
+                cancel_scope.cancel()
                 idle_primed = False
             else:
                 break
-        for task in cancelled_tasks:
-            task._attempt_delivery_of_any_pending_cancel()
 
         if not runner.runq and idle_primed:
             while runner.waiting_for_idle:

trio/_core/tests/test_run.py

@@ -637,21 +637,21 @@ async def crasher():
                     nursery.start_soon(crasher)  # t4
                 # and then our __aexit__ also receives an outer Cancelled
             except _core.MultiError as multi_exc:
-                # This is outside the nursery scope but inside the outer
-                # scope, so the nursery should have absorbed t1 and t2's
-                # exceptions but t3 and t4 should remain, plus the Cancelled
-                # from 'outer'
-                assert len(multi_exc.exceptions) == 3
+                # Since the outer scope became cancelled before the
+                # nursery block exited, all cancellations inside the
+                # nursery block continue propagating to reach the
+                # outer scope.
+                assert len(multi_exc.exceptions) == 5
                 summary = {}
                 for exc in multi_exc.exceptions:
                     summary.setdefault(type(exc), 0)
                     summary[type(exc)] += 1
-                assert summary == {_core.Cancelled: 2, KeyError: 1}
+                assert summary == {_core.Cancelled: 4, KeyError: 1}
                 raise
     except AssertionError:  # pragma: no cover
         raise
     except BaseException as exc:
-        # This is ouside the outer scope, so the two outer Cancelled
+        # This is ouside the outer scope, so all the Cancelled
         # exceptions should have been absorbed, leaving just a regular
         # KeyError from crasher()
         assert type(exc) is KeyError
@@ -1400,7 +1400,7 @@ async def main():
     assert len(record) == 2
     assert record[0] == "starting"
     assert record[1][0] == "run finished"
-    assert record[1][1] >= 20
+    assert record[1][1] >= 19
 
 
 def test_TrioToken_run_sync_soon_threaded_stress_test():

trio/tests/test_highlevel_open_tcp_stream.py

@@ -397,9 +397,11 @@ async def test_multi_success(autojump_clock):
         happy_eyeballs_delay=1,
     )
     assert not scenario.sockets["1.1.1.1"].succeeded
-    assert scenario.sockets["2.2.2.2"].succeeded
-    assert scenario.sockets["3.3.3.3"].succeeded
-    assert scenario.sockets["4.4.4.4"].succeeded
+    assert (
+        scenario.sockets["2.2.2.2"].succeeded
+        or scenario.sockets["3.3.3.3"].succeeded
+        or scenario.sockets["4.4.4.4"].succeeded
+    )
     assert not scenario.sockets["5.5.5.5"].succeeded
     assert sock.ip in ["2.2.2.2", "3.3.3.3", "4.4.4.4"]
     assert trio.current_time() == (0.5 + 10)