Updated cancellation semantics to better match the new trio and asyncio ones #586

agronholm · 2023-07-12T12:32:50Z

This is a lighter version of #496 which I could never get right.

Fixes #374.

…kGroup

zanieb

This looks great! I have some minor comments.

Does this also address cases from #432?

docs/tasks.rst

src/anyio/_backends/_asyncio.py

zanieb · 2023-07-14T16:43:33Z

src/anyio/_backends/_asyncio.py

+            elif exc_val is not None and not ignore_exception:
+                raise exc_val


This is the follow-up for "reraise the CancelledError later unless this task was already interrupted by another exception" right? As well as if exc_val is not null on context exit? Perhaps a comment about these cases (particularly when the second case would occur) would be helpful.

This was just about the hardest part to figure out, as AnyIO is treading into uncharted territory as far as cancellation semantics go. I need to double check that I got this right, or at least that I've made it behave consistently.

IIRC the idea was to raise a single cancellation exception if the host task was cancelled and there were no other errors.

At least asyncio.TaskGroup behaves this way.

In the case where exc_val is the exception originally pased to __aexit__, you will get a nicer traceback (no extraneous frame) if you return False rather than explicitly reraising it.

Sure it does -- save the original exc_val and compare its identity against the current one.

I didn't mean this wasn't fixable. What I left unsaid was that I would need to add an extra local variable just for distinguishing between these two cases.

It might be clearer not to reassign exc_val anyway. That'd make the case I had questions about at #586 (comment) more obvious.

Maybe you're right. I'll poke the code and see what I come up with.

I opted not to store any CancellationErrors which then simplifies the code somewhat. I just pushed an update.

src/anyio/_backends/_asyncio.py

Co-authored-by: Zanie Blue <contact@zanie.dev>

docs/tasks.rst

oremanj · 2023-07-14T18:19:55Z

docs/versionhistory.rst

+    ``ExceptionGroup`` (or ``BaseExceptionGroup`` if one or more ``BaseException`` were
+    included)
+  - Fixed task group not raising a cancellation exception on asyncio at exit if no child
+    tasks were spawned and an outer cancellation scope had been cancelled before


Trio would say that a Cancelled exception means some work was interrupted. In this case, it sounds like no work was interrupted, so I'm not sure it's actually important to raise the exception. Trio does raise the exception, because of its rule that every async operation that can block (including NurseryManager.__aexit__) is always both a schedule point and a cancel point, but this has caused problems occasionally and there has been recent discussion of changing it; IIRC consensus was that it would be fine to change but no one has done it yet. Discussion in python-trio/trio#1457.

The issue that this refers to was that an AnyIO TaskGroup would swallow a CancelledError that was meant to cancel the entire task.

Edge-triggered cancellation rears its ugly head again!

My intuition is that for fully consistent behavior here you need to treat each asyncio task as implicitly enclosed in a CancelScope that would be cancelled by task.cancel(), and include that implicit cancel scope in your logic of whether each cancel scope should swallow a cancellation or forward it outward. You can indirectly detect whether a task-level cancellation has occurred, at least on 3.11+, by counting anyio-originated calls to task.cancel() that are not yet uncancelled, and comparing that against task.cancelling().

Making task group exit a cancel point is defensible though. Even if Trio stops doing that, you can easily add it back by putting in an explicit checkpoint_if_cancelled.

treat each asyncio task as implicitly enclosed in a CancelScope that would be cancelled by task.cancel().

This is actually something I was going to propose for asyncio! I'll be sending that post to Async-SIG shortly. Meanwhile, I can again look for a workaround for Python < 3.11, but that should not block this PR.

agronholm · 2023-07-14T22:00:52Z

This looks great! I have some minor comments.

Does this also address cases from #432?

The differences in what comes out when cancellation happens is not really something I can reasonably control. Trio does its own thing and asyncio needs a single CancelledError to pop out in order for asyncio to work with it.

agronholm · 2023-07-16T01:06:16Z

So, what's the verdict? Are more changes needed here?

zanieb · 2023-07-16T17:21:41Z

src/anyio/_backends/_asyncio.py

+        if not waited_for_tasks_to_finish:
+            await AsyncIOBackend.checkpoint()


Worth documenting why we checkpoint here?

zanieb · 2023-07-16T17:24:30Z

src/anyio/_backends/_asyncio.py

+        # Raise the CancelledError received while waiting for child tasks to exit,
+        # unless the context manager itself was previously exited with another exception


This will also be skipped if an exception was raised in the the group (i.e. a value is in self._exceptions) from the clause above. Perhaps worth noting for clarity.

zanieb · 2023-07-16T17:24:44Z

src/anyio/_backends/_asyncio.py

+        if cancelled_exc_while_waiting_tasks:
+            if exc_val is None or ignore_exception:


Nit: This could be one statement

It would not increase readability. I've seen something similar used in CPython code, and my initial thoughts were the same as yours until I saw the wisdom in doing it like this.

zanieb · 2023-07-16T17:32:10Z

Overall I think this is clearer than before! I poked at it a little bit but couldn't come up with a better way to express the logic.

agronholm · 2023-07-17T20:55:54Z

I'm fairly happy with this, so I'm merging. If anything comes up, we can sort it out during the RC cycle.

agronholm · 2023-07-17T21:07:47Z

This looks great! I have some minor comments.
Does this also address cases from #432?

The differences in what comes out when cancellation happens is not really something I can reasonably control. Trio does its own thing and asyncio needs a single CancelledError to pop out in order for asyncio to work with it.

So I just talked to Joshua Oreman and it seems that Trio wouldn't have an issue with a single Cancelled coming out of a task group; it was just my misconception. Expect another PR to address that.

agronholm added 4 commits July 8, 2023 02:36

Added test to detect broken handling for native cancellation

e83241c

Refactored task groups for correctness and consistency

c3ccfcd

Pulled docs and dependency updates from the other PR

5c9b6f8

Changed the exception group message to match the one from asyncio.Tas…

75974f5

…kGroup

agronholm requested a review from graingert July 12, 2023 12:32

Merge branch 'master' into detect-asyncio-native-cancel

63eda32

agronholm mentioned this pull request Jul 12, 2023

Improved cancellation semantics #496

Closed

zanieb approved these changes Jul 14, 2023

View reviewed changes

agronholm and others added 2 commits July 14, 2023 20:02

Update src/anyio/_backends/_asyncio.py

3d714fa

Co-authored-by: Zanie Blue <contact@zanie.dev>

Update src/anyio/_backends/_asyncio.py

08d88a1

Co-authored-by: Zanie Blue <contact@zanie.dev>

oremanj reviewed Jul 14, 2023

View reviewed changes

docs/tasks.rst Show resolved Hide resolved

oremanj reviewed Jul 14, 2023

View reviewed changes

agronholm added 2 commits July 14, 2023 22:44

Improved wording of the notes on asyncio.TaskGroup

ab1b499

Simplified exception handling by never storing CancelledErrors

91ec9d1

Ensured that a yield point is always hit in TaskGroup.__aexit__()

fac9c2f

agronholm added 2 commits July 16, 2023 13:45

Removed unused functions

a70e90a

Merge branch 'master' into detect-asyncio-native-cancel

c098d9c

zanieb reviewed Jul 16, 2023

View reviewed changes

agronholm added 2 commits July 17, 2023 00:41

Added clarifying comments

8d4d653

Merge branch 'master' into detect-asyncio-native-cancel

a1b35e9

agronholm merged commit 316052e into master Jul 17, 2023

agronholm deleted the detect-asyncio-native-cancel branch July 17, 2023 20:56

rutger-clearblue mentioned this pull request Nov 24, 2023

Reinstate "collapse exception group" in TaskGroup exception handling #641

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated cancellation semantics to better match the new trio and asyncio ones #586

Updated cancellation semantics to better match the new trio and asyncio ones #586

agronholm commented Jul 12, 2023

zanieb left a comment

zanieb Jul 14, 2023

agronholm Jul 14, 2023

agronholm Jul 14, 2023

agronholm Jul 14, 2023

oremanj Jul 14, 2023

oremanj Jul 14, 2023

agronholm Jul 14, 2023

zanieb Jul 14, 2023

agronholm Jul 14, 2023

agronholm Jul 14, 2023 •

edited

Loading

oremanj Jul 14, 2023

agronholm Jul 14, 2023

oremanj Jul 14, 2023

agronholm Jul 15, 2023 •

edited

Loading

agronholm commented Jul 14, 2023

agronholm commented Jul 16, 2023

zanieb Jul 16, 2023

agronholm Jul 16, 2023

zanieb Jul 16, 2023

agronholm Jul 16, 2023

zanieb Jul 16, 2023

agronholm Jul 16, 2023

zanieb commented Jul 16, 2023

agronholm commented Jul 17, 2023

agronholm commented Jul 17, 2023

		elif exc_val is not None and not ignore_exception:
		raise exc_val

		if not waited_for_tasks_to_finish:
		await AsyncIOBackend.checkpoint()

		# Raise the CancelledError received while waiting for child tasks to exit,
		# unless the context manager itself was previously exited with another exception

		if cancelled_exc_while_waiting_tasks:
		if exc_val is None or ignore_exception:

Updated cancellation semantics to better match the new trio and asyncio ones #586

Updated cancellation semantics to better match the new trio and asyncio ones #586

Conversation

agronholm commented Jul 12, 2023

zanieb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agronholm Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agronholm Jul 15, 2023 • edited Loading

Choose a reason for hiding this comment

agronholm commented Jul 14, 2023

agronholm commented Jul 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanieb commented Jul 16, 2023

agronholm commented Jul 17, 2023

agronholm commented Jul 17, 2023

agronholm Jul 14, 2023 •

edited

Loading

agronholm Jul 15, 2023 •

edited

Loading