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

Conversation

Zac-HD
Copy link
Member

@Zac-HD Zac-HD commented Apr 6, 2024

Closes #2929, by ensuring that our docs present strict_exception_groups=True as the natural default (and it is indeed the default in current Trio, as well as the only behavior of asyncio and anyio).

Following discussion on that issue, I've also written up some notes on three ways of handling ExceptionGroups before raising to user code: (1) defer-to, (2) single-error, optionally handling background and foreground tasks differently, and (3) simply letting it propagate to the caller.

2b might actually be a neat option to add to trio.open_nusery() itself, but I'd want to see a proof-of-concept judged useful downstream before we committed to supporting it there.

@Zac-HD Zac-HD added the docs label Apr 6, 2024

This comment was marked as resolved.

@clint-lawrence
Copy link

I'm not sure if this is directly relevant to these docs or not.

While working on #2972 I came across a subtle different between trio and asyncio. In asyncio, if a SystemExit or KeyboardInterrupt is what leads to a task group being cancelled, that individual exception will be re-raised, rather than being wrapped into an exception group. My understanding is that in trio, a nursery (with strict_exception_groups=True) will wrap those into a exception group.

Highlighting that difference would be nice, but I'm not sure where that fits best in the docs.

@Zac-HD
Copy link
Member Author

Zac-HD commented Apr 6, 2024

In asyncio, if a SystemExit or KeyboardInterrupt is what leads to a task group being cancelled, that individual exception will be re-raised, rather than being wrapped into an exception group. My understanding is that in trio, a nursery (with strict_exception_groups=True) will wrap those into a exception group.

That matches my understanding on the Trio side. I think this should probably be a separate issue, where we can decide whether to match asyncio (probably yes?) and then implement + document whatever we do.

I've reached my timebox for this PR though, so if people agree it's an improvement on the status quo I'll merge it 🙂

@Zac-HD Zac-HD force-pushed the exceptiongroup-strict-docs branch from 6619b75 to 21d69fd Compare April 6, 2024 19:47
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)

Comment on lines +805 to +806
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
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.

Comment on lines +826 to +830
**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.
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

@A5rocks A5rocks changed the title Document strict ExceptionGrops as the natural default Document strict ExceptionGroups as the natural default Apr 16, 2024
@A5rocks
Copy link
Contributor

A5rocks commented Apr 16, 2024

Should the deprecation message for strict_exception_groups=False link to this handling exception groups section?

instead="the default value of True and rewrite exception handlers to handle ExceptionGroups",

(if that gets updated, please also update the version number to be 0.25.0!!)

@A5rocks
Copy link
Contributor

A5rocks commented Apr 26, 2024

Oops, I totally missed this:

I've reached my timebox for this PR though, so if people agree it's an improvement on the status quo I'll merge it 🙂

I'll add commits then! (was about to ask if I could without causing any sort of merge conflict with local changes)

@Zac-HD Zac-HD merged commit 3645831 into python-trio:master Apr 28, 2024
27 of 28 checks passed
@Zac-HD Zac-HD deleted the exceptiongroup-strict-docs branch April 28, 2024 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure that our docs present strict_exception_groups=True as the natural default
5 participants