gh-102780: Fix uncancel() call in asyncio timeouts

[kristjanvalur]

Record the previous cancellation state when entering asyncio.timeout.Timeout
This allows us to use asyncio.timeout even when handling a CancelledError
exception. The Timeout context manager will still not handle a CancelledError
if a new cancellation request is delivered in its scope.

• Fix Timeout context manager
• Add regression tests
• Add documentiation, clarifying when it is appropriate to call Task.uncancel()

This needs to be backported to 3.11

• Issue: Catching CancelledError requires use of task.uncancel() #102780

[gvanrossum]

Thanks! This looks like the crux of the matter, and beautifully executed. I'll properly review it later this weekend.

[gvanrossum]

I only really have doc nits, plus a feeling that maybe possibly TaskGroup could have the same problem?

I'm asking @kumaraditya303 to also review this, to make sure I didn't miss anything.

[gvanrossum]

This is a nice improvement that maybe ought to be called out in the commit message? (If you agree I can handle that when I merge it.)

[kumaraditya303]

Isn't this done by default? I see the same traceback with and without this on this example:

import asyncio

async def func():
    await asyncio.sleep(1)

async def main():
    async with asyncio.timeout(0.1):
        await func()

asyncio.run(main())

[gvanrossum]

Not quite. When using raise exc from ... the CancelledError instance gets stored in exc.__cause__, while without it, it gets stored in exc.__context__, and the connecting phrase in the output is different:

__cause__:

The above exception was the direct cause of the following exception:

__context__:

During handling of the above exception, another exception occurred:

(If both are set it follows __cause__.

[kristjanvalur]

Precicely. I has bothered me for a while that timeouts happen with the latter message, as if somehow, there was an error during exception handling. I wanted to use the opportunity to roll that fix in :)

[kristjanvalur]

This is a nice improvement that maybe ought to be called out in the commit message? (If you agree I can handle that when I merge it.)

Thank you, agreed.

[kumaraditya303]

Not quite. When using raise exc from ... the CancelledError instance gets stored in exc.cause, while without it, it gets stored in exc.context, and the connecting phrase in the output is different:

Ah, right! I missed that. Adding a test would be nice.

[kristjanvalur]

I'll add that in a few hours.

[kumaraditya303]

I would like to have a test for the exception chaining else LGTM. I leave the docs for Guido to review.

@gvanrossum Feel free to merge as you like.

[AlexWaygood]

The docs read okay to me now, but I'll let Guido or Kumar merge since I'm not an asyncio expert!

[gvanrossum]

Needs a news entry, otherwise LGTM.

[kristjanvalur]

Needs a news entry, otherwise LGTM.

I'll have a go at it.

[miss-islington]

Thanks @kristjanvalur for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

[bedevere-bot]

GH-102923 is a backport of this pull request to the 3.11 branch.

[graingert]

does this need a corresponding change in asyncio.TaskGroup? eg here

cpython/Lib/asyncio/taskgroups.py

Line 82 in e90036c

if self._parent_task.uncancel() == 0:

[kristjanvalur]

Interesting. It would be an issue if we entered a TaskGroup while cancellation state was not 0, e.g. during a finally of an outer cancel. I'm not qualified, though, to judge if that would ever be a real issue.