Issue with nested task groups and cancellation about anyio HOT 35 CLOSED

The following (quick and dirty) patch fixes the issue for the asyncio backend:

diff --git a/anyio/_backends/asyncio.py b/anyio/_backends/asyncio.py
index bcb38c0..af93864 100644
--- a/anyio/_backends/asyncio.py
+++ b/anyio/_backends/asyncio.py
@@ -329,13 +329,16 @@ abc.TaskGroup.register(TaskGroup)
 @asynccontextmanager
 @async_generator
 async def create_task_group():
+    reraise = None
     async with open_cancel_scope() as cancel_scope:
         group = TaskGroup(cancel_scope, current_task())
         exceptions = []
         try:
             try:
                 await yield_(group)
-            except (CancelledError, asyncio.CancelledError):
+            except (CancelledError, asyncio.CancelledError) as exc:
+                if not cancel_scope.cancel_called:
+                    reraise = exc
                 await cancel_scope.cancel()
             except BaseException as exc:
                 exceptions.append(exc)
@@ -364,6 +367,8 @@ async def create_task_group():
             raise ExceptionGroup(exceptions)
         elif exceptions:
             raise exceptions[0]
+    if reraise:
+        raise reraise
 
 #
 # Threads

from anyio.

Have you run the test suite to make sure that nothing else breaks with this patch?

from anyio.

I did, and nothing else broke. However, I did not try to make it conceptually correct so you probably want to look for a better fix.

from anyio.

I expect I will have some time this Sunday. I really need to first wrap my head around the problem. The trio case in particular mystifies me greatly.

from anyio.

This line in anyio._backends.trio
nursery.cancel_scope.cancel = wrap_as_awaitable(nursery.cancel_scope.cancel)
might be the culprit. You're monkeypatching the internals of the Trio nursery's cancel scope without regard of whoever else might want to call that – including calls that are internal to Trio. That's not a good idea; you'll have to create a wrapper class for the cancel scope.

from anyio.

The trio case in particular mystifies me greatly.

I ran a quick investigation. It turns out that in test_trio, g() raises <MultiError: Cancelled(), Cancelled()> but this exception gets filtered by the cancel scope of the outer nursery (here).

In test_anyio[trio], the <MultiError: Cancelled(), Cancelled()> gets converted to an ExceptionGroup by the create_task_group async generator (here). That eventually prevents the cancel scope of the outer nursery to filter the Cancelled() exceptions.

Another quick and dirty patch for the trio backend could be:

diff --git a/anyio/_backends/trio.py b/anyio/_backends/trio.py
index 623ee67..b19d935 100644
--- a/anyio/_backends/trio.py
+++ b/anyio/_backends/trio.py
@@ -94,6 +94,8 @@ async def create_task_group():
             tg = TaskGroup(nursery)
             await yield_(tg)
     except trio.MultiError as exc:
+        if all(isinstance(e, trio.Cancelled) for e in exc.exceptions):
+            raise
         raise ExceptionGroup(exc.exceptions) from None
     finally:
         if tg is not None:

from anyio.

Below the full patch fixing all three tests:

diff --git a/anyio/_backends/asyncio.py b/anyio/_backends/asyncio.py
index bcb38c0..af93864 100644
--- a/anyio/_backends/asyncio.py
+++ b/anyio/_backends/asyncio.py
@@ -329,13 +329,16 @@ abc.TaskGroup.register(TaskGroup)
 @asynccontextmanager
 @async_generator
 async def create_task_group():
+    reraise = None
     async with open_cancel_scope() as cancel_scope:
         group = TaskGroup(cancel_scope, current_task())
         exceptions = []
         try:
             try:
                 await yield_(group)
-            except (CancelledError, asyncio.CancelledError):
+            except (CancelledError, asyncio.CancelledError) as exc:
+                if not cancel_scope.cancel_called:
+                    reraise = exc
                 await cancel_scope.cancel()
             except BaseException as exc:
                 exceptions.append(exc)
@@ -364,6 +367,8 @@ async def create_task_group():
             raise ExceptionGroup(exceptions)
         elif exceptions:
             raise exceptions[0]
+    if reraise:
+        raise reraise
 
 #
 # Threads
diff --git a/anyio/_backends/curio.py b/anyio/_backends/curio.py
index 6842d0d..17d41a0 100644
--- a/anyio/_backends/curio.py
+++ b/anyio/_backends/curio.py
@@ -220,13 +220,16 @@ abc.TaskGroup.register(TaskGroup)
 @asynccontextmanager
 @async_generator
 async def create_task_group():
+    reraise = None
     async with open_cancel_scope() as cancel_scope:
         group = TaskGroup(cancel_scope, await curio.current_task())
         exceptions = []
         try:
             try:
                 await yield_(group)
