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

Don't wrap exceptions in ExceptionGroup if only one exception is raised on Python<3.11 #668

Closed
jonathanslenders opened this issue Jan 11, 2024 · 10 comments
Labels
enhancement New feature or request

Comments

@jonathanslenders
Copy link
Contributor

jonathanslenders commented Jan 11, 2024

Not sure whether it's a feature or bug/regression.

I'm in the process of upgrading from anyio 3 to anyio 4.

So far we've explicitly designed our code in a way that no more than 1 exception will be raised in a task group. (By making sure only one code path will result in an exception.) This worked great, and prevented us from having to deal with exception groups. We still support Python 3.8 so, that's important to us.

However, after upgrading to anyio 4, even if there is only one exception raised as part of a task group, it will be wrapped in an ExceptionGroup. This means, we have to use the with catch() syntax which is quite cumbersome, and everywhere. That's a huge amount of work, and boilerplate to add. :'(.

It would be nice if the code could be modified so that on Python<3.11, if there is only one exception, it won't be wrapped. The change is very simple, and should be backward compatible.

--- a/src/anyio/_backends/_asyncio.py
+++ b/src/anyio/_backends/_asyncio.py
@@ -675,6 +675,8 @@ class TaskGroup(abc.TaskGroup):

         self._active = False
         if self._exceptions:
+            if len(self._exceptions) == 1 and sys.version_info < (3, 11):
+                raise self._exceptions[0]
             raise BaseExceptionGroup(
                 "unhandled errors in a TaskGroup", self._exceptions
             )
@jonathanslenders jonathanslenders added the enhancement New feature or request label Jan 11, 2024
@agronholm
Copy link
Owner

Not going to happen. This was a deliberate design decision, and asyncio already does the same with their TaskGroup, and Trio is close behind switching strict_exception_groups=True as the default for their nurseries. Trying to catch single exceptions with except is perilous with task groups anyway – a practice I strongly discourage.

@agronholm agronholm closed this as not planned Won't fix, can't repro, duplicate, stale Jan 11, 2024
@jonathanslenders
Copy link
Contributor Author

I was afraid you were going to say that. :-)

Does this mean that Python 3.11 should be considered the minimum version for anyio 4?

This behavior is viral in unexpected ways.
For instance, let's say as a library, we define a context manager for a user to be used. The user uses this context manager, and raises an exception of their own in there, like this:

async with context_manager_of_3rd_party_library():
    raise SomeException

Then, later, as the author of the context manager, we decide that it's better to implement it by opening a task group in there. This means that suddenly, the exception will be propagated through a task group, which turns this exception into an ExceptionGroup. The end-user, not aware of the implementation details, suddenly sees an API change in their own code. At least on Python 3.10.

For anyone coming across this issue, here is the related Trio issue: python-trio/trio#2785

@agronholm
Copy link
Owner

Does this mean that Python 3.11 should be considered the minimum version for anyio 4?

Why? If you're targeting Python versions below 3.11, you just have to use the with catch() context manager you mentioned if you want to catch exceptions falling from a task group. Personally I don't recall ever needing such a thing, as I catch exceptions in the child tasks already, so anything falling through from the task group should bubble up.

@jonathanslenders
Copy link
Contributor Author

It's not "just". As a library author, it means asking end-users to use the with catch() syntax. Them being asyncio users, they are often not aware that a library uses anyio under the hood, and definitely not about exceptiongroup peculiarities.

If they had code like in the example above. If users suddenly see their exception being turned into another exception and they now require with catch(), that is a significant change.

To be clear, I totally understand if this is not going to be changed. I only want to share how this complicates upgrading libraries to anyio 4 in these situations.

as I catch exceptions in the child tasks already

Yes, I do that as well. In the example however, we're talking about an exception from a user, and not one that was raised in a task, but rather in the body of a context manager that happens to open a task group internally.

@smurfix
Copy link
Collaborator

smurfix commented Jan 12, 2024

Then, later, as the author of the context manager, we decide that it's better to implement it by opening a task group in there.

So you wrap the taskgroup and the yield with an exception handler that saves and re-raises the error, assuming it's the only one that bubbles out of the taskgroup.

Ten lines of boilerplate. Annoying, yes, but hardly viral.

@jonathanslenders
Copy link
Contributor Author

jonathanslenders commented Jan 12, 2024

I'm trying with a create_task_group wrapper. So far this seems to work fine:

from exceptiongroup import ExceptionGroup, catch
 
@asynccontextmanager
async def create_task_group() -> AsyncGenerator[anyio.abc.TaskGroup, None]:
    def handler(exc: BaseException) -> None:
        if isinstance(exc, ExceptionGroup) and len(exc.exceptions) == 1:
            raise exc.exceptions[0]
        raise exc

    with catch({Exception: handler}):
        async with anyio.create_task_group() as tg:
            yield tg

@agronholm
Copy link
Owner

And what if the exception group contains another one?

@agronholm
Copy link
Owner

It's not "just". As a library author, it means asking end-users to use the with catch() syntax. Them being asyncio users, they are often not aware that a library uses anyio under the hood, and definitely not about exceptiongroup peculiarities.

Don't forget, asyncio.TaskGroup has the same semantics too. They're going to have to deal with it sooner or later.

@jonathanslenders
Copy link
Contributor Author

And what if the exception group contains another one?

I know, it is somewhat fragile, but to me it looks feasible to write code where almost all exceptions are caught within the child tasks, and at most one exception will propagate. At least, that is what we currently have in a reasonable sized codebase (~200k loc). Do you think there will be situations where additional exceptions will be triggered, that can't be suppressed?

Don't forget, asyncio.TaskGroup has the same semantics too. They're going to have to deal with it sooner or later.

Right. It's a good point. Actually I only now realize that the star in except *ExcType is required; also in case of one exception. I think I knew, and then forgot. So, I now see why you discourage any usage of except ExcType when there are task groups involved. But anyway, this will do, I will go with the wrapper for now. In our case, no need yet to have end users deal with exceptiongroups.

@agronholm
Copy link
Owner

Yeah because, if exception group collapsing was enabled (as in anyio 3), except ExcType: would catch the exception only if it's the only exception falling out of the group, but not if there is more than one exception being raised. That's why it's dangerous.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants