Fix asyncio.Task.uncancel() being called too early
#790
Conversation
| self._pending_uncancellations: int | None | ||
| if sys.version_info >= (3, 11): | ||
| self._pending_uncancellations = 0 | ||
| else: | ||
| self._pending_uncancellations = None |
There was a problem hiding this comment.
Are you sure we're okay with not looking at the task cancellation counter when entering the scope? I think there was a reason why asyncio.timeout() does that.
There was a problem hiding this comment.
IIUC the reason that we checked cancelling() when entering the scope was #606, but that case is now covered by the __context__ walk.
There was a problem hiding this comment.
I will check asyncio.timeout and see if I can find another reason for this though.
There was a problem hiding this comment.
There was a problem hiding this comment.
It's possible that we don't need this, as we rely on the cancel messages rather than the cancel counter, as I believe you meant by that previous comment of yours.
There was a problem hiding this comment.
Potentially related? https://github.com/python/cpython/pull/102815/files#diff-0a6da8818df8b29c3c649167aa37c906eb27283c5fe1cacb40e36a6ce0458e7eR87
It's possible that we don't need this, as we rely on the cancel messages rather than the cancel counter, as I believe you meant by that previous comment of yours.
Yes, that is my understanding. IIUC, the only reason that a asyncio.Timeout checks cancelling() during __enter__ is so that it can use the delta-cancelling() value to decide whether a CancelledError that it sees during __exit__ should be converted by it to a TimeoutError or not.
An asyncio.Timeout does not, for example, vary the number of uncancel() calls that it makes based on the value cancelling_value_during_enter. An asyncio.Timeout just uses cancelling_value_during_enter to detect if a CancelledError that it sees is "owned by it" or not.
We are doing the analogous version of this detection via cancel messages instead of via cancelling() values.
For example, in my experiment where I deleted uncancel and cancelling from asyncio.Task and got all of CPython's asyncio tests to pass by using cancel messages instead, the literal replacement for asyncio.Timeout's cancelling() check was a check of the cancellation message: python/cpython@v3.12.6...f8348de#diff-0a6da8818df8b29c3c649167aa37c906eb27283c5fe1cacb40e36a6ce0458e7eR127.
Changes
This is a small patch that fixes the more obscure case that we skipped yesterday (#774 (comment)).
After a night's rest I woke up this morning with the realization that this case shouldn't involve whack-a-mole as we feared. IIUC it seems to be relatively straightforward to fix.
d342ae1 is the core patch here in its simplest form. The subsequent two commits are some minor follow-up refactoring for performance optimization:
Avoid doing repeated computations of
_effectively_cancelled&_parent_cancellation_is_visible_to_usinCancelScope.__exit__(I missed this during review).Move the counter back from
TaskStatetoCancelScope. This makes the impl. a little harder to understand IMO (as this makes the impl. diverge a bit further from Trio's model of delaying deciding which scope a level cancellation exception belongs to), but it may(?) save a little memory (as the counter is only needed for host tasks).Checklist
If this is a user-facing code change, like a bugfix or a new feature, please ensure that
you've fulfilled the following conditions (where applicable):
tests/) added which would fail without your patchdocs/, in case of behavior changes or newfeatures)
docs/versionhistory.rst).