-            except (CancelledError, curio.CancelledError, curio.TaskCancelled):
+            except (CancelledError, curio.CancelledError, curio.TaskCancelled) as exc:
+                if not cancel_scope.cancel_called:
+                    reraise = exc
                 await cancel_scope.cancel()
             except BaseException as exc:
                 exceptions.append(exc)
@@ -255,6 +258,8 @@ async def create_task_group():
             raise ExceptionGroup(exceptions)
         elif exceptions:
             raise exceptions[0]
+    if reraise:
+        raise reraise
 
 
 #
diff --git a/anyio/_backends/trio.py b/anyio/_backends/trio.py
index 623ee67..b19d935 100644
--- a/anyio/_backends/trio.py
+++ b/anyio/_backends/trio.py
@@ -94,6 +94,8 @@ async def create_task_group():
             tg = TaskGroup(nursery)
             await yield_(tg)
     except trio.MultiError as exc:
+        if all(isinstance(e, trio.Cancelled) for e in exc.exceptions):
+            raise
         raise ExceptionGroup(exc.exceptions) from None
     finally:
         if tg is not None:

from anyio.

In test_anyio[trio], the <MultiError: Cancelled(), Cancelled()> gets converted to an ExceptionGroup by the create_task_group async generator (here). That eventually prevents the cancel scope of the outer nursery to filter the Cancelled() exceptions.

What I could not understand is why there are two Cancelled exceptions. One I can understand, but where is the other one coming from?

from anyio.

You're monkeypatching the internals of the Trio nursery's cancel scope without regard of whoever else might want to call that – including calls that are internal to Trio. That's not a good idea; you'll have to create a wrapper class for the cancel scope.

I'm not monkey patching anything. Monkey patching would be if I were to modify the behavior of trio's nursery system regardless of where the nursery is being used. Instead, I'm only wrapping one method in a specific nursery instance which is created under anyio. How is that any different from creating a wrapper class?

from anyio.

@agronholm Thanks for the fix! I don't have time to test it at the moment but I'll let you know how it goes :)

On a different subject, I remember I got pretty confused with the exception management. It seems like anyio replaces the specific exceptions of the running backend with a common layer of anyio exception. This is quite problematic for my use case, as my goal is to provide a library that transparently support any backend. This means that the user of the library shouldn't need to know about anyio and be able to catch the exception defined in the async framework he's using (e.g trio.Cancelled, asyncio.CancelledError, curio.CancelledError, etc.). Is this use case outside of the anyio scope?

I can create a separate issue if you want.

from anyio.

You are not supposed to catch a CancelledError. You should always re-raise it; catching it is the framework's job. Thus, if you want to log all exceptions that cause your code to exit, use except BaseException: log(…); raise.

In fact, I very much want to recommend to follow Trio and use BaseException as the superclass of anyio.exceptions.CancelledError … except that both asyncio and curio backends no longer work when you try that because they believe that BaseException is always terminally fatal, thus they don't clean up properly. :-(

from anyio.

@smurfix

You are not supposed to catch a CancelledError

Erh, my bad... The problem I had wasn't with cancellation but timeout. It turns out that builtins.TimeoutError is different from asyncio.TimeoutError (they don't even inherit from one another).

I guess this could easily be fixed in anyio.

from anyio.

The fact that asyncio.TimeoutError (really concurrent.futures.TimeoutError) is not builtins.TimeoutError is asyncio's problem, not ours. It's a stupid decision which we shouldn't repeat.

In any case, anyio uses builtins.TimeoutError for fail_after on all backends. If you use non-anyio code, much less code that doesn't work on all anyio backends, well, you get to cater to its exceptions.

from anyio.

I discovered through trial and error that converting backend specific cancellation exceptions doesn't work well, particularly with trio. This is why I no longer try that. That said, I would like to provide some means of catching such things and I have one idea of how to go about doing that, but we'll see if it pans out.

from anyio.

@smurfix

The fact that asyncio.TimeoutError (really concurrent.futures.TimeoutError) is not builtins.TimeoutError is asyncio's problem, not ours. It's a stupid decision which we shouldn't repeat.

Do you have some reference regarding this decision being a bad one? Guido has a justification for it:

I considered this, and decided against unifying the two TimeoutErrors.
First the builtin TimeoutError is specifically a subclass of OSError representing the case where errno is ETIMEDOUT. But asyncio.TimeoutError means nothing of the sort.
Second, the precedent is concurrent.futures.TimeoutError. The asyncio one is used under the same conditions as that one.

The discussion got a bit technical, but my comment is more about the general purpose of anyio. More specifically, can I use it in my library to support asyncio, curio and trio transparently (i.e without the user having to care about the anyio compatibility layer)?

This open question is the main reason why the anyio branch is not merged into aiostream. I would definitely understand if this use case is out of the anyio scope though :)

from anyio.

What is your library doing and how is the user going to use it? The answer depends on that.

from anyio.

@agronholm

What is your library doing and how is the user going to use it? The answer depends on that

The most symptomatic issue is probably the stream.timeout operator. It raises a time-out if an element of the given asynchronous sequence takes too long to arrive. Ideally, I'd expect this stream to raise an asyncio.TimeoutError, a curio.TaskTimeout or a trio.TooSlowError depending on the loop that is currently running.

Would it make sense for anyio to define exceptions in a lazy way? Something like:

def get_timeout_error_class():
    asynclib = anyio._detect_running_asynclib()
    if asynclib == 'asyncio':
        import asyncio
        return asyncio.TimeoutError
    if asynclib == 'trio':
        import trio
        return trio.ToolSlowError
    if asynclib == 'curio':
        import curio
        return curio.TaskTimeout
    raise RuntimeError("Asynclib detection failed")

from anyio.

I'm thoroughly confused now. In what kind of scenario would you be relying on backend specific timeout exceptions without locking yourself to a specific backend? If, on the other hand, the code was written for anyio, you would simply catch the TimeoutError.

from anyio.

@agronholm
I'm sorry for not being clear. In the case of a library like aiostream, the user is most likely relying on a specific backend and expects backend specific exceptions. On the other hand, most of the library is backend agnostic, so it would be a waste of effort to maintain three versions of a similar code base. I was hoping anyio could help me by providing both a compatibility layer and test parametrization. Hope it makes sense now :)

from anyio.

What do you hope to get from anyio if you're not willing to refactor the project to replace asyncio use with anyio's API calls? And if you do that, there is no problem since you can just catch TimeoutError then.

from anyio.

@agronholm

What do you hope to get from anyio if you're not willing to refactor the project to replace asyncio use with anyio's API calls?

I'm definitely willing to do so. Actually I did just that in this commit.

And if you do that, there is no problem since you can just catch TimeoutError then.

Oh yes, I guess I could catch anyio exceptions (such as TimeoutError) and raise the corresponding backend-specific exception. Maybe it's simpler this way.

I have another example that might help with my general point: what if a user runs a library call in a trio nursery, and the library call runs an anyio task group? Can I expect those two to work together as expected if, say, the trio nursery is cancelled?

from anyio.

I have another example that might help with my general point: what if a user runs a library call in a trio nursery, and the library call runs an anyio task group? Can I expect those two to work together as expected if, say, the trio nursery is cancelled?

No. Anyio does not magically make the three async libraries compatible with each other. If you mix non-anyio code with anyio, you're stuck with the backend the code is written for. But if all the code you're running is written for anyio, you can freely switch between backends.

from anyio.

@agronholm

No. Anyio does not magically make the three async libraries compatible with each other.

Sorry for not being clear again, I wasn't talking about mixing backends. Here's a toy example:

# ./mylib.py
import anyio

async def do_something():
    async with anyio.create_task_group() as task_group:
        await task_group.spawn(anyio.sleep, 3)
    print("This should not be printed if cancelled")

# ./usercode.py
import trio
from mylib import do_something

async def main():
    async with trio.open_nursery() as nursery:
        nursery.start_soon(do_something)
        await trio.sleep(1)
        nursery.cancel_scope.cancel()

trio.run(main)

It seems to work fine as far as I can tell, but I might be missing some edge cases.
EDIT: Ooops I replied too fast, the This should not be printed message gets printed.

from anyio.

I looked into this and this happens because I'm not reraising the MultiError that is raised when all the tasks have been cancelled. Simply reraising it fixes this particular gap, but I'm not convinced that it's the right solution. I'll tinker some more.

from anyio.

My latest idea is introducing new backend specific cancellation exceptions that inherit from both the native exception and anyio.exceptions.CancelledError, and converting any native exceptions to instances of these new classes.

from anyio.

You might want to do the same thing with TimeoutError.

from anyio.

You might want to do the same thing with TimeoutError.

Why? The only reason why I'd do this dance with cancellation exceptions is because they are natively produced by the backends and cannot be normalized into anyio's exceptions without this trick. But there is no such problem with TimeoutError unless I am missing something?

from anyio.

@agronholm

But there is no such problem with TimeoutError unless I am missing something?

Here's a use case, similar to the previous example:

# ./mylib.py
import anyio

async def do_something_with_timeout():
    async with anyio.fail_after(1):
        await anyio.sleep(2)


# ./usercode.py
import trio
from mylib import do_something_with_timeout


async def main():
    try:
        await do_something_with_timeout()
    # As a trio user, it feels weird to use `TimeoutError` here.
    # Sure, the user could read the `mylib` documention or the 
    # `do_something_with_timeout` docstring but it kind of breaks 
    # the principle of least astonishment. The user uses trio, so they
    # expect standard trio exceptions (i.e `TooSlowError`, `Cancelled`, 
    # `MultiError`, etc.)
    except trio.TooSlowError:
        print("Timeout!")

