Merge pull request #2845 from gschaffner/fix-start-nursery-implementation-detail-hiding

Fix regressions introduced when hiding `Nursery.start` implementation-detail nursery

Author: Zac-HD

newsfragments/2611.bugfix.rst

@@ -1 +1,5 @@
-With ``strict_exception_groups=True``, when you ran a function in a nursery which raised an exception before calling ``task_status.started()``, it previously got wrapped twice over in ``ExceptionGroup`` in some cases. It no longer does that, and also won't wrap any ``ExceptionGroup`` raised by the function itself.
+When a starting function raises before calling :func:`trio.TaskStatus.started`,
+:func:`trio.Nursery.start` will no longer wrap the exception in an undocumented
+:exc:`ExceptionGroup`. Previously, :func:`trio.Nursery.start` would incorrectly
+raise an :exc:`ExceptionGroup` containing it when using ``trio.run(...,
+strict_exception_groups=True)``.

trio/_core/_run.py

@@ -1103,13 +1103,6 @@ def aborted(raise_cancel: _core.RaiseCancelT) -> Abort:
 
         popped = self._parent_task._child_nurseries.pop()
         assert popped is self
-
-        # don't unnecessarily wrap an exceptiongroup in another exceptiongroup
-        # see https://github.com/python-trio/trio/issues/2611
-        if len(self._pending_excs) == 1 and isinstance(
-            self._pending_excs[0], BaseExceptionGroup
-        ):
-            return self._pending_excs[0]
         if self._pending_excs:
             try:
                 return MultiError(
@@ -1218,7 +1211,11 @@ async def async_fn(arg1, arg2, *, task_status=trio.TASK_STATUS_IGNORED):
             raise RuntimeError("Nursery is closed to new arrivals")
         try:
             self._pending_starts += 1
-            async with open_nursery() as old_nursery:
+            # `strict_exception_groups=False` prevents the implementation-detail
+            # nursery from inheriting `strict_exception_groups=True` from the
+            # `run` option, which would cause it to wrap a pre-started()
+            # exception in an extra ExceptionGroup. See #2611.
+            async with open_nursery(strict_exception_groups=False) as old_nursery:
                 task_status: _TaskStatus[StatusT] = _TaskStatus(old_nursery, self)
                 thunk = functools.partial(async_fn, task_status=task_status)
                 task = GLOBAL_RUN_CONTEXT.runner.spawn_impl(

trio/_core/_tests/test_run.py

@@ -2534,60 +2534,73 @@ async def test_cancel_scope_no_cancellederror() -> None:
     assert not scope.cancelled_caught
 
 
-"""These tests are for fixing https://github.com/python-trio/trio/issues/2611
-where exceptions raised before `task_status.started()` got wrapped twice.
-"""
-
-
-async def raise_before(*, task_status: _core.TaskStatus[None]) -> None:
-    raise ValueError
-
-
-async def raise_after_started(*, task_status: _core.TaskStatus[None]) -> None:
-    task_status.started()
-    raise ValueError
-
-
-async def raise_custom_exception_group_before(
-    *, task_status: _core.TaskStatus[None]
+@pytest.mark.parametrize("run_strict", [False, True])
+@pytest.mark.parametrize("start_raiser_strict", [False, True, None])
+@pytest.mark.parametrize("raise_after_started", [False, True])
+@pytest.mark.parametrize("raise_custom_exc_grp", [False, True])
+def test_trio_run_strict_before_started(
+    run_strict: bool,
+    start_raiser_strict: bool | None,
+    raise_after_started: bool,
+    raise_custom_exc_grp: bool,
 ) -> None:
-    raise ExceptionGroup("my group", [ValueError()])
-
-
-def _check_exception(exc: pytest.ExceptionInfo[BaseException]) -> None:
-    assert isinstance(exc.value, BaseExceptionGroup)
-    assert len(exc.value.exceptions) == 1
-    assert isinstance(exc.value.exceptions[0], ValueError)
-
+    """
+    Regression tests for #2611, where exceptions raised before
+    `TaskStatus.started()` caused `Nursery.start()` to wrap them in an
+    ExceptionGroup when using `run(..., strict_exception_groups=True)`.
 
-async def _start_raiser(
-    raiser: Callable[[], Awaitable[None]], strict: bool | None = None
-) -> None:
-    async with _core.open_nursery(strict_exception_groups=strict) as nursery:
-        await nursery.start(raiser)
+    Regression tests for #2844, where #2611 was initially fixed in a way that
+    had unintended side effects.
+    """
 
+    raiser_exc: ValueError | ExceptionGroup[ValueError]
+    if raise_custom_exc_grp:
+        raiser_exc = ExceptionGroup("my group", [ValueError()])
+    else:
+        raiser_exc = ValueError()
 
-@pytest.mark.parametrize("strict", [False, True])
-@pytest.mark.parametrize("raiser", [raise_before, raise_after_started])
-async def test_strict_before_started(
-    strict: bool, raiser: Callable[[], Awaitable[None]]
-) -> None:
-    with pytest.raises(BaseExceptionGroup if strict else ValueError) as exc:
-        await _start_raiser(raiser, strict)
-    if strict:
-        _check_exception(exc)
+    async def raiser(*, task_status: _core.TaskStatus[None]) -> None:
+        if raise_after_started:
+            task_status.started()
+        raise raiser_exc
 
+    async def start_raiser() -> None:
+        try:
+            async with _core.open_nursery(
+                strict_exception_groups=start_raiser_strict
+            ) as nursery:
+                await nursery.start(raiser)
+        except BaseExceptionGroup as exc_group:
+            if start_raiser_strict:
+                # Iff the code using the nursery *forced* it to be strict
+                # (overriding the runner setting) then it may replace the bland
+                # exception group raised by trio with a more specific one (subtype,
+                # different message, etc.).
+                raise BaseExceptionGroup(
+                    "start_raiser nursery custom message", exc_group.exceptions
+                ) from None
+            raise
 
-# it was only when run from `trio.run` that the double wrapping happened
-@pytest.mark.parametrize("strict", [False, True])
-@pytest.mark.parametrize(
-    "raiser", [raise_before, raise_after_started, raise_custom_exception_group_before]
-)
-def test_trio_run_strict_before_started(
-    strict: bool, raiser: Callable[[], Awaitable[None]]
-) -> None:
-    expect_group = strict or raiser is raise_custom_exception_group_before
-    with pytest.raises(BaseExceptionGroup if expect_group else ValueError) as exc:
-        _core.run(_start_raiser, raiser, strict_exception_groups=strict)
-    if expect_group:
-        _check_exception(exc)
+    with pytest.raises(BaseException) as exc_info:
+        _core.run(start_raiser, strict_exception_groups=run_strict)
+
+    if start_raiser_strict or (run_strict and start_raiser_strict is None):
+        # start_raiser's nursery was strict.
+        assert isinstance(exc_info.value, BaseExceptionGroup)
+        if start_raiser_strict:
+            # start_raiser didn't unknowingly inherit its nursery strictness
+            # from `run`---it explicitly chose for its nursery to be strict.
+            assert exc_info.value.message == "start_raiser nursery custom message"
+        assert len(exc_info.value.exceptions) == 1
+        should_be_raiser_exc = exc_info.value.exceptions[0]
+    else:
+        # start_raiser's nursery was not strict.
+        should_be_raiser_exc = exc_info.value
+    if isinstance(raiser_exc, ValueError):
+        assert should_be_raiser_exc is raiser_exc
+    else:
+        # Check attributes, not identity, because should_be_raiser_exc may be a
+        # copy of raiser_exc rather than raiser_exc by identity.
+        assert type(should_be_raiser_exc) == type(raiser_exc)
+        assert should_be_raiser_exc.message == raiser_exc.message
+        assert should_be_raiser_exc.exceptions == raiser_exc.exceptions