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

Document strict ExceptionGroups as the natural default #2984

Merged
merged 2 commits into from
Apr 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions docs/source/conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ def setup(app: Sphinx) -> None:
"outcome": ("https://outcome.readthedocs.io/en/latest/", None),
"pyopenssl": ("https://www.pyopenssl.org/en/stable/", None),
"sniffio": ("https://sniffio.readthedocs.io/en/latest/", None),
"trio-util": ("https://trio-util.readthedocs.io/en/latest/", None),
}


Expand Down
125 changes: 86 additions & 39 deletions docs/source/reference-core.rst
Original file line number Diff line number Diff line change
Expand Up @@ -639,8 +639,7 @@ crucial things to keep in mind:
* The nursery is marked as "closed", meaning that no new tasks can
be started inside it.

* Any unhandled exceptions are re-raised inside the parent task. If
there are multiple exceptions, then they're collected up into a
* Any unhandled exceptions are re-raised inside the parent task, grouped into a
single :exc:`BaseExceptionGroup` or :exc:`ExceptionGroup` exception.

Since all tasks are descendents of the initial task, one consequence
Expand Down Expand Up @@ -733,9 +732,9 @@ If you want to reraise exceptions, or raise new ones, you can do so, but be awar
exceptions raised in ``except*`` sections will be raised together in a new exception
group.

But what if you can't use ``except*`` just yet? Well, for that there is the handy
exceptiongroup_ library which lets you approximate this behavior with exception handler
callbacks::
But what if you can't use Python 3.11, and therefore ``except*``, just yet?
The same exceptiongroup_ library which backports `ExceptionGroup` also lets
you approximate this behavior with exception handler callbacks::

from exceptiongroup import catch

Expand Down Expand Up @@ -768,43 +767,91 @@ inside the handler function(s) with the ``nonlocal`` keyword::
async with trio.open_nursery() as nursery:
nursery.start_soon(broken1)

.. _strict_exception_groups:
.. _handling_exception_groups:

Designing for multiple errors
+++++++++++++++++++++++++++++

Structured concurrency is still a young design pattern, but there are a few patterns
we've identified for how you (or your users) might want to handle groups of exceptions.
Note that the final pattern, simply raising an `ExceptionGroup`, is the most common -
and nurseries automatically do that for you.

**First**, you might want to 'defer to' a particular exception type, raising just that if
there is any such instance in the group. For example: `KeyboardInterrupt` has a clear
meaning for the surrounding code, could reasonably take priority over errors of other
types, and whether you have one or several of them doesn't really matter.

This pattern can often be implemented using a decorator or a context manager, such
as :func:`trio_util.multi_error_defer_to` or :func:`trio_util.defer_to_cancelled`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(unfortunately trio-util itself is not caught up with the MultiError/ExceptionGroup transition)

Note however that re-raising a 'leaf' exception will discard whatever part of the
traceback is attached to the `ExceptionGroup` itself, so we don't recommend this for
errors that will be presented to humans.

"Strict" versus "loose" ExceptionGroup semantics
++++++++++++++++++++++++++++++++++++++++++++++++
..
TODO: what about `Cancelled`? It's relevantly similar to `KeyboardInterrupt`,
but if you have multiple Cancelleds destined for different scopes, it seems
like it might be bad to abandon all-but-one of those - we might try to execute
some more code which then itself gets cancelled again, and incur more cleanup.
That's only a mild inefficiency though, and the semantics are fine overall.

**Second**, you might want to treat the concurrency inside your code as an implementation
detail which is hidden from your users - for example, abstracting a protocol which
involves sending and receiving data to a simple receive-only interface, or implementing
a context manager which maintains some background tasks for the length of the
``async with`` block.

The simple option here is to ``raise MySpecificError from group``, allowing users to
handle your library-specific error. This is simple and reliable, but doesn't completely
hide the nursery. *Do not* unwrap single exceptions if there could ever be multiple
Comment on lines +805 to +806
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it what way does it not hide the nursery?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# end-user code
with library_function_wrapping_a_nursery():
    1/0

