-
Notifications
You must be signed in to change notification settings - Fork 135
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
Issue with nested task groups and cancellation #37
Comments
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 |
Have you run the test suite to make sure that nothing else breaks with this patch? |
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. |
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. |
This line in |
I ran a quick investigation. It turns out that in In 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: |
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: |
What I could not understand is why there are two |
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? |
@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 I can create a separate issue if you want. |
You are not supposed to catch a In fact, I very much want to recommend to follow Trio and use |
Erh, my bad... The problem I had wasn't with cancellation but timeout. It turns out that I guess this could easily be fixed in anyio. |
The fact that In any case, anyio uses |
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. |
Do you have some reference regarding this decision being a bad one? Guido has a justification for it:
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 :) |
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 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") |
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 |
@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? And if you do that, there is no problem since you can just catch |
I'm definitely willing to do so. Actually I did just that in this commit.
Oh yes, I guess I could catch anyio exceptions (such as 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. |
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)
|
I looked into this and this happens because I'm not reraising the |
My latest idea is introducing new backend specific cancellation exceptions that inherit from both the native exception and |
You might want to do the same thing with |
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 |
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) |
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 |
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 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.
|
Umm, how about (3) documenting that |
I can think of two arguments against that:
|
IIRC this is the exception raised by blocking socket calls when the timeout set via
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. |
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.
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!") |
Well … you don't raise a
The difficult part is unpacking all of this from and to |
As far as I can tell, this issue is different from #34.
When an outer task group cancels a task containing an inner task group, the inner task group does not re-raise the
CancelledError
:Reports:
As a side note,
test_anyio[trio]
does not fail for the same reason as the othertest_anyio
: it raises an ExceptionGroup containing twotrio.Cancelled
exceptions.The text was updated successfully, but these errors were encountered: