From 80d492a8f524e1adb5224e7d2c9f362076ec766e Mon Sep 17 00:00:00 2001 From: jakkdl Date: Mon, 23 Oct 2023 12:47:23 +0200 Subject: [PATCH 1/6] add test --- trio/_tests/test_extra_wrap.py | 38 ++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 trio/_tests/test_extra_wrap.py diff --git a/trio/_tests/test_extra_wrap.py b/trio/_tests/test_extra_wrap.py new file mode 100644 index 0000000000..ae86e02395 --- /dev/null +++ b/trio/_tests/test_extra_wrap.py @@ -0,0 +1,38 @@ +import pytest + +import trio +from trio import TaskStatus + + +async def startable_fn_which_doesnt_contain_a_nursery( + raise_before: bool, *, task_status: TaskStatus[None] +) -> None: + if raise_before: + print("Something went wrong") + raise RuntimeError # this might be wrapped in an ExceptionGroup + task_status.started() + raise ValueError # ...but this will never be wrapped! + + +async def test_strict_before_started() -> None: + with pytest.raises(RuntimeError): + async with trio.open_nursery(strict_exception_groups=True) as nursery: + await nursery.start(startable_fn_which_doesnt_contain_a_nursery, True) + + +async def test_no_strict_before_started() -> None: + with pytest.raises(RuntimeError): + async with trio.open_nursery(strict_exception_groups=False) as nursery: + await nursery.start(startable_fn_which_doesnt_contain_a_nursery, True) + + +async def test_strict_after_started() -> None: + with pytest.raises(ValueError): + async with trio.open_nursery(strict_exception_groups=True) as nursery: + await nursery.start(startable_fn_which_doesnt_contain_a_nursery, False) + + +async def test_no_strict_after_started() -> None: + with pytest.raises(ValueError): + async with trio.open_nursery(strict_exception_groups=False) as nursery: + await nursery.start(startable_fn_which_doesnt_contain_a_nursery, False) From 209267a506fde14c6e011a58dc2afd2fe54bc7c5 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Mon, 23 Oct 2023 13:39:28 +0200 Subject: [PATCH 2/6] Fix unnecessary wrapping of exception groups --- trio/_core/_run.py | 7 +++ trio/_tests/test_extra_wrap.py | 79 ++++++++++++++++++++++++++++------ 2 files changed, 72 insertions(+), 14 deletions(-) diff --git a/trio/_core/_run.py b/trio/_core/_run.py index 2ee73868e5..18abafba81 100644 --- a/trio/_core/_run.py +++ b/trio/_core/_run.py @@ -1103,6 +1103,13 @@ 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( diff --git a/trio/_tests/test_extra_wrap.py b/trio/_tests/test_extra_wrap.py index ae86e02395..9b9ea50070 100644 --- a/trio/_tests/test_extra_wrap.py +++ b/trio/_tests/test_extra_wrap.py @@ -1,38 +1,89 @@ +"""These tests are for fixing https://github.com/python-trio/trio/issues/2611""" +import sys + import pytest import trio from trio import TaskStatus +if sys.version_info < (3, 11): + from exceptiongroup import BaseExceptionGroup + + +async def raise_before(*, task_status: TaskStatus[None]) -> None: + raise ValueError -async def startable_fn_which_doesnt_contain_a_nursery( - raise_before: bool, *, task_status: TaskStatus[None] -) -> None: - if raise_before: - print("Something went wrong") - raise RuntimeError # this might be wrapped in an ExceptionGroup + +async def raise_after_started(*, task_status: TaskStatus[None]) -> None: task_status.started() - raise ValueError # ...but this will never be wrapped! + raise ValueError async def test_strict_before_started() -> None: - with pytest.raises(RuntimeError): + with pytest.raises(BaseExceptionGroup) as exc: async with trio.open_nursery(strict_exception_groups=True) as nursery: - await nursery.start(startable_fn_which_doesnt_contain_a_nursery, True) + await nursery.start(raise_before) + assert len(exc.value.exceptions) == 1 + assert isinstance(exc.value.exceptions[0], ValueError) async def test_no_strict_before_started() -> None: - with pytest.raises(RuntimeError): + with pytest.raises(ValueError): async with trio.open_nursery(strict_exception_groups=False) as nursery: - await nursery.start(startable_fn_which_doesnt_contain_a_nursery, True) + await nursery.start(raise_before) async def test_strict_after_started() -> None: - with pytest.raises(ValueError): + with pytest.raises(BaseExceptionGroup) as exc: async with trio.open_nursery(strict_exception_groups=True) as nursery: - await nursery.start(startable_fn_which_doesnt_contain_a_nursery, False) + await nursery.start(raise_after_started) + assert len(exc.value.exceptions) == 1 + assert isinstance(exc.value.exceptions[0], ValueError) async def test_no_strict_after_started() -> None: with pytest.raises(ValueError): async with trio.open_nursery(strict_exception_groups=False) as nursery: - await nursery.start(startable_fn_which_doesnt_contain_a_nursery, False) + await nursery.start(raise_after_started) + + +# this is the only test that didn't work before +# I created the others just to check my assumptions and help figuring stuff out +def test_trio_run_strict_before_started() -> None: + async def main() -> None: + async with trio.open_nursery() as nursery: + await nursery.start(raise_before) + + with pytest.raises(BaseExceptionGroup) as exc: + trio.run(main, strict_exception_groups=True) + assert len(exc.value.exceptions) == 1 + assert isinstance(exc.value.exceptions[0], ValueError) + + +def test_trio_run_strict_after_started() -> None: + async def main() -> None: + async with trio.open_nursery() as nursery: + await nursery.start(raise_after_started) + + with pytest.raises(BaseExceptionGroup) as exc: + trio.run(main, strict_exception_groups=True) + assert len(exc.value.exceptions) == 1 + assert isinstance(exc.value.exceptions[0], ValueError) + + +def test_trio_run_no_strict_before_started() -> None: + async def main() -> None: + async with trio.open_nursery() as nursery: + await nursery.start(raise_before) + + with pytest.raises(ValueError): + trio.run(main, strict_exception_groups=False) + + +def test_trio_run_no_strict_after_started() -> None: + async def main() -> None: + async with trio.open_nursery() as nursery: + await nursery.start(raise_after_started) + + with pytest.raises(ValueError): + trio.run(main, strict_exception_groups=False) From f11810a1820ba496e3ccdd6afb322625894a21f9 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Mon, 23 Oct 2023 16:10:09 +0200 Subject: [PATCH 3/6] add test where child task raises an exception group --- trio/_tests/test_extra_wrap.py | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/trio/_tests/test_extra_wrap.py b/trio/_tests/test_extra_wrap.py index 9b9ea50070..72aa547bfc 100644 --- a/trio/_tests/test_extra_wrap.py +++ b/trio/_tests/test_extra_wrap.py @@ -7,7 +7,7 @@ from trio import TaskStatus if sys.version_info < (3, 11): - from exceptiongroup import BaseExceptionGroup + from exceptiongroup import BaseExceptionGroup, ExceptionGroup async def raise_before(*, task_status: TaskStatus[None]) -> None: @@ -19,6 +19,11 @@ async def raise_after_started(*, task_status: TaskStatus[None]) -> None: raise ValueError +async def raise_custom_exception_group_before(*, task_status: TaskStatus[None]) -> None: + raise ExceptionGroup("my group", [ValueError()]) + + +# interestingly enough, this never did a double wrap async def test_strict_before_started() -> None: with pytest.raises(BaseExceptionGroup) as exc: async with trio.open_nursery(strict_exception_groups=True) as nursery: @@ -47,8 +52,7 @@ async def test_no_strict_after_started() -> None: await nursery.start(raise_after_started) -# this is the only test that didn't work before -# I created the others just to check my assumptions and help figuring stuff out +# it was only when run from `trio.run` that the double wrapping happened def test_trio_run_strict_before_started() -> None: async def main() -> None: async with trio.open_nursery() as nursery: @@ -87,3 +91,15 @@ async def main() -> None: with pytest.raises(ValueError): trio.run(main, strict_exception_groups=False) + + +# TODO: this shouldn't doublewrap, but should it modify the exception/traceback in any way? +def test_trio_run_strict_exceptiongroup_before_started() -> None: + async def main() -> None: + async with trio.open_nursery() as nursery: + await nursery.start(raise_custom_exception_group_before) + + with pytest.raises(BaseExceptionGroup) as exc: + trio.run(main, strict_exception_groups=True) + assert len(exc.value.exceptions) == 1 + assert isinstance(exc.value.exceptions[0], ValueError) From 5d5ffa16699bb044f94f7ca89ed6a6d19da6def2 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Wed, 25 Oct 2023 16:26:44 +0200 Subject: [PATCH 4/6] refactor tests --- trio/_tests/test_extra_wrap.py | 105 +++++++++++---------------------- 1 file changed, 36 insertions(+), 69 deletions(-) diff --git a/trio/_tests/test_extra_wrap.py b/trio/_tests/test_extra_wrap.py index 72aa547bfc..0533bce1cb 100644 --- a/trio/_tests/test_extra_wrap.py +++ b/trio/_tests/test_extra_wrap.py @@ -1,5 +1,8 @@ """These tests are for fixing https://github.com/python-trio/trio/issues/2611""" +from __future__ import annotations + import sys +from typing import Awaitable, Callable import pytest @@ -19,87 +22,51 @@ async def raise_after_started(*, task_status: TaskStatus[None]) -> None: raise ValueError -async def raise_custom_exception_group_before(*, task_status: TaskStatus[None]) -> None: - raise ExceptionGroup("my group", [ValueError()]) - - -# interestingly enough, this never did a double wrap -async def test_strict_before_started() -> None: - with pytest.raises(BaseExceptionGroup) as exc: - async with trio.open_nursery(strict_exception_groups=True) as nursery: - await nursery.start(raise_before) +def _check_exception(exc: pytest.ExceptionInfo) -> None: + assert isinstance(exc.value, BaseExceptionGroup) assert len(exc.value.exceptions) == 1 assert isinstance(exc.value.exceptions[0], ValueError) -async def test_no_strict_before_started() -> None: - with pytest.raises(ValueError): - async with trio.open_nursery(strict_exception_groups=False) as nursery: - await nursery.start(raise_before) - - -async def test_strict_after_started() -> None: - with pytest.raises(BaseExceptionGroup) as exc: - async with trio.open_nursery(strict_exception_groups=True) as nursery: - await nursery.start(raise_after_started) - assert len(exc.value.exceptions) == 1 - assert isinstance(exc.value.exceptions[0], ValueError) +async def main( + raiser: Callable[[], Awaitable[None]], strict: bool | None = None +) -> None: + async with trio.open_nursery(strict_exception_groups=strict) as nursery: + await nursery.start(raiser) -async def test_no_strict_after_started() -> None: - with pytest.raises(ValueError): - async with trio.open_nursery(strict_exception_groups=False) as nursery: - await nursery.start(raise_after_started) +@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 main(raiser, strict) + if strict: + _check_exception(exc) # it was only when run from `trio.run` that the double wrapping happened -def test_trio_run_strict_before_started() -> None: - async def main() -> None: - async with trio.open_nursery() as nursery: - await nursery.start(raise_before) - - with pytest.raises(BaseExceptionGroup) as exc: - trio.run(main, strict_exception_groups=True) - assert len(exc.value.exceptions) == 1 - assert isinstance(exc.value.exceptions[0], ValueError) - - -def test_trio_run_strict_after_started() -> None: - async def main() -> None: - async with trio.open_nursery() as nursery: - await nursery.start(raise_after_started) - - with pytest.raises(BaseExceptionGroup) as exc: - trio.run(main, strict_exception_groups=True) - assert len(exc.value.exceptions) == 1 - assert isinstance(exc.value.exceptions[0], ValueError) - - -def test_trio_run_no_strict_before_started() -> None: - async def main() -> None: - async with trio.open_nursery() as nursery: - await nursery.start(raise_before) - - with pytest.raises(ValueError): - trio.run(main, strict_exception_groups=False) - - -def test_trio_run_no_strict_after_started() -> None: - async def main() -> None: - async with trio.open_nursery() as nursery: - await nursery.start(raise_after_started) - - with pytest.raises(ValueError): - trio.run(main, strict_exception_groups=False) +@pytest.mark.parametrize("strict", [False, True]) +@pytest.mark.parametrize("raiser", [raise_before, raise_after_started]) +def test_trio_run_strict_before_started( + strict: bool, raiser: Callable[[], Awaitable[None]] +) -> None: + with pytest.raises(BaseExceptionGroup if strict else ValueError) as exc: + trio.run(main, raiser, strict_exception_groups=strict) + if strict: + _check_exception(exc) # TODO: this shouldn't doublewrap, but should it modify the exception/traceback in any way? def test_trio_run_strict_exceptiongroup_before_started() -> None: - async def main() -> None: - async with trio.open_nursery() as nursery: - await nursery.start(raise_custom_exception_group_before) + async def raise_custom_exception_group_before( + *, task_status: TaskStatus[None] + ) -> None: + raise ExceptionGroup("my group", [ValueError()]) with pytest.raises(BaseExceptionGroup) as exc: - trio.run(main, strict_exception_groups=True) - assert len(exc.value.exceptions) == 1 - assert isinstance(exc.value.exceptions[0], ValueError) + trio.run( + main, raise_custom_exception_group_before, strict_exception_groups=True + ) + _check_exception(exc) From d7e99206450c20bd3482bbb6d893cb37092d203c Mon Sep 17 00:00:00 2001 From: jakkdl Date: Wed, 25 Oct 2023 16:38:44 +0200 Subject: [PATCH 5/6] move tests into _core/_tests/test_run.py --- trio/_core/_tests/test_run.py | 61 +++++++++++++++++++++++++++- trio/_tests/test_extra_wrap.py | 72 ---------------------------------- 2 files changed, 60 insertions(+), 73 deletions(-) delete mode 100644 trio/_tests/test_extra_wrap.py diff --git a/trio/_core/_tests/test_run.py b/trio/_core/_tests/test_run.py index 56691b31d5..c02ec77855 100644 --- a/trio/_core/_tests/test_run.py +++ b/trio/_core/_tests/test_run.py @@ -40,7 +40,7 @@ ) if sys.version_info < (3, 11): - from exceptiongroup import ExceptionGroup + from exceptiongroup import BaseExceptionGroup, ExceptionGroup T = TypeVar("T") @@ -2542,3 +2542,62 @@ async def test_cancel_scope_no_cancellederror() -> None: raise ExceptionGroup("test", [RuntimeError(), RuntimeError()]) 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] +) -> None: + raise ExceptionGroup("my group", [ValueError()]) + + +def _check_exception(exc: pytest.ExceptionInfo) -> None: + assert isinstance(exc.value, BaseExceptionGroup) + assert len(exc.value.exceptions) == 1 + assert isinstance(exc.value.exceptions[0], ValueError) + + +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) + + +@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) + + +# 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) diff --git a/trio/_tests/test_extra_wrap.py b/trio/_tests/test_extra_wrap.py deleted file mode 100644 index 0533bce1cb..0000000000 --- a/trio/_tests/test_extra_wrap.py +++ /dev/null @@ -1,72 +0,0 @@ -"""These tests are for fixing https://github.com/python-trio/trio/issues/2611""" -from __future__ import annotations - -import sys -from typing import Awaitable, Callable - -import pytest - -import trio -from trio import TaskStatus - -if sys.version_info < (3, 11): - from exceptiongroup import BaseExceptionGroup, ExceptionGroup - - -async def raise_before(*, task_status: TaskStatus[None]) -> None: - raise ValueError - - -async def raise_after_started(*, task_status: TaskStatus[None]) -> None: - task_status.started() - raise ValueError - - -def _check_exception(exc: pytest.ExceptionInfo) -> None: - assert isinstance(exc.value, BaseExceptionGroup) - assert len(exc.value.exceptions) == 1 - assert isinstance(exc.value.exceptions[0], ValueError) - - -async def main( - raiser: Callable[[], Awaitable[None]], strict: bool | None = None -) -> None: - async with trio.open_nursery(strict_exception_groups=strict) as nursery: - await nursery.start(raiser) - - -@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 main(raiser, strict) - if strict: - _check_exception(exc) - - -# 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]) -def test_trio_run_strict_before_started( - strict: bool, raiser: Callable[[], Awaitable[None]] -) -> None: - with pytest.raises(BaseExceptionGroup if strict else ValueError) as exc: - trio.run(main, raiser, strict_exception_groups=strict) - if strict: - _check_exception(exc) - - -# TODO: this shouldn't doublewrap, but should it modify the exception/traceback in any way? -def test_trio_run_strict_exceptiongroup_before_started() -> None: - async def raise_custom_exception_group_before( - *, task_status: TaskStatus[None] - ) -> None: - raise ExceptionGroup("my group", [ValueError()]) - - with pytest.raises(BaseExceptionGroup) as exc: - trio.run( - main, raise_custom_exception_group_before, strict_exception_groups=True - ) - _check_exception(exc) From e803320ab5d878b72d38187ca21902129230b17a Mon Sep 17 00:00:00 2001 From: John Litborn <11260241+jakkdl@users.noreply.github.com> Date: Thu, 26 Oct 2023 14:24:34 +0200 Subject: [PATCH 6/6] Update trio/_core/_tests/test_run.py Co-authored-by: Thomas Grainger --- trio/_core/_tests/test_run.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/trio/_core/_tests/test_run.py b/trio/_core/_tests/test_run.py index 175612d758..361310143e 100644 --- a/trio/_core/_tests/test_run.py +++ b/trio/_core/_tests/test_run.py @@ -2560,7 +2560,7 @@ async def raise_custom_exception_group_before( raise ExceptionGroup("my group", [ValueError()]) -def _check_exception(exc: pytest.ExceptionInfo) -> None: +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)