raises MySpecificError from ExceptionGroup("", [ZeroDivisionError()]) instead of a bare ZeroDivisionError.

exceptions though; that always ends in latent bugs and then tears.

The more complex option is to ensure that only one exception can in fact happen at a time.
This is *very hard*, for example you'll need to handle `KeyboardInterrupt` somehow, and
we strongly recommend having a ``raise PleaseReportBug from group`` fallback just in case
you get a group containing more than one exception.
This is useful when writing a context manager which starts some background tasks, and then
yields to user code which effectively runs 'inline' in the body of the nursery block.
In this case, the background tasks can be wrapped with e.g. the `outcome
<https://pypi.org/project/outcome/>`__ library to ensure that only one exception
can be raised (from end-user code); and then you can either ``raise SomeInternalError``
if a background task failed, or unwrap the user exception if that was the only error.

..
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
ValueError:`` (not ``except*``), which works if a single exception is raised but fails to
catch _anything_ in the case of multiple simultaneous exceptions (even if one of them is
a ValueError). However, this is not how Trio worked in the past: as a concession to
practicality when the ``except*`` syntax hadn't been dreamed up yet, the old
``trio.MultiError`` was raised only when at least two exceptions occurred
simultaneously. Adding a layer of `ExceptionGroup` around every nursery, while
theoretically appealing, would probably break a lot of existing code in practice.

Therefore, we've chosen to gate the newer, "stricter" behavior behind a parameter
called ``strict_exception_groups``. This is accepted as a parameter to
:func:`open_nursery`, to set the behavior for that nursery, and to :func:`trio.run`,
to set the default behavior for any nursery in your program that doesn't override it.

* With ``strict_exception_groups=True``, the exception(s) coming out of a nursery will
always be wrapped in an `ExceptionGroup`, so you'll know that if you're handling
single errors correctly, multiple simultaneous errors will work as well.

* With ``strict_exception_groups=False``, a nursery in which only one task has failed
will raise that task's exception without an additional layer of `ExceptionGroup`
wrapping, so you'll get maximum compatibility with code that was written to
support older versions of Trio.

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.
For more on this pattern, see https://github.com/python-trio/trio/issues/2929
and the linked issue on trio-websocket. We may want to provide a nursery mode
which handles this automatically; it's annoying but not too complicated and
seems like it might be a good feature to ship for such cases.

**Third and most often**, the existence of a nursery in your code is not just an
implementation detail, and callers *should* be prepared to handle multiple exceptions
in the form of an `ExceptionGroup`, whether with ``except*`` or manual inspection
or by just letting it propagate to *their* callers. Because this is so common,
it's nurseries' default behavior and you don't need to do anything.
Comment on lines +826 to +830
Copy link
Member

@belm0 belm0 Apr 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

most often -> most common

But if we're talking about an API, I don't think it's desirable for this to be the most common. I'm afraid it will become the most common though, because it takes a lot of work to do otherwise.

I'd guess that an async call or context manager having concurrency inside is usually an implementation detail, and should not be exposed. Exposing ExceptionGroup from an API is a significant burden for users, and should be considered carefully.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For many functions, internal concurrency is part of the documented semantics - for example it's not just an implementation detail that trio.serve_listeners() has a nursery internally. In such cases I think that responsibility for handling an ExceptionGroup should lie with the caller.

I agree it's a fuzzy boundary though, and I'll remove the most-common claim. I don't think the burden is particularly heavy once you're on Python 3.11+ and can use except*, though obviously this will vary by project.

Copy link
Member

@belm0 belm0 Apr 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No doubt there are API's that need to expose concurrency, though we might disagree on how common that is.

A few burden points I can think of:

  • "once you're on Python 3.11+" - for any package trying to support the active Python versions, that's 2 years off. In the meantime you need the cumbersome exceptiongroup. And on the other hand, people will complain if you depend on exceptiongroup for 3.11+ even though it's harmless, complicating pinned dependencies for CI, etc...
  • we've agreed that pytest doesn't have a good story for matching the semantics of except*
  • that the user has to learn except* at all, even for an API that doesn't otherwise expose concurrency


