From 5ff34647d5464c1bd1f4ce14568aef3bfe9984fe Mon Sep 17 00:00:00 2001 From: jakkdl Date: Mon, 27 Nov 2023 16:43:25 +0100 Subject: [PATCH 01/20] remove multierror --- src/trio/__init__.py | 25 +- src/trio/_core/_multierror.py | 190 +--------- src/trio/_core/_run.py | 19 +- src/trio/_core/_tests/test_multierror.py | 328 +----------------- .../apport_excepthook.py | 5 +- .../simple_excepthook.py | 7 +- src/trio/_core/_tests/test_run.py | 41 ++- src/trio/_highlevel_open_tcp_stream.py | 5 +- src/trio/_tests/test_exports.py | 4 - 9 files changed, 79 insertions(+), 545 deletions(-) diff --git a/src/trio/__init__.py b/src/trio/__init__.py index 5adf14618e..f28733811b 100644 --- a/src/trio/__init__.py +++ b/src/trio/__init__.py @@ -43,10 +43,6 @@ open_nursery as open_nursery, run as run, ) -from ._core._multierror import ( - MultiError as _MultiError, - NonBaseMultiError as _NonBaseMultiError, -) from ._deprecate import TrioDeprecationWarning as TrioDeprecationWarning from ._dtls import ( DTLSChannel as DTLSChannel, @@ -118,26 +114,7 @@ _deprecate.enable_attribute_deprecations(__name__) -__deprecated_attributes__: dict[str, _deprecate.DeprecatedAttribute] = { - "MultiError": _deprecate.DeprecatedAttribute( - value=_MultiError, - version="0.22.0", - issue=2211, - instead=( - "BaseExceptionGroup (on Python 3.11 and later) or " - "exceptiongroup.BaseExceptionGroup (earlier versions)" - ), - ), - "NonBaseMultiError": _deprecate.DeprecatedAttribute( - value=_NonBaseMultiError, - version="0.22.0", - issue=2211, - instead=( - "ExceptionGroup (on Python 3.11 and later) or " - "exceptiongroup.ExceptionGroup (earlier versions)" - ), - ), -} +__deprecated_attributes__: dict[str, _deprecate.DeprecatedAttribute] = {} # Having the public path in .__module__ attributes is important for: # - exception names in printed tracebacks diff --git a/src/trio/_core/_multierror.py b/src/trio/_core/_multierror.py index f439f59c97..0ccbea7b19 100644 --- a/src/trio/_core/_multierror.py +++ b/src/trio/_core/_multierror.py @@ -2,32 +2,29 @@ import sys from types import TracebackType -from typing import TYPE_CHECKING, Any, ClassVar, cast, overload +from typing import TYPE_CHECKING, Any, ClassVar, cast import attr -from trio._deprecate import warn_deprecated - if sys.version_info < (3, 11): from exceptiongroup import BaseExceptionGroup, ExceptionGroup if TYPE_CHECKING: - from collections.abc import Callable, Sequence + from collections.abc import Callable - from typing_extensions import Self ################################################################ -# MultiError +# ExceptionGroup ################################################################ def _filter_impl( handler: Callable[[BaseException], BaseException | None], root_exc: BaseException ) -> BaseException | None: - # We have a tree of MultiError's, like: + # We have a tree of ExceptionGroup's, like: # - # MultiError([ + # ExceptionGroup([ # ValueError, - # MultiError([ + # ExceptionGroup([ # KeyError, # ValueError, # ]), @@ -41,13 +38,13 @@ def _filter_impl( # potentially sticking around as __context__ or __cause__), or # disappear altogether. # 2) simplify the resulting tree -- remove empty nodes, and replace - # singleton MultiError's with their contents, e.g.: - # MultiError([KeyError]) -> KeyError + # singleton ExceptionGroup's with their contents, e.g.: + # ExceptionGroup([KeyError]) -> KeyError # (This can happen recursively, e.g. if the two ValueErrors above # get caught then we'll just be left with a bare KeyError.) # 3) preserve sensible tracebacks # - # It's the tracebacks that are most confusing. As a MultiError + # It's the tracebacks that are most confusing. As a ExceptionGroup # propagates through the stack, it accumulates traceback frames, but # the exceptions inside it don't. Semantically, the traceback for a # leaf exception is the concatenation the tracebacks of all the @@ -57,7 +54,7 @@ def _filter_impl( # # The easy way to do that would be to, at the beginning of this # function, "push" all tracebacks down to the leafs, so all the - # MultiErrors have __traceback__=None, and all the leafs have complete + # ExceptionGroups have __traceback__=None, and all the leafs have complete # tracebacks. But whenever possible, we'd actually prefer to keep # tracebacks as high up in the tree as possible, because this lets us # keep only a single copy of the common parts of these exception's @@ -80,11 +77,11 @@ def _filter_impl( # which is difficult to fix on our end. # Filters a subtree, ignoring tracebacks, while keeping a record of - # which MultiErrors were preserved unchanged + # which ExceptionGroups were preserved unchanged def filter_tree( - exc: MultiError | BaseException, preserved: set[int] - ) -> MultiError | BaseException | None: - if isinstance(exc, MultiError): + exc: BaseExceptionGroup[BaseException] | BaseException, preserved: set[int] + ) -> BaseExceptionGroup[BaseException] | BaseException | None: + if isinstance(exc, BaseExceptionGroup): new_exceptions = [] changed = False for child_exc in exc.exceptions: @@ -98,7 +95,7 @@ def filter_tree( if not new_exceptions: return None elif changed: - return MultiError(new_exceptions) + return BaseExceptionGroup(new_exceptions) else: preserved.add(id(exc)) return exc @@ -115,7 +112,7 @@ def push_tb_down( if id(exc) in preserved: return new_tb = concat_tb(tb, exc.__traceback__) - if isinstance(exc, MultiError): + if isinstance(exc, BaseExceptionGroup): for child_exc in exc.exceptions: push_tb_down( # noqa: F821 # Deleted in local scope below, causes ruff to think it's not defined (astral-sh/ruff#7733) new_tb, child_exc, preserved @@ -183,166 +180,11 @@ def __exit__( _BaseExceptionGroup = BaseExceptionGroup -class MultiError(_BaseExceptionGroup): - """An exception that contains other exceptions; also known as an - "inception". - - It's main use is to represent the situation when multiple child tasks all - raise errors "in parallel". - - Args: - exceptions (list): The exceptions - - Returns: - If ``len(exceptions) == 1``, returns that exception. This means that a - call to ``MultiError(...)`` is not guaranteed to return a - :exc:`MultiError` object! - - Otherwise, returns a new :exc:`MultiError` object. - - Raises: - TypeError: if any of the passed in objects are not instances of - :exc:`BaseException`. - - """ - - def __init__( - self, exceptions: Sequence[BaseException], *, _collapse: bool = True - ) -> None: - self.collapse = _collapse - - # Avoid double initialization when _collapse is True and exceptions[0] returned - # by __new__() happens to be a MultiError and subsequently __init__() is called. - if _collapse and getattr(self, "exceptions", None) is not None: - # This exception was already initialized. - return - - super().__init__("multiple tasks failed", exceptions) - - def __new__( # type: ignore[misc] # mypy says __new__ must return a class instance - cls, exceptions: Sequence[BaseException], *, _collapse: bool = True - ) -> NonBaseMultiError | Self | BaseException: - exceptions = list(exceptions) - for exc in exceptions: - if not isinstance(exc, BaseException): - raise TypeError(f"Expected an exception object, not {exc!r}") - if _collapse and len(exceptions) == 1: - # If this lone object happens to itself be a MultiError, then - # Python will implicitly call our __init__ on it again. See - # special handling in __init__. - return exceptions[0] - else: - # The base class __new__() implicitly invokes our __init__, which - # is what we want. - # - # In an earlier version of the code, we didn't define __init__ and - # simply set the `exceptions` attribute directly on the new object. - # However, linters expect attributes to be initialized in __init__. - from_class: type[Self | NonBaseMultiError] = cls - if all(isinstance(exc, Exception) for exc in exceptions): - from_class = NonBaseMultiError - - # Ignoring arg-type: 'Argument 3 to "__new__" of "BaseExceptionGroup" has incompatible type "list[BaseException]"; expected "Sequence[_BaseExceptionT_co]"' - # We have checked that exceptions is indeed a list of BaseException objects, this is fine. - new_obj = super().__new__(from_class, "multiple tasks failed", exceptions) # type: ignore[arg-type] - assert isinstance(new_obj, (cls, NonBaseMultiError)) - return new_obj - - def __reduce__( - self, - ) -> tuple[object, tuple[type[Self], list[BaseException]], dict[str, bool]]: - return ( - self.__new__, - (self.__class__, list(self.exceptions)), - {"collapse": self.collapse}, - ) - - def __str__(self) -> str: - return ", ".join(repr(exc) for exc in self.exceptions) - - def __repr__(self) -> str: - return f"" - - @overload # type: ignore[override] # 'Exception' != '_ExceptionT' - def derive(self, excs: Sequence[Exception], /) -> NonBaseMultiError: - ... - - @overload - def derive(self, excs: Sequence[BaseException], /) -> MultiError: - ... - - def derive( - self, excs: Sequence[Exception | BaseException], / - ) -> NonBaseMultiError | MultiError: - # We use _collapse=False here to get ExceptionGroup semantics, since derive() - # is part of the PEP 654 API - exc = MultiError(excs, _collapse=False) - exc.collapse = self.collapse - return exc - - @classmethod - def filter( - cls, - handler: Callable[[BaseException], BaseException | None], - root_exc: BaseException, - ) -> BaseException | None: - """Apply the given ``handler`` to all the exceptions in ``root_exc``. - - Args: - handler: A callable that takes an atomic (non-MultiError) exception - as input, and returns either a new exception object or None. - root_exc: An exception, often (though not necessarily) a - :exc:`MultiError`. - - Returns: - A new exception object in which each component exception ``exc`` has - been replaced by the result of running ``handler(exc)`` – or, if - ``handler`` returned None for all the inputs, returns None. - - """ - warn_deprecated( - "MultiError.filter()", - "0.22.0", - instead="BaseExceptionGroup.split()", - issue=2211, - ) - return _filter_impl(handler, root_exc) - - @classmethod - def catch( - cls, handler: Callable[[BaseException], BaseException | None] - ) -> MultiErrorCatcher: - """Return a context manager that catches and re-throws exceptions - after running :meth:`filter` on them. - - Args: - handler: as for :meth:`filter` - - """ - warn_deprecated( - "MultiError.catch", - "0.22.0", - instead="except* or exceptiongroup.catch()", - issue=2211, - ) - - return MultiErrorCatcher(handler) - - if TYPE_CHECKING: _ExceptionGroup = ExceptionGroup[Exception] else: _ExceptionGroup = ExceptionGroup - -class NonBaseMultiError(MultiError, _ExceptionGroup): - __slots__ = () - - -# Clean up exception printing: -MultiError.__module__ = "trio" -NonBaseMultiError.__module__ = "trio" - ################################################################ # concat_tb ################################################################ diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index d5006eed91..d09f43ddc8 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -39,7 +39,7 @@ from ._exceptions import Cancelled, RunFinishedError, TrioInternalError from ._instrumentation import Instruments from ._ki import LOCALS_KEY_KI_PROTECTION_ENABLED, KIManager, enable_ki_protection -from ._multierror import MultiError, concat_tb +from ._multierror import concat_tb from ._thread_cache import start_thread_soon from ._traps import ( Abort, @@ -194,7 +194,11 @@ def collapse_exception_group( modified = True exceptions[i] = new_exc - if len(exceptions) == 1 and isinstance(excgroup, MultiError) and excgroup.collapse: + if ( + len(exceptions) == 1 + and isinstance(excgroup, BaseExceptionGroup) + and "collapsible" in str(excgroup) + ): exceptions[0].__traceback__ = concat_tb( excgroup.__traceback__, exceptions[0].__traceback__ ) @@ -1108,9 +1112,16 @@ def aborted(raise_cancel: _core.RaiseCancelT) -> Abort: assert popped is self if self._pending_excs: try: - return MultiError( - self._pending_excs, _collapse=not self._strict_exception_groups + return BaseExceptionGroup( + "collapsible" if not self._strict_exception_groups else "TODO", + self._pending_excs, ) + # if self._strict_exception_groups or len(self._pending_excs) > 1: + # return BaseExceptionGroup("TODO", + # self._pending_excs + # ) + # else: + # return self._pending_excs[0] finally: # avoid a garbage cycle # (see test_nursery_cancel_doesnt_create_cyclic_garbage) diff --git a/src/trio/_core/_tests/test_multierror.py b/src/trio/_core/_tests/test_multierror.py index 505b371f41..f2897e9174 100644 --- a/src/trio/_core/_tests/test_multierror.py +++ b/src/trio/_core/_tests/test_multierror.py @@ -2,20 +2,17 @@ import gc import os -import pickle import re import subprocess import sys -import warnings from pathlib import Path -from traceback import extract_tb, print_exception -from typing import TYPE_CHECKING, Callable, NoReturn +from traceback import extract_tb +from typing import TYPE_CHECKING, Any, Callable, NoReturn, TypeVar import pytest -from ... import TrioDeprecationWarning from ..._core import open_nursery -from .._multierror import MultiError, NonBaseMultiError, concat_tb +from .._multierror import concat_tb from .tutil import slow if TYPE_CHECKING: @@ -24,6 +21,8 @@ if sys.version_info < (3, 11): from exceptiongroup import ExceptionGroup +E = TypeVar("E", bound=BaseException) + class NotHashableException(Exception): code: int | None = None @@ -62,11 +61,7 @@ def raiser2_2() -> NoReturn: raise KeyError("raiser2_string") -def raiser3() -> NoReturn: - raise NameError - - -def get_exc(raiser: Callable[[], NoReturn]) -> BaseException: +def get_exc(raiser: Callable[[], NoReturn]) -> Exception: try: raiser() except Exception as exc: @@ -103,83 +98,22 @@ def test_concat_tb() -> None: assert extract_tb(get_tb(raiser2)) == entries2 -def test_MultiError() -> None: - exc1 = get_exc(raiser1) - exc2 = get_exc(raiser2) - - assert MultiError([exc1]) is exc1 - m = MultiError([exc1, exc2]) - assert m.exceptions == (exc1, exc2) - assert "ValueError" in str(m) - assert "ValueError" in repr(m) - - with pytest.raises(TypeError): - MultiError(object()) # type: ignore[arg-type] - with pytest.raises(TypeError): - MultiError([KeyError(), ValueError]) # type: ignore[list-item] - - -def test_MultiErrorOfSingleMultiError() -> None: - # For MultiError([MultiError]), ensure there is no bad recursion by the - # constructor where __init__ is called if __new__ returns a bare MultiError. - exceptions = (KeyError(), ValueError()) - a = MultiError(exceptions) - b = MultiError([a]) - assert b == a - assert b.exceptions == exceptions - - -async def test_MultiErrorNotHashable() -> None: +async def test_ExceptionGroupNotHashable() -> None: exc1 = NotHashableException(42) exc2 = NotHashableException(4242) exc3 = ValueError() assert exc1 != exc2 assert exc1 != exc3 - with pytest.raises(MultiError): + with pytest.raises(ExceptionGroup): async with open_nursery() as nursery: nursery.start_soon(raise_nothashable, 42) nursery.start_soon(raise_nothashable, 4242) -def test_MultiError_filter_NotHashable() -> None: - excs = MultiError([NotHashableException(42), ValueError()]) - - def handle_ValueError(exc: BaseException) -> BaseException | None: - if isinstance(exc, ValueError): - return None - else: - return exc - - with pytest.warns(TrioDeprecationWarning): - filtered_excs = MultiError.filter(handle_ValueError, excs) - - assert isinstance(filtered_excs, NotHashableException) - - -def make_tree() -> MultiError: - # Returns an object like: - # MultiError([ - # MultiError([ - # ValueError, - # KeyError, - # ]), - # NameError, - # ]) - # where all exceptions except the root have a non-trivial traceback. - exc1 = get_exc(raiser1) - exc2 = get_exc(raiser2) - exc3 = get_exc(raiser3) - - # Give m12 a non-trivial traceback - try: - raise MultiError([exc1, exc2]) - except BaseException as m12: - return MultiError([m12, exc3]) - - def assert_tree_eq( - m1: BaseException | MultiError | None, m2: BaseException | MultiError | None + m1: BaseException | ExceptionGroup[Any] | None, + m2: BaseException | ExceptionGroup[Any] | None, ) -> None: if m1 is None or m2 is None: assert m1 is m2 @@ -188,197 +122,24 @@ def assert_tree_eq( assert extract_tb(m1.__traceback__) == extract_tb(m2.__traceback__) assert_tree_eq(m1.__cause__, m2.__cause__) assert_tree_eq(m1.__context__, m2.__context__) - if isinstance(m1, MultiError): - assert isinstance(m2, MultiError) + if isinstance(m1, ExceptionGroup): + assert isinstance(m2, ExceptionGroup) assert len(m1.exceptions) == len(m2.exceptions) for e1, e2 in zip(m1.exceptions, m2.exceptions): assert_tree_eq(e1, e2) -def test_MultiError_filter() -> None: - def null_handler(exc: BaseException) -> BaseException: - return exc - - m = make_tree() - assert_tree_eq(m, m) - with pytest.warns(TrioDeprecationWarning): - assert MultiError.filter(null_handler, m) is m - - assert_tree_eq(m, make_tree()) - - # Make sure we don't pick up any detritus if run in a context where - # implicit exception chaining would like to kick in - m = make_tree() - try: - raise ValueError - except ValueError: - with pytest.warns(TrioDeprecationWarning): - assert MultiError.filter(null_handler, m) is m - assert_tree_eq(m, make_tree()) - - def simple_filter(exc: BaseException) -> BaseException | None: - if isinstance(exc, ValueError): - return None - if isinstance(exc, KeyError): - return RuntimeError() - return exc - - with pytest.warns(TrioDeprecationWarning): - new_m = MultiError.filter(simple_filter, make_tree()) - - assert isinstance(new_m, MultiError) - assert len(new_m.exceptions) == 2 - # was: [[ValueError, KeyError], NameError] - # ValueError disappeared & KeyError became RuntimeError, so now: - assert isinstance(new_m.exceptions[0], RuntimeError) - assert isinstance(new_m.exceptions[1], NameError) - - # implicit chaining: - assert isinstance(new_m.exceptions[0].__context__, KeyError) - - # also, the traceback on the KeyError incorporates what used to be the - # traceback on its parent MultiError - orig = make_tree() - # make sure we have the right path - assert isinstance(orig.exceptions[0], MultiError) - assert isinstance(orig.exceptions[0].exceptions[1], KeyError) - # get original traceback summary - orig_extracted = ( - extract_tb(orig.__traceback__) - + extract_tb(orig.exceptions[0].__traceback__) - + extract_tb(orig.exceptions[0].exceptions[1].__traceback__) - ) - - def p(exc: BaseException) -> None: - print_exception(type(exc), exc, exc.__traceback__) - - p(orig) - p(orig.exceptions[0]) - p(orig.exceptions[0].exceptions[1]) - p(new_m.exceptions[0].__context__) - # compare to the new path - assert new_m.__traceback__ is None - new_extracted = extract_tb(new_m.exceptions[0].__context__.__traceback__) - assert orig_extracted == new_extracted - - # check preserving partial tree - def filter_NameError(exc: BaseException) -> BaseException | None: - if isinstance(exc, NameError): - return None - return exc - - m = make_tree() - with pytest.warns(TrioDeprecationWarning): - new_m = MultiError.filter(filter_NameError, m) - # with the NameError gone, the other branch gets promoted - assert new_m is m.exceptions[0] - - # check fully handling everything - def filter_all(exc: BaseException) -> None: - return None - - with pytest.warns(TrioDeprecationWarning): - assert MultiError.filter(filter_all, make_tree()) is None - - -def test_MultiError_catch() -> None: - # No exception to catch - - def noop(_: object) -> None: - pass # pragma: no cover - - with pytest.warns(TrioDeprecationWarning), MultiError.catch(noop): - pass - - # Simple pass-through of all exceptions - m = make_tree() - with pytest.raises(MultiError) as excinfo: - with pytest.warns(TrioDeprecationWarning), MultiError.catch(lambda exc: exc): - raise m - assert excinfo.value is m - # Should be unchanged, except that we added a traceback frame by raising - # it here - assert m.__traceback__ is not None - assert m.__traceback__.tb_frame.f_code.co_name == "test_MultiError_catch" - assert m.__traceback__.tb_next is None - m.__traceback__ = None - assert_tree_eq(m, make_tree()) - - # Swallows everything - with pytest.warns(TrioDeprecationWarning), MultiError.catch(lambda _: None): - raise make_tree() - - def simple_filter(exc): - if isinstance(exc, ValueError): - return None - if isinstance(exc, KeyError): - return RuntimeError() - return exc - - with pytest.raises(MultiError) as excinfo: - with pytest.warns(TrioDeprecationWarning), MultiError.catch(simple_filter): - raise make_tree() - new_m = excinfo.value - assert isinstance(new_m, MultiError) - assert len(new_m.exceptions) == 2 - # was: [[ValueError, KeyError], NameError] - # ValueError disappeared & KeyError became RuntimeError, so now: - assert isinstance(new_m.exceptions[0], RuntimeError) - assert isinstance(new_m.exceptions[1], NameError) - # Make sure that Python did not successfully attach the old MultiError to - # our new MultiError's __context__ - assert not new_m.__suppress_context__ - assert new_m.__context__ is None - - # check preservation of __cause__ and __context__ - v = ValueError() - v.__cause__ = KeyError() - with pytest.raises(ValueError) as excinfo: - with pytest.warns(TrioDeprecationWarning), MultiError.catch(lambda exc: exc): - raise v - assert isinstance(excinfo.value.__cause__, KeyError) - - v = ValueError() - context = KeyError() - v.__context__ = context - with pytest.raises(ValueError) as excinfo: - with pytest.warns(TrioDeprecationWarning), MultiError.catch(lambda exc: exc): - raise v - assert excinfo.value.__context__ is context - assert not excinfo.value.__suppress_context__ - - for suppress_context in [True, False]: - v = ValueError() - context = KeyError() - v.__context__ = context - v.__suppress_context__ = suppress_context - distractor = RuntimeError() - with pytest.raises(ValueError) as excinfo: - - def catch_RuntimeError(exc): - if isinstance(exc, RuntimeError): - return None - else: - return exc - - with pytest.warns(TrioDeprecationWarning): - with MultiError.catch(catch_RuntimeError): - raise MultiError([v, distractor]) - assert excinfo.value.__context__ is context - assert excinfo.value.__suppress_context__ == suppress_context - - @pytest.mark.skipif( sys.implementation.name != "cpython", reason="Only makes sense with refcounting GC" ) -def test_MultiError_catch_doesnt_create_cyclic_garbage() -> None: +def test_ExceptionGroup_catch_doesnt_create_cyclic_garbage() -> None: # https://github.com/python-trio/trio/pull/2063 gc.collect() old_flags = gc.get_debug() def make_multi() -> NoReturn: # make_tree creates cycles itself, so a simple - raise MultiError([get_exc(raiser1), get_exc(raiser2)]) + raise ExceptionGroup("", [get_exc(raiser1), get_exc(raiser2)]) def simple_filter(exc: BaseException) -> Exception | RuntimeError: if isinstance(exc, ValueError): @@ -391,10 +152,9 @@ def simple_filter(exc: BaseException) -> Exception | RuntimeError: try: gc.set_debug(gc.DEBUG_SAVEALL) - with pytest.raises(MultiError): - # covers MultiErrorCatcher.__exit__ and _multierror.copy_tb - with pytest.warns(TrioDeprecationWarning), MultiError.catch(simple_filter): - raise make_multi() + with pytest.raises(ExceptionGroup): + # covers ExceptionGroupCatcher.__exit__ and _multierror.copy_tb + raise make_multi() gc.collect() assert not gc.garbage finally: @@ -420,27 +180,6 @@ def test_assert_match_in_seq() -> None: assert_match_in_seq(["a", "b"], "xx b xx a xx") -def test_base_multierror() -> None: - """ - Test that MultiError() with at least one base exception will return a MultiError - object. - """ - - exc = MultiError([ZeroDivisionError(), KeyboardInterrupt()]) - assert type(exc) is MultiError - - -def test_non_base_multierror() -> None: - """ - Test that MultiError() without base exceptions will return a NonBaseMultiError - object. - """ - - exc = MultiError([ZeroDivisionError(), ValueError()]) - assert type(exc) is NonBaseMultiError - assert isinstance(exc, ExceptionGroup) - - def run_script(name: str) -> subprocess.CompletedProcess[bytes]: import trio @@ -484,36 +223,3 @@ def test_apport_excepthook_monkeypatch_interaction() -> None: ["--- 1 ---", "KeyError", "--- 2 ---", "ValueError"], stdout, ) - - -@pytest.mark.parametrize("protocol", range(0, pickle.HIGHEST_PROTOCOL + 1)) -def test_pickle_multierror(protocol: int) -> None: - # use trio.MultiError to make sure that pickle works through the deprecation layer - import trio - - my_except = ZeroDivisionError() - - try: - 1 / 0 # noqa: B018 # "useless statement" - except ZeroDivisionError as exc: - my_except = exc - - # MultiError will collapse into different classes depending on the errors - for cls, errors in ( - (ZeroDivisionError, [my_except]), - (NonBaseMultiError, [my_except, ValueError()]), - (MultiError, [BaseException(), my_except]), - ): - with warnings.catch_warnings(): - warnings.simplefilter("ignore", TrioDeprecationWarning) - me = trio.MultiError(errors) # type: ignore[attr-defined] - dump = pickle.dumps(me, protocol=protocol) - load = pickle.loads(dump) - assert repr(me) == repr(load) - assert me.__class__ == load.__class__ == cls - - assert me.__dict__.keys() == load.__dict__.keys() - for me_val, load_val in zip(me.__dict__.values(), load.__dict__.values()): - # tracebacks etc are not preserved through pickling for the default - # exceptions, so we only check that the repr stays the same - assert repr(me_val) == repr(load_val) diff --git a/src/trio/_core/_tests/test_multierror_scripts/apport_excepthook.py b/src/trio/_core/_tests/test_multierror_scripts/apport_excepthook.py index 0e46f37e17..a4217a3155 100644 --- a/src/trio/_core/_tests/test_multierror_scripts/apport_excepthook.py +++ b/src/trio/_core/_tests/test_multierror_scripts/apport_excepthook.py @@ -10,6 +10,7 @@ apport_python_hook.install() -from trio._core._multierror import MultiError # Bypass deprecation warnings +if sys.version_info < (3, 11): + from exceptiongroup import ExceptionGroup -raise MultiError([KeyError("key_error"), ValueError("value_error")]) +raise ExceptionGroup("", [KeyError("key_error"), ValueError("value_error")]) diff --git a/src/trio/_core/_tests/test_multierror_scripts/simple_excepthook.py b/src/trio/_core/_tests/test_multierror_scripts/simple_excepthook.py index 236d34e9ba..9a3fc8e4a7 100644 --- a/src/trio/_core/_tests/test_multierror_scripts/simple_excepthook.py +++ b/src/trio/_core/_tests/test_multierror_scripts/simple_excepthook.py @@ -1,6 +1,9 @@ import _common # isort: split -from trio._core._multierror import MultiError # Bypass deprecation warnings +import sys + +if sys.version_info < (3, 11): + from exceptiongroup import ExceptionGroup def exc1_fn() -> Exception: @@ -18,4 +21,4 @@ def exc2_fn() -> Exception: # This should be printed nicely, because Trio overrode sys.excepthook -raise MultiError([exc1_fn(), exc2_fn()]) +raise ExceptionGroup("", [exc1_fn(), exc2_fn()]) diff --git a/src/trio/_core/_tests/test_run.py b/src/trio/_core/_tests/test_run.py index 310e9a67e5..5c07d53143 100644 --- a/src/trio/_core/_tests/test_run.py +++ b/src/trio/_core/_tests/test_run.py @@ -17,7 +17,6 @@ import sniffio from ... import _core -from ..._core._multierror import MultiError, NonBaseMultiError from ..._threads import to_thread_run_sync from ..._timeouts import fail_after, sleep from ...testing import Sequencer, assert_checkpoints, wait_all_tasks_blocked @@ -187,7 +186,7 @@ async def main() -> None: def test_main_and_task_both_crash() -> None: # If main crashes and there's also a task crash, then we get both in a - # MultiError + # ExceptionGroup async def crasher() -> NoReturn: raise ValueError @@ -196,7 +195,7 @@ async def main() -> NoReturn: nursery.start_soon(crasher) raise KeyError - with pytest.raises(MultiError) as excinfo: + with pytest.raises(ExceptionGroup) as excinfo: _core.run(main) print(excinfo.value) assert {type(exc) for exc in excinfo.value.exceptions} == { @@ -214,7 +213,7 @@ async def main() -> None: nursery.start_soon(crasher, KeyError) nursery.start_soon(crasher, ValueError) - with pytest.raises(MultiError) as excinfo: + with pytest.raises(ExceptionGroup) as excinfo: _core.run(main) assert {type(exc) for exc in excinfo.value.exceptions} == { ValueError, @@ -449,7 +448,7 @@ async def crasher() -> NoReturn: # And one that raises a different error nursery.start_soon(crasher) # t4 # and then our __aexit__ also receives an outer Cancelled - except MultiError as multi_exc: + except ExceptionGroup as multi_exc: # Since the outer scope became cancelled before the # nursery block exited, all cancellations inside the # nursery block continue propagating to reach the @@ -788,7 +787,7 @@ async def task2() -> None: with pytest.raises(RuntimeError) as exc_info: await nursery_mgr.__aexit__(*sys.exc_info()) assert "which had already been exited" in str(exc_info.value) - assert type(exc_info.value.__context__) is NonBaseMultiError + assert type(exc_info.value.__context__) is ExceptionGroup assert len(exc_info.value.__context__.exceptions) == 3 cancelled_in_context = False for exc in exc_info.value.__context__.exceptions: @@ -930,7 +929,7 @@ async def main() -> None: _core.run(main) -def test_system_task_crash_MultiError() -> None: +def test_system_task_crash_ExceptionGroup() -> None: async def crasher1() -> NoReturn: raise KeyError @@ -950,7 +949,7 @@ async def main() -> None: _core.run(main) me = excinfo.value.__cause__ - assert isinstance(me, MultiError) + assert isinstance(me, ExceptionGroup) assert len(me.exceptions) == 2 for exc in me.exceptions: assert isinstance(exc, (KeyError, ValueError)) @@ -958,7 +957,7 @@ async def main() -> None: def test_system_task_crash_plus_Cancelled() -> None: # Set up a situation where a system task crashes with a - # MultiError([Cancelled, ValueError]) + # ExceptionGroup([Cancelled, ValueError]) async def crasher() -> None: try: await sleep_forever() @@ -1127,11 +1126,11 @@ async def test_nursery_exception_chaining_doesnt_make_context_loops() -> None: async def crasher() -> NoReturn: raise KeyError - with pytest.raises(MultiError) as excinfo: + with pytest.raises(ExceptionGroup) as excinfo: async with _core.open_nursery() as nursery: nursery.start_soon(crasher) raise ValueError - # the MultiError should not have the KeyError or ValueError as context + # the ExceptionGroup should not have the KeyError or ValueError as context assert excinfo.value.__context__ is None @@ -1894,7 +1893,7 @@ async def fail() -> NoReturn: async with _core.open_nursery() as nursery: nursery.start_soon(fail) raise StopIteration - except MultiError as e: + except ExceptionGroup as e: assert tuple(map(type, e.exceptions)) == (StopIteration, ValueError) @@ -1946,14 +1945,14 @@ async def my_child_task() -> NoReturn: try: # Trick: For now cancel/nursery scopes still leave a bunch of tb gunk - # behind. But if there's a MultiError, they leave it on the MultiError, + # behind. But if there's a ExceptionGroup, they leave it on the ExceptionGroup, # which lets us get a clean look at the KeyError itself. Someday I - # guess this will always be a MultiError (#611), but for now we can + # guess this will always be a ExceptionGroup (#611), but for now we can # force it by raising two exceptions. async with _core.open_nursery() as nursery: nursery.start_soon(my_child_task) nursery.start_soon(my_child_task) - except MultiError as exc: + except ExceptionGroup as exc: first_exc = exc.exceptions[0] assert isinstance(first_exc, KeyError) # The top frame in the exception traceback should be inside the child @@ -2356,7 +2355,7 @@ async def crasher() -> NoReturn: outer.cancel() # And one that raises a different error nursery.start_soon(crasher) - # so that outer filters a Cancelled from the MultiError and + # so that outer filters a Cancelled from the ExceptionGroup and # covers CancelScope.__exit__ (and NurseryManager.__aexit__) # (See https://github.com/python-trio/trio/pull/2063) @@ -2436,7 +2435,7 @@ async def main() -> NoReturn: async with _core.open_nursery(): raise Exception("foo") - with pytest.raises(MultiError) as exc: + with pytest.raises(ExceptionGroup) as exc: _core.run(main, strict_exception_groups=True) assert len(exc.value.exceptions) == 1 @@ -2460,7 +2459,7 @@ async def main() -> NoReturn: async def test_nursery_strict_exception_groups() -> None: """Test that strict exception groups can be enabled on a per-nursery basis.""" - with pytest.raises(MultiError) as exc: + with pytest.raises(ExceptionGroup) as exc: async with _core.open_nursery(strict_exception_groups=True): raise Exception("foo") @@ -2478,7 +2477,7 @@ async def test_nursery_collapse_strict() -> None: async def raise_error() -> NoReturn: raise RuntimeError("test error") - with pytest.raises(MultiError) as exc: + with pytest.raises(ExceptionGroup) as exc: async with _core.open_nursery() as nursery: nursery.start_soon(sleep_forever) nursery.start_soon(raise_error) @@ -2490,7 +2489,7 @@ async def raise_error() -> NoReturn: exceptions = exc.value.exceptions assert len(exceptions) == 2 assert isinstance(exceptions[0], RuntimeError) - assert isinstance(exceptions[1], MultiError) + assert isinstance(exceptions[1], ExceptionGroup) assert len(exceptions[1].exceptions) == 1 assert isinstance(exceptions[1].exceptions[0], RuntimeError) @@ -2504,7 +2503,7 @@ async def test_nursery_collapse_loose() -> None: async def raise_error() -> NoReturn: raise RuntimeError("test error") - with pytest.raises(MultiError) as exc: + with pytest.raises(ExceptionGroup) as exc: async with _core.open_nursery() as nursery: nursery.start_soon(sleep_forever) nursery.start_soon(raise_error) diff --git a/src/trio/_highlevel_open_tcp_stream.py b/src/trio/_highlevel_open_tcp_stream.py index f6f8aba425..d5c83da7c0 100644 --- a/src/trio/_highlevel_open_tcp_stream.py +++ b/src/trio/_highlevel_open_tcp_stream.py @@ -5,7 +5,6 @@ from typing import TYPE_CHECKING, Any import trio -from trio._core._multierror import MultiError from trio.socket import SOCK_STREAM, SocketType, getaddrinfo, socket if TYPE_CHECKING: @@ -13,7 +12,7 @@ from socket import AddressFamily, SocketKind if sys.version_info < (3, 11): - from exceptiongroup import ExceptionGroup + from exceptiongroup import BaseExceptionGroup, ExceptionGroup # Implementation of RFC 6555 "Happy eyeballs" @@ -130,7 +129,7 @@ def close_all() -> Generator[set[SocketType], None, None]: if len(errs) == 1: raise errs[0] elif errs: - raise MultiError(errs) + raise BaseExceptionGroup("", errs) def reorder_for_rfc_6555_section_5_4( diff --git a/src/trio/_tests/test_exports.py b/src/trio/_tests/test_exports.py index 8afb710eb6..7a6aacd48d 100644 --- a/src/trio/_tests/test_exports.py +++ b/src/trio/_tests/test_exports.py @@ -327,10 +327,6 @@ def lookup_symbol(symbol: str) -> dict[str, str]: continue if module_name == "trio.socket" and class_name in dir(stdlib_socket): continue - # Deprecated classes are exported with a leading underscore - # We don't care about errors in _MultiError as that's on its way out anyway - if class_name.startswith("_"): # pragma: no cover - continue # dir() and inspect.getmembers doesn't display properties from the metaclass # also ignore some dunder methods that tend to differ but are of no consequence From 9c23ab0728db6c5b96be1b102b66759a3063ddd9 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Mon, 27 Nov 2023 17:36:03 +0100 Subject: [PATCH 02/20] remove more unused code, fix test_cancel_scope_multierror_filtering inner check not being run --- src/trio/_core/_multierror.py | 183 +---------------------- src/trio/_core/_tests/test_multierror.py | 33 +--- src/trio/_core/_tests/test_run.py | 8 +- 3 files changed, 12 insertions(+), 212 deletions(-) diff --git a/src/trio/_core/_multierror.py b/src/trio/_core/_multierror.py index 0ccbea7b19..13e8e565cc 100644 --- a/src/trio/_core/_multierror.py +++ b/src/trio/_core/_multierror.py @@ -2,188 +2,7 @@ import sys from types import TracebackType -from typing import TYPE_CHECKING, Any, ClassVar, cast - -import attr - -if sys.version_info < (3, 11): - from exceptiongroup import BaseExceptionGroup, ExceptionGroup - -if TYPE_CHECKING: - from collections.abc import Callable - -################################################################ -# ExceptionGroup -################################################################ - - -def _filter_impl( - handler: Callable[[BaseException], BaseException | None], root_exc: BaseException -) -> BaseException | None: - # We have a tree of ExceptionGroup's, like: - # - # ExceptionGroup([ - # ValueError, - # ExceptionGroup([ - # KeyError, - # ValueError, - # ]), - # ]) - # - # or similar. - # - # We want to - # 1) apply the filter to each of the leaf exceptions -- each leaf - # might stay the same, be replaced (with the original exception - # potentially sticking around as __context__ or __cause__), or - # disappear altogether. - # 2) simplify the resulting tree -- remove empty nodes, and replace - # singleton ExceptionGroup's with their contents, e.g.: - # ExceptionGroup([KeyError]) -> KeyError - # (This can happen recursively, e.g. if the two ValueErrors above - # get caught then we'll just be left with a bare KeyError.) - # 3) preserve sensible tracebacks - # - # It's the tracebacks that are most confusing. As a ExceptionGroup - # propagates through the stack, it accumulates traceback frames, but - # the exceptions inside it don't. Semantically, the traceback for a - # leaf exception is the concatenation the tracebacks of all the - # exceptions you see when traversing the exception tree from the root - # to that leaf. Our correctness invariant is that this concatenated - # traceback should be the same before and after. - # - # The easy way to do that would be to, at the beginning of this - # function, "push" all tracebacks down to the leafs, so all the - # ExceptionGroups have __traceback__=None, and all the leafs have complete - # tracebacks. But whenever possible, we'd actually prefer to keep - # tracebacks as high up in the tree as possible, because this lets us - # keep only a single copy of the common parts of these exception's - # tracebacks. This is cheaper (in memory + time -- tracebacks are - # unpleasantly quadratic-ish to work with, and this might matter if - # you have thousands of exceptions, which can happen e.g. after - # cancelling a large task pool, and no-one will ever look at their - # tracebacks!), and more importantly, factoring out redundant parts of - # the tracebacks makes them more readable if/when users do see them. - # - # So instead our strategy is: - # - first go through and construct the new tree, preserving any - # unchanged subtrees - # - then go through the original tree (!) and push tracebacks down - # until either we hit a leaf, or we hit a subtree which was - # preserved in the new tree. - - # This used to also support async handler functions. But that runs into: - # https://bugs.python.org/issue29600 - # which is difficult to fix on our end. - - # Filters a subtree, ignoring tracebacks, while keeping a record of - # which ExceptionGroups were preserved unchanged - def filter_tree( - exc: BaseExceptionGroup[BaseException] | BaseException, preserved: set[int] - ) -> BaseExceptionGroup[BaseException] | BaseException | None: - if isinstance(exc, BaseExceptionGroup): - new_exceptions = [] - changed = False - for child_exc in exc.exceptions: - new_child_exc = filter_tree( # noqa: F821 # Deleted in local scope below, causes ruff to think it's not defined (astral-sh/ruff#7733) - child_exc, preserved - ) - if new_child_exc is not child_exc: - changed = True - if new_child_exc is not None: - new_exceptions.append(new_child_exc) - if not new_exceptions: - return None - elif changed: - return BaseExceptionGroup(new_exceptions) - else: - preserved.add(id(exc)) - return exc - else: - new_exc = handler(exc) - # Our version of implicit exception chaining - if new_exc is not None and new_exc is not exc: - new_exc.__context__ = exc - return new_exc - - def push_tb_down( - tb: TracebackType | None, exc: BaseException, preserved: set[int] - ) -> None: - if id(exc) in preserved: - return - new_tb = concat_tb(tb, exc.__traceback__) - if isinstance(exc, BaseExceptionGroup): - for child_exc in exc.exceptions: - push_tb_down( # noqa: F821 # Deleted in local scope below, causes ruff to think it's not defined (astral-sh/ruff#7733) - new_tb, child_exc, preserved - ) - exc.__traceback__ = None - else: - exc.__traceback__ = new_tb - - preserved: set[int] = set() - new_root_exc = filter_tree(root_exc, preserved) - push_tb_down(None, root_exc, preserved) - # Delete the local functions to avoid a reference cycle (see - # test_simple_cancel_scope_usage_doesnt_create_cyclic_garbage) - del filter_tree, push_tb_down - return new_root_exc - - -# Normally I'm a big fan of (a)contextmanager, but in this case I found it -# easier to use the raw context manager protocol, because it makes it a lot -# easier to reason about how we're mutating the traceback as we go. (End -# result: if the exception gets modified, then the 'raise' here makes this -# frame show up in the traceback; otherwise, we leave no trace.) -@attr.s(frozen=True) -class MultiErrorCatcher: - _handler: Callable[[BaseException], BaseException | None] = attr.ib() - - def __enter__(self) -> None: - pass - - def __exit__( - self, - exc_type: type[BaseException] | None, - exc_value: BaseException | None, - traceback: TracebackType | None, - ) -> bool | None: - if exc_value is not None: - filtered_exc = _filter_impl(self._handler, exc_value) - - if filtered_exc is exc_value: - # Let the interpreter re-raise it - return False - if filtered_exc is None: - # Swallow the exception - return True - # When we raise filtered_exc, Python will unconditionally blow - # away its __context__ attribute and replace it with the original - # exc we caught. So after we raise it, we have to pause it while - # it's in flight to put the correct __context__ back. - old_context = filtered_exc.__context__ - try: - raise filtered_exc - finally: - _, value, _ = sys.exc_info() - assert value is filtered_exc - value.__context__ = old_context - # delete references from locals to avoid creating cycles - # see test_MultiError_catch_doesnt_create_cyclic_garbage - del _, filtered_exc, value - return False - - -if TYPE_CHECKING: - _BaseExceptionGroup = BaseExceptionGroup[BaseException] -else: - _BaseExceptionGroup = BaseExceptionGroup - - -if TYPE_CHECKING: - _ExceptionGroup = ExceptionGroup[Exception] -else: - _ExceptionGroup = ExceptionGroup +from typing import Any, ClassVar, cast ################################################################ # concat_tb diff --git a/src/trio/_core/_tests/test_multierror.py b/src/trio/_core/_tests/test_multierror.py index f2897e9174..67380df9c3 100644 --- a/src/trio/_core/_tests/test_multierror.py +++ b/src/trio/_core/_tests/test_multierror.py @@ -7,7 +7,7 @@ import sys from pathlib import Path from traceback import extract_tb -from typing import TYPE_CHECKING, Any, Callable, NoReturn, TypeVar +from typing import TYPE_CHECKING, Callable, NoReturn, TypeVar import pytest @@ -111,24 +111,6 @@ async def test_ExceptionGroupNotHashable() -> None: nursery.start_soon(raise_nothashable, 4242) -def assert_tree_eq( - m1: BaseException | ExceptionGroup[Any] | None, - m2: BaseException | ExceptionGroup[Any] | None, -) -> None: - if m1 is None or m2 is None: - assert m1 is m2 - return - assert type(m1) is type(m2) - assert extract_tb(m1.__traceback__) == extract_tb(m2.__traceback__) - assert_tree_eq(m1.__cause__, m2.__cause__) - assert_tree_eq(m1.__context__, m2.__context__) - if isinstance(m1, ExceptionGroup): - assert isinstance(m2, ExceptionGroup) - assert len(m1.exceptions) == len(m2.exceptions) - for e1, e2 in zip(m1.exceptions, m2.exceptions): - assert_tree_eq(e1, e2) - - @pytest.mark.skipif( sys.implementation.name != "cpython", reason="Only makes sense with refcounting GC" ) @@ -141,20 +123,13 @@ def make_multi() -> NoReturn: # make_tree creates cycles itself, so a simple raise ExceptionGroup("", [get_exc(raiser1), get_exc(raiser2)]) - def simple_filter(exc: BaseException) -> Exception | RuntimeError: - if isinstance(exc, ValueError): - return Exception() - if isinstance(exc, KeyError): - return RuntimeError() - raise AssertionError( - "only ValueError and KeyError should exist" - ) # pragma: no cover - try: gc.set_debug(gc.DEBUG_SAVEALL) - with pytest.raises(ExceptionGroup): + with pytest.raises(ExceptionGroup) as excinfo: # covers ExceptionGroupCatcher.__exit__ and _multierror.copy_tb raise make_multi() + for exc in excinfo.value.exceptions: + assert isinstance(exc, (ValueError, KeyError)) gc.collect() assert not gc.garbage finally: diff --git a/src/trio/_core/_tests/test_run.py b/src/trio/_core/_tests/test_run.py index 5c07d53143..96965e0c10 100644 --- a/src/trio/_core/_tests/test_run.py +++ b/src/trio/_core/_tests/test_run.py @@ -432,6 +432,10 @@ async def test_cancel_scope_multierror_filtering() -> None: async def crasher() -> NoReturn: raise KeyError + # check that the inner except is properly executed. + # alternative would be to have a `except BaseException` and an `else` + exception_group_caught_inner = False + try: with _core.CancelScope() as outer: try: @@ -448,7 +452,8 @@ async def crasher() -> NoReturn: # And one that raises a different error nursery.start_soon(crasher) # t4 # and then our __aexit__ also receives an outer Cancelled - except ExceptionGroup as multi_exc: + except BaseExceptionGroup as multi_exc: + exception_group_caught_inner = True # Since the outer scope became cancelled before the # nursery block exited, all cancellations inside the # nursery block continue propagating to reach the @@ -469,6 +474,7 @@ async def crasher() -> NoReturn: assert type(exc) is KeyError else: # pragma: no cover raise AssertionError() + assert exception_group_caught_inner async def test_precancelled_task() -> None: From 4cb329f8e746b3f7e9393f0b4895a84ea2706b16 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Tue, 28 Nov 2023 15:39:47 +0100 Subject: [PATCH 03/20] fix remaining failing tests, instantly collapse groups in nurseries in some cases --- src/trio/_core/_run.py | 8 ++------ src/trio/_core/_tests/test_run.py | 3 ++- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index d09f43ddc8..bef0f76505 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -1112,16 +1112,12 @@ def aborted(raise_cancel: _core.RaiseCancelT) -> Abort: assert popped is self if self._pending_excs: try: + if not self._strict_exception_groups and len(self._pending_excs) == 1: + return self._pending_excs[0] return BaseExceptionGroup( "collapsible" if not self._strict_exception_groups else "TODO", self._pending_excs, ) - # if self._strict_exception_groups or len(self._pending_excs) > 1: - # return BaseExceptionGroup("TODO", - # self._pending_excs - # ) - # else: - # return self._pending_excs[0] finally: # avoid a garbage cycle # (see test_nursery_cancel_doesnt_create_cyclic_garbage) diff --git a/src/trio/_core/_tests/test_run.py b/src/trio/_core/_tests/test_run.py index 96965e0c10..8d8694ff19 100644 --- a/src/trio/_core/_tests/test_run.py +++ b/src/trio/_core/_tests/test_run.py @@ -1785,7 +1785,8 @@ async def raise_keyerror_after_started( t0 = _core.current_time() with pytest.raises(RuntimeError): await closed_nursery.start(sleep_then_start, 7) - assert _core.current_time() == t0 + # sub-second delays can be caused by unrelated multitasking by an OS + assert int(_core.current_time()) == int(t0) async def test_task_nursery_stack() -> None: From 3822dc1474b2dba6851449cecb3113f0fe7ca7ea Mon Sep 17 00:00:00 2001 From: jakkdl Date: Tue, 28 Nov 2023 15:59:16 +0100 Subject: [PATCH 04/20] bump exceptiongroup version to one that has the apport excepthook --- src/trio/_core/_multierror.py | 37 ----------------------------------- test-requirements.in | 3 ++- test-requirements.txt | 2 +- 3 files changed, 3 insertions(+), 39 deletions(-) diff --git a/src/trio/_core/_multierror.py b/src/trio/_core/_multierror.py index 13e8e565cc..9a9fab9c86 100644 --- a/src/trio/_core/_multierror.py +++ b/src/trio/_core/_multierror.py @@ -1,6 +1,5 @@ from __future__ import annotations -import sys from types import TracebackType from typing import Any, ClassVar, cast @@ -124,39 +123,3 @@ def concat_tb( for head_tb in reversed(head_tbs): current_head = copy_tb(head_tb, tb_next=current_head) return current_head - - -# Ubuntu's system Python has a sitecustomize.py file that import -# apport_python_hook and replaces sys.excepthook. -# -# The custom hook captures the error for crash reporting, and then calls -# sys.__excepthook__ to actually print the error. -# -# We don't mind it capturing the error for crash reporting, but we want to -# take over printing the error. So we monkeypatch the apport_python_hook -# module so that instead of calling sys.__excepthook__, it calls our custom -# hook. -# -# More details: https://github.com/python-trio/trio/issues/1065 -if sys.version_info < (3, 11) and getattr(sys.excepthook, "__name__", None) in ( - "apport_excepthook", - "partial_apport_excepthook", -): - from types import ModuleType - - import apport_python_hook - from exceptiongroup import format_exception - - assert sys.excepthook is apport_python_hook.apport_excepthook - - def replacement_excepthook( - etype: type[BaseException], value: BaseException, tb: TracebackType | None - ) -> None: - # This does work, it's an overloaded function - sys.stderr.write("".join(format_exception(etype, value, tb))) # type: ignore[arg-type] - - fake_sys = ModuleType("trio_fake_sys") - fake_sys.__dict__.update(sys.__dict__) - # Fake does have __excepthook__ after __dict__ update, but type checkers don't recognize this - fake_sys.__excepthook__ = replacement_excepthook # type: ignore[attr-defined] - apport_python_hook.sys = fake_sys diff --git a/test-requirements.in b/test-requirements.in index 683eeb21db..0cc6f73285 100644 --- a/test-requirements.in +++ b/test-requirements.in @@ -30,4 +30,5 @@ sortedcontainers idna outcome sniffio -exceptiongroup >= 1.0.0rc9; python_version < "3.11" +# 1.2.0 ships monkeypatching for apport excepthook +exceptiongroup >= 1.2.0; python_version < "3.11" diff --git a/test-requirements.txt b/test-requirements.txt index 0dc9d07a15..668455cef8 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -36,7 +36,7 @@ cryptography==41.0.5 # types-pyopenssl dill==0.3.7 # via pylint -exceptiongroup==1.1.3 ; python_version < "3.11" +exceptiongroup==1.2.0 ; python_version < "3.11" # via # -r test-requirements.in # pytest From f52ea57e6dc1b7813f04f7b45c466ad83bf8e7e8 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Tue, 28 Nov 2023 16:15:04 +0100 Subject: [PATCH 05/20] remove multierror_scripts, rename _core/_multierror.py to _core/_concat_tb.py --- pyproject.toml | 4 -- .../_core/{_multierror.py => _concat_tb.py} | 3 + src/trio/_core/_run.py | 2 +- src/trio/_core/_tests/test_multierror.py | 70 +------------------ .../test_multierror_scripts/__init__.py | 3 - .../_tests/test_multierror_scripts/_common.py | 7 -- .../apport_excepthook.py | 16 ----- .../simple_excepthook.py | 24 ------- src/trio/_core/_tests/test_run.py | 2 +- 9 files changed, 6 insertions(+), 125 deletions(-) rename src/trio/_core/{_multierror.py => _concat_tb.py} (96%) delete mode 100644 src/trio/_core/_tests/test_multierror_scripts/__init__.py delete mode 100644 src/trio/_core/_tests/test_multierror_scripts/_common.py delete mode 100644 src/trio/_core/_tests/test_multierror_scripts/apport_excepthook.py delete mode 100644 src/trio/_core/_tests/test_multierror_scripts/simple_excepthook.py diff --git a/pyproject.toml b/pyproject.toml index cb24dd9eb8..a1b2585485 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -122,7 +122,6 @@ extend-exclude = [ [tool.ruff.per-file-ignores] 'src/trio/__init__.py' = ['F401'] 'src/trio/_core/__init__.py' = ['F401'] -'src/trio/_core/_tests/test_multierror_scripts/*' = ['F401'] 'src/trio/abc.py' = ['F401'] 'src/trio/lowlevel.py' = ['F401'] 'src/trio/socket.py' = ['F401'] @@ -230,9 +229,6 @@ showcontent = true branch = true source_pkgs = ["trio"] omit = [ - # These are run in subprocesses, but still don't work. We follow - # coverage's documentation to no avail. - "*/trio/_core/_tests/test_multierror_scripts/*", # Omit the generated files in trio/_core starting with _generated_ "*/trio/_core/_generated_*", ] diff --git a/src/trio/_core/_multierror.py b/src/trio/_core/_concat_tb.py similarity index 96% rename from src/trio/_core/_multierror.py rename to src/trio/_core/_concat_tb.py index 9a9fab9c86..15f2a54f7c 100644 --- a/src/trio/_core/_multierror.py +++ b/src/trio/_core/_concat_tb.py @@ -108,6 +108,9 @@ def controller(operation: tputil.ProxyOperation) -> Any | None: # type: ignore[ ) # Returns proxy to traceback +# this is used for collapsing single-exception multierrors when using +# `strict_exception_groups=False`. Once that is retired this function and its helper can +# be removed as well. def concat_tb( head: TracebackType | None, tail: TracebackType | None ) -> TracebackType | None: diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index bef0f76505..13581cfe87 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -35,11 +35,11 @@ from .._abc import Clock, Instrument from .._util import NoPublicConstructor, coroutine_or_error, final from ._asyncgens import AsyncGenerators +from ._concat_tb import concat_tb from ._entry_queue import EntryQueue, TrioToken from ._exceptions import Cancelled, RunFinishedError, TrioInternalError from ._instrumentation import Instruments from ._ki import LOCALS_KEY_KI_PROTECTION_ENABLED, KIManager, enable_ki_protection -from ._multierror import concat_tb from ._thread_cache import start_thread_soon from ._traps import ( Abort, diff --git a/src/trio/_core/_tests/test_multierror.py b/src/trio/_core/_tests/test_multierror.py index 67380df9c3..4e06fff8bc 100644 --- a/src/trio/_core/_tests/test_multierror.py +++ b/src/trio/_core/_tests/test_multierror.py @@ -1,19 +1,14 @@ from __future__ import annotations import gc -import os -import re -import subprocess import sys -from pathlib import Path from traceback import extract_tb from typing import TYPE_CHECKING, Callable, NoReturn, TypeVar import pytest from ..._core import open_nursery -from .._multierror import concat_tb -from .tutil import slow +from .._concat_tb import concat_tb if TYPE_CHECKING: from types import TracebackType @@ -135,66 +130,3 @@ def make_multi() -> NoReturn: finally: gc.set_debug(old_flags) gc.garbage.clear() - - -def assert_match_in_seq(pattern_list: list[str], string: str) -> None: - offset = 0 - print("looking for pattern matches...") - for pattern in pattern_list: - print("checking pattern:", pattern) - reobj = re.compile(pattern) - match = reobj.search(string, offset) - assert match is not None - offset = match.end() - - -def test_assert_match_in_seq() -> None: - assert_match_in_seq(["a", "b"], "xx a xx b xx") - assert_match_in_seq(["b", "a"], "xx b xx a xx") - with pytest.raises(AssertionError): - assert_match_in_seq(["a", "b"], "xx b xx a xx") - - -def run_script(name: str) -> subprocess.CompletedProcess[bytes]: - import trio - - trio_path = Path(trio.__file__).parent.parent - script_path = Path(__file__).parent / "test_multierror_scripts" / name - - env = dict(os.environ) - print("parent PYTHONPATH:", env.get("PYTHONPATH")) - pp = [] - if "PYTHONPATH" in env: # pragma: no cover - pp = env["PYTHONPATH"].split(os.pathsep) - pp.insert(0, str(trio_path)) - pp.insert(0, str(script_path.parent)) - env["PYTHONPATH"] = os.pathsep.join(pp) - print("subprocess PYTHONPATH:", env.get("PYTHONPATH")) - - cmd = [sys.executable, "-u", str(script_path)] - print("running:", cmd) - completed = subprocess.run( - cmd, env=env, stdout=subprocess.PIPE, stderr=subprocess.STDOUT - ) - print("process output:") - print(completed.stdout.decode("utf-8")) - return completed - - -@slow -@pytest.mark.skipif( - not Path("/usr/lib/python3/dist-packages/apport_python_hook.py").exists(), - reason="need Ubuntu with python3-apport installed", -) -def test_apport_excepthook_monkeypatch_interaction() -> None: - completed = run_script("apport_excepthook.py") - stdout = completed.stdout.decode("utf-8") - - # No warning - assert "custom sys.excepthook" not in stdout - - # Proper traceback - assert_match_in_seq( - ["--- 1 ---", "KeyError", "--- 2 ---", "ValueError"], - stdout, - ) diff --git a/src/trio/_core/_tests/test_multierror_scripts/__init__.py b/src/trio/_core/_tests/test_multierror_scripts/__init__.py deleted file mode 100644 index 26b5192ec7..0000000000 --- a/src/trio/_core/_tests/test_multierror_scripts/__init__.py +++ /dev/null @@ -1,3 +0,0 @@ -# This isn't really a package, everything in here is a standalone script. This -# __init__.py is just to fool setuptools' find = {namespaces = false} into -# actually installing the things. diff --git a/src/trio/_core/_tests/test_multierror_scripts/_common.py b/src/trio/_core/_tests/test_multierror_scripts/_common.py deleted file mode 100644 index 0c70df1840..0000000000 --- a/src/trio/_core/_tests/test_multierror_scripts/_common.py +++ /dev/null @@ -1,7 +0,0 @@ -# https://coverage.readthedocs.io/en/latest/subprocess.html -try: - import coverage -except ImportError: # pragma: no cover - pass -else: - coverage.process_startup() diff --git a/src/trio/_core/_tests/test_multierror_scripts/apport_excepthook.py b/src/trio/_core/_tests/test_multierror_scripts/apport_excepthook.py deleted file mode 100644 index a4217a3155..0000000000 --- a/src/trio/_core/_tests/test_multierror_scripts/apport_excepthook.py +++ /dev/null @@ -1,16 +0,0 @@ -# The apport_python_hook package is only installed as part of Ubuntu's system -# python, and not available in venvs. So before we can import it we have to -# make sure it's on sys.path. -import sys - -import _common # isort: split - -sys.path.append("/usr/lib/python3/dist-packages") -import apport_python_hook - -apport_python_hook.install() - -if sys.version_info < (3, 11): - from exceptiongroup import ExceptionGroup - -raise ExceptionGroup("", [KeyError("key_error"), ValueError("value_error")]) diff --git a/src/trio/_core/_tests/test_multierror_scripts/simple_excepthook.py b/src/trio/_core/_tests/test_multierror_scripts/simple_excepthook.py deleted file mode 100644 index 9a3fc8e4a7..0000000000 --- a/src/trio/_core/_tests/test_multierror_scripts/simple_excepthook.py +++ /dev/null @@ -1,24 +0,0 @@ -import _common # isort: split - -import sys - -if sys.version_info < (3, 11): - from exceptiongroup import ExceptionGroup - - -def exc1_fn() -> Exception: - try: - raise ValueError - except Exception as exc: - return exc - - -def exc2_fn() -> Exception: - try: - raise KeyError - except Exception as exc: - return exc - - -# This should be printed nicely, because Trio overrode sys.excepthook -raise ExceptionGroup("", [exc1_fn(), exc2_fn()]) diff --git a/src/trio/_core/_tests/test_run.py b/src/trio/_core/_tests/test_run.py index 8d8694ff19..b86992cc65 100644 --- a/src/trio/_core/_tests/test_run.py +++ b/src/trio/_core/_tests/test_run.py @@ -428,7 +428,7 @@ async def test_cancel_edge_cases() -> None: await sleep_forever() -async def test_cancel_scope_multierror_filtering() -> None: +async def test_cancel_scope_exceptiongroup_filtering() -> None: async def crasher() -> NoReturn: raise KeyError From 4d600bcfdc3e58116c003a59ec3e16f72970eb82 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Tue, 28 Nov 2023 16:27:59 +0100 Subject: [PATCH 06/20] edit some comments, remove test_ExceptionGroupNotHashable --- src/trio/_core/_concat_tb.py | 4 +-- src/trio/_core/_run.py | 6 ++-- src/trio/_core/_tests/test_multierror.py | 38 ++---------------------- 3 files changed, 8 insertions(+), 40 deletions(-) diff --git a/src/trio/_core/_concat_tb.py b/src/trio/_core/_concat_tb.py index 15f2a54f7c..088c690e8d 100644 --- a/src/trio/_core/_concat_tb.py +++ b/src/trio/_core/_concat_tb.py @@ -79,7 +79,7 @@ def copy_tb(base_tb: TracebackType, tb_next: TracebackType | None) -> TracebackT return new_tb finally: # delete references from locals to avoid creating cycles - # see test_MultiError_catch_doesnt_create_cyclic_garbage + # see test_ExceptionGroup_catch_doesnt_create_cyclic_garbage del new_tb, old_tb_frame else: @@ -108,7 +108,7 @@ def controller(operation: tputil.ProxyOperation) -> Any | None: # type: ignore[ ) # Returns proxy to traceback -# this is used for collapsing single-exception multierrors when using +# this is used for collapsing single-exception ExceptionGroups when using # `strict_exception_groups=False`. Once that is retired this function and its helper can # be removed as well. def concat_tb( diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index 13581cfe87..10e6754780 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -634,7 +634,7 @@ def __exit__( elif remaining_error_after_cancel_scope is exc: return False else: - # Copied verbatim from MultiErrorCatcher. Python doesn't + # Copied verbatim from the old MultiErrorCatcher. Python doesn't # allow us to encapsulate this __context__ fixup. old_context = remaining_error_after_cancel_scope.__context__ try: @@ -948,7 +948,7 @@ async def __aexit__( elif combined_error_from_nursery is exc: return False else: - # Copied verbatim from MultiErrorCatcher. Python doesn't + # Copied verbatim from the old MultiErrorCatcher. Python doesn't # allow us to encapsulate this __context__ fixup. old_context = combined_error_from_nursery.__context__ try: @@ -1079,7 +1079,7 @@ def _child_finished(self, task: Task, outcome: Outcome[Any]) -> None: async def _nested_child_finished( self, nested_child_exc: BaseException | None ) -> BaseException | None: - # Returns MultiError instance (or any exception if the nursery is in loose mode + # Returns ExceptionGroup instance (or any exception if the nursery is in loose mode # and there is just one contained exception) if there are pending exceptions if nested_child_exc is not None: self._add_exc(nested_child_exc) diff --git a/src/trio/_core/_tests/test_multierror.py b/src/trio/_core/_tests/test_multierror.py index 4e06fff8bc..b09487cc5b 100644 --- a/src/trio/_core/_tests/test_multierror.py +++ b/src/trio/_core/_tests/test_multierror.py @@ -3,11 +3,10 @@ import gc import sys from traceback import extract_tb -from typing import TYPE_CHECKING, Callable, NoReturn, TypeVar +from typing import TYPE_CHECKING, Callable, NoReturn import pytest -from ..._core import open_nursery from .._concat_tb import concat_tb if TYPE_CHECKING: @@ -16,25 +15,6 @@ if sys.version_info < (3, 11): from exceptiongroup import ExceptionGroup -E = TypeVar("E", bound=BaseException) - - -class NotHashableException(Exception): - code: int | None = None - - def __init__(self, code: int) -> None: - super().__init__() - self.code = code - - def __eq__(self, other: object) -> bool: - if not isinstance(other, NotHashableException): - return False - return self.code == other.code - - -async def raise_nothashable(code: int) -> NoReturn: - raise NotHashableException(code) - def raiser1() -> NoReturn: raiser1_2() @@ -93,19 +73,7 @@ def test_concat_tb() -> None: assert extract_tb(get_tb(raiser2)) == entries2 -async def test_ExceptionGroupNotHashable() -> None: - exc1 = NotHashableException(42) - exc2 = NotHashableException(4242) - exc3 = ValueError() - assert exc1 != exc2 - assert exc1 != exc3 - - with pytest.raises(ExceptionGroup): - async with open_nursery() as nursery: - nursery.start_soon(raise_nothashable, 42) - nursery.start_soon(raise_nothashable, 4242) - - +# I'm not sure this one can fail anymore? @pytest.mark.skipif( sys.implementation.name != "cpython", reason="Only makes sense with refcounting GC" ) @@ -121,7 +89,7 @@ def make_multi() -> NoReturn: try: gc.set_debug(gc.DEBUG_SAVEALL) with pytest.raises(ExceptionGroup) as excinfo: - # covers ExceptionGroupCatcher.__exit__ and _multierror.copy_tb + # covers ~~MultiErrorCatcher.__exit__ and~~ _concat_tb.copy_tb raise make_multi() for exc in excinfo.value.exceptions: assert isinstance(exc, (ValueError, KeyError)) From 8246daf5415c86622934982829be99bf29fe6d31 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Tue, 28 Nov 2023 16:43:23 +0100 Subject: [PATCH 07/20] docs: update some references to MultiError, remove section about maintaining backwards-compatibility from raising multierror's --- docs/source/reference-core.rst | 6 ------ docs/source/tutorial.rst | 2 +- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/docs/source/reference-core.rst b/docs/source/reference-core.rst index a6dc5b1f5a..fe187b36ce 100644 --- a/docs/source/reference-core.rst +++ b/docs/source/reference-core.rst @@ -768,12 +768,6 @@ inside the handler function(s) with the ``nonlocal`` keyword:: async with trio.open_nursery() as nursery: nursery.start_soon(broken1) -For reasons of backwards compatibility, nurseries raise ``trio.MultiError`` and -``trio.NonBaseMultiError`` which inherit from :exc:`BaseExceptionGroup` and -:exc:`ExceptionGroup`, respectively. Users should refrain from attempting to raise or -catch the Trio specific exceptions themselves, and treat them as if they were standard -:exc:`BaseExceptionGroup` or :exc:`ExceptionGroup` instances instead. - "Strict" versus "loose" ExceptionGroup semantics ++++++++++++++++++++++++++++++++++++++++++++++++ diff --git a/docs/source/tutorial.rst b/docs/source/tutorial.rst index 40eafd2833..a280bc839a 100644 --- a/docs/source/tutorial.rst +++ b/docs/source/tutorial.rst @@ -1168,7 +1168,7 @@ TODO: maybe a brief discussion of :exc:`KeyboardInterrupt` handling? you can stick anything inside a timeout block, even child tasks [show something like the first example but with a timeout – they - both get cancelled, the cancelleds get packed into a multierror, and + both get cancelled, the cancelleds get packed into an ExceptionGroup, and then the timeout block catches the cancelled] brief discussion of KI? From a7a5a71a32052c74307aa5dd298df4f514631309 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Tue, 28 Nov 2023 16:46:02 +0100 Subject: [PATCH 08/20] add newsfragment --- newsfragments/2891.deprecated.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 newsfragments/2891.deprecated.rst diff --git a/newsfragments/2891.deprecated.rst b/newsfragments/2891.deprecated.rst new file mode 100644 index 0000000000..59dba79360 --- /dev/null +++ b/newsfragments/2891.deprecated.rst @@ -0,0 +1 @@ +``MultiError`` has been fully removed, and all relevant trio functions now raise ExceptionGroups instead. This should not affect end users that have transitioned to using ``except*`` or catching ExceptionGroup/BaseExceptionGroup. From 5dd4b95e15628a0e807f41ee2bf8825776cc65b4 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Wed, 29 Nov 2023 13:41:23 +0100 Subject: [PATCH 09/20] update gc comments to my current state of understanding as to what causes what --- src/trio/_core/_concat_tb.py | 2 +- src/trio/_core/_run.py | 6 ++++-- src/trio/_core/_tests/test_multierror.py | 5 +++-- src/trio/_core/_tests/test_run.py | 2 ++ 4 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/trio/_core/_concat_tb.py b/src/trio/_core/_concat_tb.py index 088c690e8d..6fa1c6b4ba 100644 --- a/src/trio/_core/_concat_tb.py +++ b/src/trio/_core/_concat_tb.py @@ -79,7 +79,7 @@ def copy_tb(base_tb: TracebackType, tb_next: TracebackType | None) -> TracebackT return new_tb finally: # delete references from locals to avoid creating cycles - # see test_ExceptionGroup_catch_doesnt_create_cyclic_garbage + # see test_cancel_scope_exit_doesnt_create_cyclic_garbage del new_tb, old_tb_frame else: diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index 10e6754780..6b202a95cd 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -645,6 +645,7 @@ def __exit__( value.__context__ = old_context # delete references from locals to avoid creating cycles # see test_cancel_scope_exit_doesnt_create_cyclic_garbage + # Note: still relevant del remaining_error_after_cancel_scope, value, _, exc # deep magic to remove refs via f_locals locals() @@ -958,7 +959,7 @@ async def __aexit__( assert value is combined_error_from_nursery value.__context__ = old_context # delete references from locals to avoid creating cycles - # see test_simple_cancel_scope_usage_doesnt_create_cyclic_garbage + # see test_cancel_scope_exit_doesnt_create_cyclic_garbage del _, combined_error_from_nursery, value, new_exc # make sure these raise errors in static analysis if called @@ -1094,6 +1095,7 @@ def aborted(raise_cancel: _core.RaiseCancelT) -> Abort: exn = capture(raise_cancel).error if not isinstance(exn, Cancelled): self._add_exc(exn) + # see test_cancel_scope_exit_doesnt_create_cyclic_garbage del exn # prevent cyclic garbage creation return Abort.FAILED @@ -1120,7 +1122,7 @@ def aborted(raise_cancel: _core.RaiseCancelT) -> Abort: ) finally: # avoid a garbage cycle - # (see test_nursery_cancel_doesnt_create_cyclic_garbage) + # (see test_locals_destroyed_promptly_on_cancel) del self._pending_excs return None diff --git a/src/trio/_core/_tests/test_multierror.py b/src/trio/_core/_tests/test_multierror.py index b09487cc5b..d2d84eddfc 100644 --- a/src/trio/_core/_tests/test_multierror.py +++ b/src/trio/_core/_tests/test_multierror.py @@ -73,7 +73,8 @@ def test_concat_tb() -> None: assert extract_tb(get_tb(raiser2)) == entries2 -# I'm not sure this one can fail anymore? +# I haven't managed to get this one to fail, despite removing the `del` from +# _concat_tb.copy_tb (on a platform where the statement is executed) @pytest.mark.skipif( sys.implementation.name != "cpython", reason="Only makes sense with refcounting GC" ) @@ -83,13 +84,13 @@ def test_ExceptionGroup_catch_doesnt_create_cyclic_garbage() -> None: old_flags = gc.get_debug() def make_multi() -> NoReturn: - # make_tree creates cycles itself, so a simple raise ExceptionGroup("", [get_exc(raiser1), get_exc(raiser2)]) try: gc.set_debug(gc.DEBUG_SAVEALL) with pytest.raises(ExceptionGroup) as excinfo: # covers ~~MultiErrorCatcher.__exit__ and~~ _concat_tb.copy_tb + # TODO: is the above comment true anymore? as this no longer uses MultiError.catch raise make_multi() for exc in excinfo.value.exceptions: assert isinstance(exc, (ValueError, KeyError)) diff --git a/src/trio/_core/_tests/test_run.py b/src/trio/_core/_tests/test_run.py index 3081ea2bd8..bab3a4a320 100644 --- a/src/trio/_core/_tests/test_run.py +++ b/src/trio/_core/_tests/test_run.py @@ -2293,6 +2293,8 @@ async def test_cancel_scope_deadline_duplicates() -> None: await sleep(0.01) +# I don't know if this one can fail anymore, the `del` next to the comment that used to +# refer to this only seems to break test_cancel_scope_exit_doesnt_create_cyclic_garbage @pytest.mark.skipif( sys.implementation.name != "cpython", reason="Only makes sense with refcounting GC" ) From 9c1400c3ee1b4f7f95b86cccbad8ffa385c025c2 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Sat, 16 Dec 2023 13:26:13 +0100 Subject: [PATCH 10/20] move 'collapsible' to __notes__, clean up some stray unused-ignores, update comments, add test_nursery_loose_exception_groups, add string for the exceptiongroup --- src/trio/_core/_run.py | 20 ++++++++++---------- src/trio/_core/_tests/test_multierror.py | 4 ++-- src/trio/_core/_tests/test_run.py | 18 ++++++++++++++++++ 3 files changed, 30 insertions(+), 12 deletions(-) diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index 568a58de93..573d6b3189 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -209,7 +209,7 @@ def collapse_exception_group( if ( len(exceptions) == 1 and isinstance(excgroup, BaseExceptionGroup) - and "collapsible" in str(excgroup) + and "collapsible" in getattr(excgroup, "__notes__", ()) ): exceptions[0].__traceback__ = concat_tb( excgroup.__traceback__, exceptions[0].__traceback__ @@ -1128,10 +1128,12 @@ def aborted(raise_cancel: _core.RaiseCancelT) -> Abort: try: if not self._strict_exception_groups and len(self._pending_excs) == 1: return self._pending_excs[0] - return BaseExceptionGroup( - "collapsible" if not self._strict_exception_groups else "TODO", - self._pending_excs, + exc = BaseExceptionGroup( + "ExceptionGroup from trio nursery", self._pending_excs ) + if not self._strict_exception_groups: + exc.add_note("collapsible") + return exc finally: # avoid a garbage cycle # (see test_locals_destroyed_promptly_on_cancel) @@ -1777,8 +1779,7 @@ async def python_wrapper(orig_coro: Awaitable[RetT]) -> RetT: self.instruments.call("task_spawned", task) # Special case: normally next_send should be an Outcome, but for the # very first send we have to send a literal unboxed None. - # TODO: remove [unused-ignore] when Outcome is typed - self.reschedule(task, None) # type: ignore[arg-type, unused-ignore] + self.reschedule(task, None) # type: ignore[arg-type] return task def task_exited(self, task: Task, outcome: Outcome[Any]) -> None: @@ -2624,18 +2625,17 @@ def unrolled_run( # protocol of unwrapping whatever outcome gets sent in. # Instead, we'll arrange to throw `exc` in directly, # which works for at least asyncio and curio. - # TODO: remove [unused-ignore] when Outcome is typed - runner.reschedule(task, exc) # type: ignore[arg-type, unused-ignore] + runner.reschedule(task, exc) # type: ignore[arg-type] task._next_send_fn = task.coro.throw # prevent long-lived reference - # TODO: develop test for this deletion + # There's no regression test checking the necessity of this del msg if "after_task_step" in runner.instruments: runner.instruments.call("after_task_step", task) del GLOBAL_RUN_CONTEXT.task # prevent long-lived references - # TODO: develop test for these deletions + # There's no regression test checking the necessity of this del task, next_send, next_send_fn except GeneratorExit: diff --git a/src/trio/_core/_tests/test_multierror.py b/src/trio/_core/_tests/test_multierror.py index d2d84eddfc..8957a581a5 100644 --- a/src/trio/_core/_tests/test_multierror.py +++ b/src/trio/_core/_tests/test_multierror.py @@ -73,8 +73,8 @@ def test_concat_tb() -> None: assert extract_tb(get_tb(raiser2)) == entries2 -# I haven't managed to get this one to fail, despite removing the `del` from -# _concat_tb.copy_tb (on a platform where the statement is executed) +# Unclear if this can still fail, removing the `del` from _concat_tb.copy_tb does not seem +# to trigger it (on a platform where the `del` is executed) @pytest.mark.skipif( sys.implementation.name != "cpython", reason="Only makes sense with refcounting GC" ) diff --git a/src/trio/_core/_tests/test_run.py b/src/trio/_core/_tests/test_run.py index 4558ad190b..7f1d91a6ce 100644 --- a/src/trio/_core/_tests/test_run.py +++ b/src/trio/_core/_tests/test_run.py @@ -2561,6 +2561,24 @@ async def test_nursery_strict_exception_groups() -> None: assert exc.value.exceptions[0].args == ("foo",) +async def test_nursery_loose_exception_groups() -> None: + """Test that loose exception groups can be enabled on a per-nursery basis.""" + + async def raise_error() -> NoReturn: + raise RuntimeError("test error") + + with pytest.raises(ExceptionGroup) as exc: # noqa: PT012 + async with _core.open_nursery(strict_exception_groups=False) as nursery: + nursery.start_soon(raise_error) + nursery.start_soon(raise_error) + + assert exc.value.__notes__ == ["collapsible"] + assert len(exc.value.exceptions) == 2 + for subexc in exc.value.exceptions: + assert type(subexc) is RuntimeError + assert subexc.args == ("test error",) + + async def test_nursery_collapse_strict() -> None: """ Test that a single exception from a nested nursery with strict semantics doesn't get From 907a2c57bf1be20261279aa949dea169ef003634 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Sat, 16 Dec 2023 13:28:42 +0100 Subject: [PATCH 11/20] update exceptiongroup message, test for it with a match --- src/trio/_core/_run.py | 2 +- src/trio/_core/_tests/test_run.py | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index 573d6b3189..267138309c 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -1129,7 +1129,7 @@ def aborted(raise_cancel: _core.RaiseCancelT) -> Abort: if not self._strict_exception_groups and len(self._pending_excs) == 1: return self._pending_excs[0] exc = BaseExceptionGroup( - "ExceptionGroup from trio nursery", self._pending_excs + "Exceptions from Trio nursery", self._pending_excs ) if not self._strict_exception_groups: exc.add_note("collapsible") diff --git a/src/trio/_core/_tests/test_run.py b/src/trio/_core/_tests/test_run.py index 7f1d91a6ce..8a42bebb69 100644 --- a/src/trio/_core/_tests/test_run.py +++ b/src/trio/_core/_tests/test_run.py @@ -2528,7 +2528,7 @@ async def main() -> NoReturn: async with _core.open_nursery(): raise Exception("foo") - with pytest.raises(ExceptionGroup) as exc: + with pytest.raises(ExceptionGroup, match="^Exceptions from Trio nursery$") as exc: _core.run(main, strict_exception_groups=True) assert len(exc.value.exceptions) == 1 @@ -2567,7 +2567,9 @@ async def test_nursery_loose_exception_groups() -> None: async def raise_error() -> NoReturn: raise RuntimeError("test error") - with pytest.raises(ExceptionGroup) as exc: # noqa: PT012 + with pytest.raises( # noqa: PT012 # multiple statements + ExceptionGroup, match="^Exceptions from Trio nursery$" + ) as exc: async with _core.open_nursery(strict_exception_groups=False) as nursery: nursery.start_soon(raise_error) nursery.start_soon(raise_error) From 3319a0120b7d1f92410c08623d136b6b3323508e Mon Sep 17 00:00:00 2001 From: jakkdl Date: Sat, 16 Dec 2023 13:34:37 +0100 Subject: [PATCH 12/20] fix expected message --- src/trio/_core/_tests/test_run.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/trio/_core/_tests/test_run.py b/src/trio/_core/_tests/test_run.py index 8a42bebb69..2f8255f463 100644 --- a/src/trio/_core/_tests/test_run.py +++ b/src/trio/_core/_tests/test_run.py @@ -2528,7 +2528,9 @@ async def main() -> NoReturn: async with _core.open_nursery(): raise Exception("foo") - with pytest.raises(ExceptionGroup, match="^Exceptions from Trio nursery$") as exc: + with pytest.raises( + ExceptionGroup, match="^Exceptions from Trio nursery \\(1 sub-exception\\)$" + ) as exc: _core.run(main, strict_exception_groups=True) assert len(exc.value.exceptions) == 1 @@ -2568,7 +2570,7 @@ async def raise_error() -> NoReturn: raise RuntimeError("test error") with pytest.raises( # noqa: PT012 # multiple statements - ExceptionGroup, match="^Exceptions from Trio nursery$" + ExceptionGroup, match="^Exceptions from Trio nursery \\(2 sub-exceptions\\)$" ) as exc: async with _core.open_nursery(strict_exception_groups=False) as nursery: nursery.start_soon(raise_error) From 9941762edba6108517d87af6e1066ffa4143fa16 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Sat, 16 Dec 2023 13:57:30 +0100 Subject: [PATCH 13/20] fix mypy (this should be checked by pre-commit...) --- src/trio/_core/_run.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index 267138309c..759f1bb530 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -1128,12 +1128,12 @@ def aborted(raise_cancel: _core.RaiseCancelT) -> Abort: try: if not self._strict_exception_groups and len(self._pending_excs) == 1: return self._pending_excs[0] - exc = BaseExceptionGroup( + exception = BaseExceptionGroup( "Exceptions from Trio nursery", self._pending_excs ) if not self._strict_exception_groups: - exc.add_note("collapsible") - return exc + exception.add_note("collapsible") + return exception finally: # avoid a garbage cycle # (see test_locals_destroyed_promptly_on_cancel) From 6977353b0c7b226d2e725ac1cf581d679c7f20b4 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Mon, 18 Dec 2023 13:28:28 +0100 Subject: [PATCH 14/20] add test for loose exceptiongroup collapsing, rename test_multierror --- .../_tests/{test_multierror.py => test_exceptiongroup_gc.py} | 0 src/trio/_core/_tests/test_run.py | 4 ++++ 2 files changed, 4 insertions(+) rename src/trio/_core/_tests/{test_multierror.py => test_exceptiongroup_gc.py} (100%) diff --git a/src/trio/_core/_tests/test_multierror.py b/src/trio/_core/_tests/test_exceptiongroup_gc.py similarity index 100% rename from src/trio/_core/_tests/test_multierror.py rename to src/trio/_core/_tests/test_exceptiongroup_gc.py diff --git a/src/trio/_core/_tests/test_run.py b/src/trio/_core/_tests/test_run.py index 2f8255f463..474cd610c3 100644 --- a/src/trio/_core/_tests/test_run.py +++ b/src/trio/_core/_tests/test_run.py @@ -2569,6 +2569,10 @@ async def test_nursery_loose_exception_groups() -> None: async def raise_error() -> NoReturn: raise RuntimeError("test error") + with pytest.raises(ValueError, match="^test error$"): + async with _core.open_nursery(strict_exception_groups=False) as nursery: + nursery.start_soon(raise_error) + with pytest.raises( # noqa: PT012 # multiple statements ExceptionGroup, match="^Exceptions from Trio nursery \\(2 sub-exceptions\\)$" ) as exc: From bd72a32bada0f589d1376fcc48ae5b9f876f7159 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Mon, 18 Dec 2023 13:38:18 +0100 Subject: [PATCH 15/20] fix test --- src/trio/_core/_tests/test_run.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/trio/_core/_tests/test_run.py b/src/trio/_core/_tests/test_run.py index 474cd610c3..5416dc99c9 100644 --- a/src/trio/_core/_tests/test_run.py +++ b/src/trio/_core/_tests/test_run.py @@ -2569,7 +2569,7 @@ async def test_nursery_loose_exception_groups() -> None: async def raise_error() -> NoReturn: raise RuntimeError("test error") - with pytest.raises(ValueError, match="^test error$"): + with pytest.raises(RuntimeError, match="^test error$"): async with _core.open_nursery(strict_exception_groups=False) as nursery: nursery.start_soon(raise_error) From 0439e6648dee43c16c940fbbd862d3a096b440d1 Mon Sep 17 00:00:00 2001 From: John Litborn <11260241+jakkdl@users.noreply.github.com> Date: Tue, 19 Dec 2023 11:51:16 +0100 Subject: [PATCH 16/20] Apply suggestions from code review Co-authored-by: EXPLOSION --- src/trio/_core/_concat_tb.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/trio/_core/_concat_tb.py b/src/trio/_core/_concat_tb.py index 6fa1c6b4ba..de0877832b 100644 --- a/src/trio/_core/_concat_tb.py +++ b/src/trio/_core/_concat_tb.py @@ -20,7 +20,7 @@ # On CPython, we use ctypes. On PyPy, we use "transparent proxies". # # Jinja2 is a useful source of inspiration: -# https://github.com/pallets/jinja/blob/master/jinja2/debug.py +# https://github.com/pallets/jinja/blob/main/src/jinja2/debug.py try: import tputil @@ -53,7 +53,7 @@ def copy_tb(base_tb: TracebackType, tb_next: TracebackType | None) -> TracebackT assert new_tb is not None c_new_tb = CTraceback.from_address(id(new_tb)) - # At the C level, tb_next either pointer to the next traceback or is + # At the C level, tb_next either points to the next traceback or is # NULL. c_void_p and the .tb_next accessor both convert NULL to None, # but we shouldn't DECREF None just because we assigned to a NULL # pointer! Here we know that our new traceback has only 1 frame in it, @@ -101,7 +101,7 @@ def controller(operation: tputil.ProxyOperation) -> Any | None: # type: ignore[ and operation.args[0] == "tb_next" ): # pragma: no cover return tb_next - return operation.delegate() # Deligate is reverting to original behaviour + return operation.delegate() # Delegate is reverting to original behaviour return cast( TracebackType, tputil.make_proxy(controller, type(base_tb), base_tb) From e19f34127e974a0450861bc2a22c0b91d462a526 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Tue, 19 Dec 2023 12:01:13 +0100 Subject: [PATCH 17/20] update comments after review from a5rocks --- src/trio/_core/_concat_tb.py | 3 ++- src/trio/_core/_run.py | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/trio/_core/_concat_tb.py b/src/trio/_core/_concat_tb.py index de0877832b..14f31f6184 100644 --- a/src/trio/_core/_concat_tb.py +++ b/src/trio/_core/_concat_tb.py @@ -85,7 +85,8 @@ def copy_tb(base_tb: TracebackType, tb_next: TracebackType | None) -> TracebackT else: # http://doc.pypy.org/en/latest/objspace-proxies.html def copy_tb(base_tb: TracebackType, tb_next: TracebackType | None) -> TracebackType: - # tputil.ProxyOperation is PyPy-only, but we run mypy on CPython + # tputil.ProxyOperation is PyPy-only, and there's no way to specify + # cpython/pypy in current type checkers. def controller(operation: tputil.ProxyOperation) -> Any | None: # type: ignore[no-any-unimported] # Rationale for pragma: I looked fairly carefully and tried a few # things, and AFAICT it's not actually possible to get any diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index 759f1bb530..0587e04417 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -2628,14 +2628,14 @@ def unrolled_run( runner.reschedule(task, exc) # type: ignore[arg-type] task._next_send_fn = task.coro.throw # prevent long-lived reference - # There's no regression test checking the necessity of this + # TODO: develop test for this deletion del msg if "after_task_step" in runner.instruments: runner.instruments.call("after_task_step", task) del GLOBAL_RUN_CONTEXT.task # prevent long-lived references - # There's no regression test checking the necessity of this + # TODO: develop test for this deletion del task, next_send, next_send_fn except GeneratorExit: From b0eb6b471feb0c1554a74326deb3f89d325c7ffd Mon Sep 17 00:00:00 2001 From: jakkdl Date: Wed, 20 Dec 2023 12:17:58 +0100 Subject: [PATCH 18/20] make the note more verbose --- src/trio/_core/_run.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index 0587e04417..1e37a7dbb2 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -95,6 +95,9 @@ def __call__( # Passed as a sentinel _NO_SEND: Final[Outcome[Any]] = cast("Outcome[Any]", object()) +# Used to track if an exceptiongroup can be collapsed +NONSTRICT_EXCEPTIONGROUP_NOTE = 'This is a "loose" ExceptionGroup, and may be collapsed by Trio if it only contains one exception - typically after `Cancelled` has been stripped from it. Note this has consequences for exception handling, and strict_exception_groups=True is recommended.' + @final class _NoStatus(metaclass=NoPublicConstructor): @@ -209,7 +212,7 @@ def collapse_exception_group( if ( len(exceptions) == 1 and isinstance(excgroup, BaseExceptionGroup) - and "collapsible" in getattr(excgroup, "__notes__", ()) + and NONSTRICT_EXCEPTIONGROUP_NOTE in getattr(excgroup, "__notes__", ()) ): exceptions[0].__traceback__ = concat_tb( excgroup.__traceback__, exceptions[0].__traceback__ @@ -1132,7 +1135,7 @@ def aborted(raise_cancel: _core.RaiseCancelT) -> Abort: "Exceptions from Trio nursery", self._pending_excs ) if not self._strict_exception_groups: - exception.add_note("collapsible") + exception.add_note(NONSTRICT_EXCEPTIONGROUP_NOTE) return exception finally: # avoid a garbage cycle From f433fdbdbc4b18b9535b06e9bd11b2cd92e23647 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Wed, 20 Dec 2023 16:30:08 +0100 Subject: [PATCH 19/20] fix the test --- src/trio/_core/_tests/test_run.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/trio/_core/_tests/test_run.py b/src/trio/_core/_tests/test_run.py index 5416dc99c9..a685fdeb94 100644 --- a/src/trio/_core/_tests/test_run.py +++ b/src/trio/_core/_tests/test_run.py @@ -2580,7 +2580,7 @@ async def raise_error() -> NoReturn: nursery.start_soon(raise_error) nursery.start_soon(raise_error) - assert exc.value.__notes__ == ["collapsible"] + assert exc.value.__notes__ == [_core._run.NONSTRICT_EXCEPTIONGROUP_NOTE] assert len(exc.value.exceptions) == 2 for subexc in exc.value.exceptions: assert type(subexc) is RuntimeError From 3e485dcb65527e5e518b9c76437b0c3809b430eb Mon Sep 17 00:00:00 2001 From: jakkdl Date: Thu, 21 Dec 2023 11:42:46 +0100 Subject: [PATCH 20/20] update comment --- src/trio/_core/_tests/test_run.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/trio/_core/_tests/test_run.py b/src/trio/_core/_tests/test_run.py index a685fdeb94..71b95c8dcd 100644 --- a/src/trio/_core/_tests/test_run.py +++ b/src/trio/_core/_tests/test_run.py @@ -2386,6 +2386,7 @@ async def test_cancel_scope_deadline_duplicates() -> None: # I don't know if this one can fail anymore, the `del` next to the comment that used to # refer to this only seems to break test_cancel_scope_exit_doesnt_create_cyclic_garbage +# We're keeping it for now to cover Outcome and potential future refactoring @pytest.mark.skipif( sys.implementation.name != "cpython", reason="Only makes sense with refcounting GC" )