trio.run(main)

from anyio.

If you used blocking code or any other foreign library, you would have to adjust the containing code to handle it differently. How is this situation any different? If anyio's API description said it raises TimeoutError, then it is squarely the developer's fault if this is ignored.

from anyio.

If anyio's API description said it raises TimeoutError, then it is squarely the developer's fault if this is ignored.

Sure, but the maintainer of the hypothetical library would have to write something like that:

# ./mylib.py
import anyio

async def do_something_with_timeout():
    try:
        async with anyio.fail_after(1):
            await anyio.sleep(2)
    except TimeoutError:
        raise_specific_timeout_error()

And then maintain his own version of raise_specific_timeout_error:

def raise_specific_timeout_error():
    asynclib = anyio._detect_running_asynclib()
    if asynclib == 'asyncio':
        import asyncio
        raise asyncio.TimeoutError()
    if asynclib == 'trio':
        import trio
        raise trio.ToolSlowError()
    if asynclib == 'curio':
        import curio
        raise curio.TaskTimeout()
    raise RuntimeError("Asynclib detection failed")

To sum it up, I can see two solutions for supporting this kind of use case.

• 1 - only use backend-specific exceptions in anyio (using runtime detection for raising and catching)
• 2 - provide helpers (decorators, context managers, optional arguments, etc.) for leaving anyio land transparently. For instance:

  @contextmanager
  def anyio_context():
      try:
          yield
      except TimeoutError as exc:
          raise specific_timeout_error(exc)
      except anyio.ExceptionGroup:
          raise specific_exception_group(exc)
      except [...]

from anyio.

Umm, how about (3) documenting that mylib raises TimeoutError when it times out?

from anyio.

@smurfix

how about (3) documenting that mylib raises TimeoutError when it times out?

I can think of two arguments against that:

• Using TimeoutError is quite opinionated since it is originally designed as an OSError corresponding to errno.ETIMEDOUT:

  >>> OSError(errno.ETIMEDOUT, os.strerror(errno.ETIMEDOUT))
  TimeoutError(110, 'Connection timed out')

• Maybe mylib was originally designed for one specific backend, and the maintainer wish to extend it to other backends. Changing to anyio specific exceptions would break backward compatibility. This is the case for aiostream for instance.

from anyio.

Using TimeoutError is quite opinionated since it is originally designed as an OSError corresponding to errno.ETIMEDOUT

IIRC this is the exception raised by blocking socket calls when the timeout set via setsockopt() is triggered. Since that can (or at least should) never happen with async code, I saw no reason not to reuse it.

Maybe mylib was originally designed for one specific backend, and the maintainer wish to extend it to other backends. Changing to anyio specific exceptions would break backward compatibility. This is the case for aiostream for instance.

The API description will have to be altered in any case. The good point made here is that if we had backend specific timeout exceptions of our own which inherit from both the backend native timeout exception and the anyio provided one, the anyio transition would go smoother since any code that catches the native timeout exception would still work as expected. My only concern with this solution is this: will there be problems if the user code needs to raise such exceptions directly? Should we just tell devs to raise the anyio timeout exception (without the inheritance from the native exceptions)?

If there are no valid concerns about this, then I'm starting to lean towards adding our own timeout exception.

from anyio.

I saw no reason not to reuse it.

Yes, it is (most probably) fine to use it. But my point is that this is anyio's opinion, in the same way that asyncio, curio and trio have a different opinion about what a timeout error is.

The good point made here is that if we had backend specific timeout exceptions of our own which inherit from both the backend native timeout exception and the anyio provided one, the anyio transition would go smoother since any code that catches the native timeout exception would still work as expected. My only concern with this solution is this: will there be problems if the user code needs to raise such exceptions directly? Should we just tell devs to raise the anyio timeout exception (without the inheritance from the native exceptions)?

Those are indeed complicated questions. One way to get rid of them is to let the developers clearly define which part of the code is anyio land and which part of the code is backend land. Using the context manager from earlier:

async def some_trio_stuff():
    # This trio land
    try:
        with anyio_context():
            # This is anyio land
            try:
                await some_anyio_stuff()
            except anyio.TimeoutError:
                print("Timeout!")
                raise
    except trio.TooSlowError:
        print("Timeout!")

from anyio.

My only concern with this solution is this: will there be problems if the user code needs to raise such exceptions directly?

Well … you don't raise a CancelledError directly – instead, you get it raised by cancelling a scope. You also don't raise a TimeoutError directly – instead, you get it raised by spending too much time in a fail_after block.
Thus I'd recommend to simply forbid raising these exceptions directly.

with anyio_context

The difficult part is unpacking all of this from and to MultiError, or whatever your runtime's idea of multiple errors is, assuming that it has any in the first place.

from anyio.