.. _strict_exception_groups:

Historical Note: "non-strict" ExceptionGroups
+++++++++++++++++++++++++++++++++++++++++++++

In early versions of Trio, the ``except*`` syntax hadn't be dreamt up yet, and we
hadn't worked with structured concurrency for long or in large codebases.
As a concession to convenience, some APIs would therefore raise single exceptions,
and only wrap concurrent exceptions in the old ``trio.MultiError`` type if there
were two or more.

Unfortunately, the results were not good: calling code often didn't realize that
some function *could* raise a ``MultiError``, and therefore handle only the common
case - with the result that things would work well in testing, and then crash under
heavier load (typically in production). `asyncio.TaskGroup` learned from this
experience and *always* wraps errors into an `ExceptionGroup`, as does ``anyio``,
and as of Trio 0.25 that's our default behavior too.

We currently support a compatibility argument ``strict_exception_groups=False`` to
`trio.run` and `trio.open_nursery`, which restores the old behavior (although
``MultiError`` itself has been fully removed). We strongly advise against it for
new code, and encourage existing uses to migrate - we consider the option deprecated
and plan to remove it after a period of documented and then runtime warnings.
A5rocks marked this conversation as resolved.
Show resolved Hide resolved

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

Expand Down
21 changes: 15 additions & 6 deletions src/trio/_core/_run.py
Original file line number Diff line number Diff line change
Expand Up @@ -997,9 +997,12 @@ def open_nursery(
if strict_exception_groups is not None and not strict_exception_groups:
warn_deprecated(
"open_nursery(strict_exception_groups=False)",
version="0.24.1",
version="0.25.0",
issue=2929,
instead="the default value of True and rewrite exception handlers to handle ExceptionGroups",
instead=(
"the default value of True and rewrite exception handlers to handle ExceptionGroups. "
"See https://trio.readthedocs.io/en/stable/reference-core.html#designing-for-multiple-errors"
),
)

if strict_exception_groups is None:
Expand Down Expand Up @@ -2253,9 +2256,12 @@ def run(
if strict_exception_groups is not None and not strict_exception_groups:
warn_deprecated(
"trio.run(..., strict_exception_groups=False)",
version="0.24.1",
version="0.25.0",
issue=2929,
instead="the default value of True and rewrite exception handlers to handle ExceptionGroups",
instead=(
"the default value of True and rewrite exception handlers to handle ExceptionGroups. "
"See https://trio.readthedocs.io/en/stable/reference-core.html#designing-for-multiple-errors"
),
)

__tracebackhide__ = True
Expand Down Expand Up @@ -2366,9 +2372,12 @@ def my_done_callback(run_outcome):
if strict_exception_groups is not None and not strict_exception_groups:
warn_deprecated(
"trio.start_guest_run(..., strict_exception_groups=False)",
version="0.24.1",
version="0.25.0",
issue=2929,
instead="the default value of True and rewrite exception handlers to handle ExceptionGroups",
instead=(
"the default value of True and rewrite exception handlers to handle ExceptionGroups. "
"See https://trio.readthedocs.io/en/stable/reference-core.html#designing-for-multiple-errors"
),
)

runner = setup_runner(
Expand Down
5 changes: 4 additions & 1 deletion src/trio/_subprocess.py
Original file line number Diff line number Diff line change
Expand Up @@ -651,7 +651,10 @@ async def my_deliver_cancel(process):
and the process exits with a nonzero exit status
OSError: if an error is encountered starting or communicating with
the process
ExceptionGroup: if exceptions occur in ``deliver_cancel``, or when exceptions occur when communicating with the subprocess. If strict_exception_groups is set to false in the global context, then single exceptions will be collapsed.
ExceptionGroup: if exceptions occur in ``deliver_cancel``,
or when exceptions occur when communicating with the subprocess.
If strict_exception_groups is set to false in the global context,
which is deprecated, then single exceptions will be collapsed.

.. note:: The child process runs in the same process group as the parent
Trio process, so a Ctrl+C will be delivered simultaneously to both
Expand Down
Loading