Merge pull request #1696 from catern/master

Nursery: don't act as a checkpoint when not running anything

newsfragments/1457.feature.rst

@@ -0,0 +1,7 @@
+When exiting a nursery block, the parent task always waits for child
+tasks to exit. This wait cannot be cancelled. However, previously, if
+you tried to cancel it, it *would* inject a `Cancelled` exception,
+even though it wasn't cancelled. Most users probably never noticed
+either way, but injecting a `Cancelled` here is not really useful, and
+in some rare cases caused confusion or problems, so Trio no longer
+does that.

trio/_core/_run.py

@@ -1080,21 +1080,24 @@ async def _nested_child_finished(
         self._check_nursery_closed()
 
         if not self._closed:
-            # If we get cancelled (or have an exception injected, like
-            # KeyboardInterrupt), then save that, but still wait until our
-            # children finish.
+            # If we have a KeyboardInterrupt injected, we want to save it in
+            # the nursery's final exceptions list. But if it's just a
+            # Cancelled, then we don't -- see gh-1457.
             def aborted(raise_cancel: _core.RaiseCancelT) -> Abort:
-                self._add_exc(capture(raise_cancel).error)
+                exn = capture(raise_cancel).error
+                if not isinstance(exn, Cancelled):
+                    self._add_exc(exn)
+                del exn  # prevent cyclic garbage creation
                 return Abort.FAILED
 
             self._parent_waiting_in_aexit = True
             await wait_task_rescheduled(aborted)
         else:
-            # Nothing to wait for, so just execute a checkpoint -- but we
-            # still need to mix any exception (e.g. from an external
-            # cancellation) in with the rest of our exceptions.
+            # Nothing to wait for, so execute a schedule point, but don't
+            # allow us to be cancelled, just like the other branch.  We
+            # still need to catch and store non-Cancelled exceptions.
             try:
-                await checkpoint()
+                await cancel_shielded_checkpoint()
             except BaseException as exc:
                 self._add_exc(exc)
 

trio/_core/_tests/test_run.py

@@ -454,12 +454,12 @@ async def crasher() -> NoReturn:
                 # nursery block exited, all cancellations inside the
                 # nursery block continue propagating to reach the
                 # outer scope.
-                assert len(multi_exc.exceptions) == 5
+                assert len(multi_exc.exceptions) == 4
                 summary: dict[type, int] = {}
                 for exc in multi_exc.exceptions:
                     summary.setdefault(type(exc), 0)
                     summary[type(exc)] += 1
-                assert summary == {_core.Cancelled: 4, KeyError: 1}
+                assert summary == {_core.Cancelled: 3, KeyError: 1}
                 raise
     except AssertionError:  # pragma: no cover
         raise
@@ -1125,6 +1125,9 @@ async def child() -> None:
     assert isinstance(excinfo.value.__context__, KeyError)
 
 
+@pytest.mark.skipif(
+    sys.version_info < (3, 6, 2), reason="https://bugs.python.org/issue29600"
+)
 async def test_nursery_exception_chaining_doesnt_make_context_loops() -> None:
     async def crasher() -> NoReturn:
         raise KeyError
@@ -1637,20 +1640,35 @@ async def test_trivial_yields() -> None:
         await _core.checkpoint_if_cancelled()
         await _core.cancel_shielded_checkpoint()
 
-    with assert_checkpoints():
+    # Weird case: opening and closing a nursery schedules, but doesn't check
+    # for cancellation (unless something inside the nursery does)
+    task = _core.current_task()
+    before_schedule_points = task._schedule_points
+    with _core.CancelScope() as cs:
+        cs.cancel()
         async with _core.open_nursery():
             pass
+    assert not cs.cancelled_caught
+    assert task._schedule_points > before_schedule_points
+
+    before_schedule_points = task._schedule_points
+
+    async def noop_with_no_checkpoint() -> None:
+        pass
+
+    with _core.CancelScope() as cs:
+        cs.cancel()
+        async with _core.open_nursery() as nursery:
+            nursery.start_soon(noop_with_no_checkpoint)
+    assert not cs.cancelled_caught
+
+    assert task._schedule_points > before_schedule_points
 
     with _core.CancelScope() as cancel_scope:
         cancel_scope.cancel()
-        with pytest.raises(MultiError) as excinfo:
+        with pytest.raises(KeyError):
             async with _core.open_nursery():
                 raise KeyError
-        assert len(excinfo.value.exceptions) == 2
-        assert {type(e) for e in excinfo.value.exceptions} == {
-            KeyError,
-            _core.Cancelled,
-        }
 
 
 async def test_nursery_start(autojump_clock: _core.MockClock) -> None:
@@ -1727,15 +1745,30 @@ async def just_started(
         task_status: _core.TaskStatus[str] = _core.TASK_STATUS_IGNORED,
     ) -> None:
         task_status.started("hi")
+        await _core.checkpoint()
 
     async with _core.open_nursery() as nursery:
         with _core.CancelScope() as cs:
             cs.cancel()
             with pytest.raises(_core.Cancelled):
                 await nursery.start(just_started)
 
-    # and if after the no-op started(), the child crashes, the error comes out
-    # of start()
+    # but if the task does not execute any checkpoints, and exits, then start()
+    # doesn't raise Cancelled, since the task completed successfully.
+    async def started_with_no_checkpoint(
+        *, task_status: _core.TaskStatus[None] = _core.TASK_STATUS_IGNORED
+    ) -> None:
+        task_status.started(None)
+
+    async with _core.open_nursery() as nursery:
+        with _core.CancelScope() as cs:
+            cs.cancel()
+            await nursery.start(started_with_no_checkpoint)
+        assert not cs.cancelled_caught
+
+    # and since starting in a cancelled context makes started() a no-op, if
+    # the child crashes after calling started(), the error can *still* come
+    # out of start()
     async def raise_keyerror_after_started(
         *, task_status: _core.TaskStatus[None] = _core.TASK_STATUS_IGNORED
     ) -> None:
@@ -1745,12 +1778,8 @@ async def raise_keyerror_after_started(
     async with _core.open_nursery() as nursery:
         with _core.CancelScope() as cs:
             cs.cancel()
-            with pytest.raises(MultiError) as excinfo2:
+            with pytest.raises(KeyError):
                 await nursery.start(raise_keyerror_after_started)
-    assert {type(e) for e in excinfo2.value.exceptions} == {
-        _core.Cancelled,
-        KeyError,
-    }
 
     # trying to start in a closed nursery raises an error immediately
     async with _core.open_nursery() as closed_nursery: