From 21d69fd5e37426fb4704c03c9d8aed04901cd8b5 Mon Sep 17 00:00:00 2001 From: Zac Hatfield-Dodds Date: Sat, 6 Apr 2024 02:44:35 -0400 Subject: [PATCH 1/2] Doc: strict ExceptionGrops as natural --- docs/source/conf.py | 1 + docs/source/reference-core.rst | 125 +++++++++++++++++++++++---------- src/trio/_subprocess.py | 5 +- 3 files changed, 91 insertions(+), 40 deletions(-) diff --git a/docs/source/conf.py b/docs/source/conf.py index ff071bb088..d9f223b217 100755 --- a/docs/source/conf.py +++ b/docs/source/conf.py @@ -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), } diff --git a/docs/source/reference-core.rst b/docs/source/reference-core.rst index 24d7cee380..bb110da6a0 100644 --- a/docs/source/reference-core.rst +++ b/docs/source/reference-core.rst @@ -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 @@ -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 @@ -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`. +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 +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 +`__ 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. + +.. _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. .. _exceptiongroup: https://pypi.org/project/exceptiongroup/ diff --git a/src/trio/_subprocess.py b/src/trio/_subprocess.py index 6b2f0a856e..562476cd94 100644 --- a/src/trio/_subprocess.py +++ b/src/trio/_subprocess.py @@ -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 From 44407061fb2b2588705052639a8afb1b95c21418 Mon Sep 17 00:00:00 2001 From: EXPLOSION Date: Fri, 26 Apr 2024 13:16:10 +0900 Subject: [PATCH 2/2] Link the deprecations to new documentation --- src/trio/_core/_run.py | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index 9d27793d82..72ad961561 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -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: @@ -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 @@ -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(