Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change strict_exception_groups default to True #2886

Merged
merged 30 commits into from
Jan 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
cfc0755
change strict_exception_groups default to True
jakkdl Nov 24, 2023
84fb527
get rid of the last vestiges of _assert_raises
jakkdl Nov 24, 2023
c4548bd
Merge remote-tracking branch 'origin/master' into remove_multierror
jakkdl Nov 28, 2023
2437d1e
fix check for @final
jakkdl Nov 28, 2023
dcadc87
add typeguard to ExpectedExceptionGroup.matches
jakkdl Nov 29, 2023
a39732f
update comments/tests where I was confused about lack of ExceptionGro…
jakkdl Dec 11, 2023
669c345
Merge branch 'master' into remove_multierror
CoolCat467 Dec 13, 2023
f7e0b8f
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Dec 13, 2023
38f47a5
Fix ruff issues
CoolCat467 Dec 13, 2023
d455a45
Merge remote-tracking branch 'origin/master' into remove_multierror
jakkdl Dec 22, 2023
4232be2
Merge remote-tracking branch 'origin/master' into remove_multierror
jakkdl Jan 7, 2024
5caf49f
replace all instances of ExpectedExceptionGroup with RaisesGroup
jakkdl Jan 7, 2024
f50386c
gone through the whole PR and cleaned it up
jakkdl Jan 7, 2024
5acae48
fix tests: remove stray pytest import
jakkdl Jan 7, 2024
a33bea6
fix coverage
jakkdl Jan 7, 2024
295e5af
proposed fix for run_process raising exceptiongroup
jakkdl Jan 8, 2024
0089248
add test for TrioInternalError from run_process
jakkdl Jan 8, 2024
26a9c62
Apply suggestions from code review
jakkdl Jan 8, 2024
a93a185
rephrase newsfragment, and add rst cross-references
jakkdl Jan 8, 2024
b3d5aa3
update docstrings
jakkdl Jan 8, 2024
c00a05f
add comment with TODO about rewriting strict_exception_groups section
jakkdl Jan 8, 2024
4b393ed
parametrize run/nursery strictness tests
jakkdl Jan 8, 2024
e64d508
revert reraising exceptiongroup as a TrioInternalError, mention that …
jakkdl Jan 11, 2024
0547313
Merge remote-tracking branch 'origin/master' into remove_multierror
jakkdl Jan 11, 2024
22e4765
fix test_run_process_internal_error, add test_bad_deliver_cancel
jakkdl Jan 11, 2024
79754b8
fix docstring
jakkdl Jan 11, 2024
506e211
Merge branch 'master' into remove_multierror
CoolCat467 Jan 19, 2024
ea48cf0
revert accidental removal of empty line
jakkdl Jan 21, 2024
3f519f8
Add a bunch of words to the newsfragment
jakkdl Jan 21, 2024
0b45ec9
Merge remote-tracking branch 'origin/master' into remove_multierror
jakkdl Jan 21, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions docs/source/reference-core.rst
Original file line number Diff line number Diff line change
Expand Up @@ -768,9 +768,14 @@ inside the handler function(s) with the ``nonlocal`` keyword::
async with trio.open_nursery() as nursery:
nursery.start_soon(broken1)

.. _strict_exception_groups:

"Strict" versus "loose" ExceptionGroup semantics
++++++++++++++++++++++++++++++++++++++++++++++++

..
TODO: rewrite this (and possible other) sections from the new strict-by-default perspective, under the heading "Deprecated: non-strict ExceptionGroups" - to explain that it only exists for backwards-compatibility, will be removed in future, and that we recommend against it for all new code.

Ideally, in some abstract sense we'd want everything that *can* raise an
`ExceptionGroup` to *always* raise an `ExceptionGroup` (rather than, say, a single
`ValueError`). Otherwise, it would be easy to accidentally write something like ``except
Expand All @@ -796,9 +801,10 @@ to set the default behavior for any nursery in your program that doesn't overrid
wrapping, so you'll get maximum compatibility with code that was written to
support older versions of Trio.

To maintain backwards compatibility, the default is ``strict_exception_groups=False``.
The default will eventually change to ``True`` in a future version of Trio, once
Python 3.11 and later versions are in wide use.
The default is set to ``strict_exception_groups=True``, in line with the default behaviour
of ``TaskGroup`` in asyncio and anyio. We've also found that non-strict mode makes it
too easy to neglect the possibility of several exceptions being raised concurrently,
causing nasty latent bugs when errors occur under load.

.. _exceptiongroup: https://pypi.org/project/exceptiongroup/

Expand Down
10 changes: 10 additions & 0 deletions newsfragments/2786.breaking.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
The :ref:`strict_exception_groups <strict_exception_groups>` parameter now defaults to `True` in `trio.run` and `trio.lowlevel.start_guest_run`. `trio.open_nursery` still defaults to the same value as was specified in `trio.run`/`trio.lowlevel.start_guest_run`, but if you didn't specify it there then all subsequent calls to `trio.open_nursery` will change.
This is unfortunately very tricky to change with a deprecation period, as raising a `DeprecationWarning` whenever :ref:`strict_exception_groups <strict_exception_groups>` is not specified would raise a lot of unnecessary warnings.

Notable side effects of changing code to run with ``strict_exception_groups==True``

* If an iterator raises `StopAsyncIteration` or `StopIteration` inside a nursery, then python will not recognize wrapped instances of those for stopping iteration.
* `trio.run_process` is now documented that it can raise an `ExceptionGroup`. It previously could do this in very rare circumstances, but with :ref:`strict_exception_groups <strict_exception_groups>` set to `True` it will now do so whenever exceptions occur in ``deliver_cancel`` or with problems communicating with the subprocess.

* Errors in opening the process is now done outside the internal nursery, so if code previously ran with ``strict_exception_groups=True`` there are cases now where an `ExceptionGroup` is *no longer* added.
* `trio.TrioInternalError` ``.__cause__`` might be wrapped in one or more `ExceptionGroups <ExceptionGroup>`
1 change: 1 addition & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ extend-ignore = [
'E402', # module-import-not-at-top-of-file (usually OS-specific)
'E501', # line-too-long
'SIM117', # multiple-with-statements (messes up lots of context-based stuff and looks bad)
'PT012', # multiple statements in pytest.raises block
]

include = ["*.py", "*.pyi", "**/pyproject.toml"]
Expand Down
20 changes: 11 additions & 9 deletions src/trio/_core/_run.py
Original file line number Diff line number Diff line change
Expand Up @@ -928,7 +928,7 @@ class NurseryManager:

"""

strict_exception_groups: bool = attr.ib(default=False)
strict_exception_groups: bool = attr.ib(default=True)

@enable_ki_protection
async def __aenter__(self) -> Nursery:
Expand Down Expand Up @@ -995,9 +995,10 @@ def open_nursery(
have exited.

Args:
strict_exception_groups (bool): If true, even a single raised exception will be
wrapped in an exception group. This will eventually become the default
behavior. If not specified, uses the value passed to :func:`run`.
strict_exception_groups (bool): Unless set to False, even a single raised exception
will be wrapped in an exception group. If not specified, uses the value passed
to :func:`run`, which defaults to true. Setting it to False will be deprecated
and ultimately removed in a future version of Trio.

"""
if strict_exception_groups is None:
Expand Down Expand Up @@ -2162,7 +2163,7 @@ def run(
clock: Clock | None = None,
instruments: Sequence[Instrument] = (),
restrict_keyboard_interrupt_to_checkpoints: bool = False,
strict_exception_groups: bool = False,
strict_exception_groups: bool = True,
) -> RetT:
"""Run a Trio-flavored async function, and return the result.

Expand Down Expand Up @@ -2219,9 +2220,10 @@ def run(
main thread (this is a Python limitation), or if you use
:func:`open_signal_receiver` to catch SIGINT.

strict_exception_groups (bool): If true, nurseries will always wrap even a single
raised exception in an exception group. This can be overridden on the level of
individual nurseries. This will eventually become the default behavior.
strict_exception_groups (bool): Unless set to False, nurseries will always wrap
even a single raised exception in an exception group. This can be overridden
on the level of individual nurseries. Setting it to False will be deprecated
and ultimately removed in a future version of Trio.

Returns:
Whatever ``async_fn`` returns.
Expand Down Expand Up @@ -2279,7 +2281,7 @@ def start_guest_run(
clock: Clock | None = None,
instruments: Sequence[Instrument] = (),
restrict_keyboard_interrupt_to_checkpoints: bool = False,
strict_exception_groups: bool = False,
strict_exception_groups: bool = True,
) -> None:
"""Start a "guest" run of Trio on top of some other "host" event loop.

Expand Down
10 changes: 8 additions & 2 deletions src/trio/_core/_tests/test_ki.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
import outcome
import pytest

from trio.testing import RaisesGroup

try:
from async_generator import async_generator, yield_
except ImportError: # pragma: no cover
Expand Down Expand Up @@ -293,7 +295,8 @@ async def check_unprotected_kill() -> None:
nursery.start_soon(sleeper, "s2", record_set)
nursery.start_soon(raiser, "r1", record_set)

with pytest.raises(KeyboardInterrupt):
# raises inside a nursery, so the KeyboardInterrupt is wrapped in an ExceptionGroup
with RaisesGroup(KeyboardInterrupt):
_core.run(check_unprotected_kill)
assert record_set == {"s1 ok", "s2 ok", "r1 raise ok"}

Expand All @@ -309,7 +312,8 @@ async def check_protected_kill() -> None:
nursery.start_soon(_core.enable_ki_protection(raiser), "r1", record_set)
# __aexit__ blocks, and then receives the KI

with pytest.raises(KeyboardInterrupt):
# raises inside a nursery, so the KeyboardInterrupt is wrapped in an ExceptionGroup
with RaisesGroup(KeyboardInterrupt):
_core.run(check_protected_kill)
assert record_set == {"s1 ok", "s2 ok", "r1 cancel ok"}

Expand All @@ -331,6 +335,7 @@ def kill_during_shutdown() -> None:

token.run_sync_soon(kill_during_shutdown)

# no nurseries involved, so the KeyboardInterrupt isn't wrapped
with pytest.raises(KeyboardInterrupt):
_core.run(check_kill_during_shutdown)

Expand All @@ -344,6 +349,7 @@ def before_run(self) -> None:
async def main_1() -> None:
await _core.checkpoint()

# no nurseries involved, so the KeyboardInterrupt isn't wrapped
with pytest.raises(KeyboardInterrupt):
_core.run(main_1, instruments=[InstrumentOfDeath()])

Expand Down
Loading