Issue 45390: asyncio.Task doesn't propagate CancelledError() exception correctly. (original) (raw)

Created on 2021-10-06 11:30 by pagliaricci.m, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Messages (20)

msg403294 - (view)

Author: Marco Pagliaricci (pagliaricci.m)

Date: 2021-10-06 11:30

I've spotted a little bug in how asyncio.CancelledError() exception is propagated inside an asyncio.Task.

Since python 3.9 the asyncio.Task.cancel() method has a new 'msg' parameter, that will create an asyncio.CancelledError(msg) exception incorporating that message.

The exception is successfully propagated to the coroutine the asyncio.Task is running, so the coroutine successfully gets raised an asyncio.CancelledError(msg) with the specified message in asyncio.Task.cancel(msg) method.

But, once the asyncio.Task is cancelled, is impossible to retrieve that original asyncio.CancelledError(msg) exception with the message, because it seems that a new asyncio.CancelledError() [without the message] is raised when asyncio.Task.result() or asyncio.Task.exception() methods are called.

I have the feeling that this is just wrong, and that the original message specified in asyncio.Task.cancel(msg) should be propagated even also asyncio.Task.result() is called.

I'm including a little snippet of code that clearly shows this bug.

I'm using python 3.9.6, in particular: Python 3.9.6 (default, Aug 21 2021, 09:02:49) [GCC 10.2.1 20210110] on linux

msg403297 - (view)

Author: Ben (bjs) *

Date: 2021-10-06 11:36

This seems to be present in both the Python implementation as well as the accelerated C _asyncio module.

It looks like that when a Task awaits a cancelled future, the task itself is cancelled but the cancellation message is not propagated to the task. https://github.com/python/cpython/blob/main/Lib/asyncio/tasks.py#L242

msg403350 - (view)

Author: Chris Jerdonek (chris.jerdonek) * (Python committer)

Date: 2021-10-07 06:46

But, once the asyncio.Task is cancelled, is impossible to retrieve that original asyncio.CancelledError(msg) exception with the message, because it seems that a new asyncio.CancelledError() [without the message] is raised when asyncio.Task.result() or asyncio.Task.exception() methods are called.

You say it's "impossible", but isn't the message accessible via the exception chain (and visible in the traceback)? One benefit of not duplicating the message on the internal call to cancel() is that it makes it easier to pinpoint which CancelledError object is associated with the user's call to cancel(), and which is associated with the call done by asyncio internals, which is a different cancellation. Another benefit is that it prevents info from being duplicated in the traceback.

msg403360 - (view)

Author: Thomas Grainger (graingert) *

Date: 2021-10-07 08:25

afaik this is intentional https://bugs.python.org/issue31033

msg403521 - (view)

Author: Marco Pagliaricci (pagliaricci.m)

Date: 2021-10-09 07:38

OK, I see your point. But I still can't understand one thing and I think it's very confusing:

  1. if you see my example, inside the job() coroutine, I get correctly cancelled with an asyncio.CancelledError exception containing my message.
  2. Now: if I re-raise the asyncio.CancelledError as-is, I lose the message, if I call the asyncio.Task.exception() function.
  3. If I raise a new asyncio.CancelledError with a new message, inside the job() coroutine's except asyncio.CancelledError: block, I still lose the message if I call asyncio.Task.exception().
  4. But if I raise another exception, say raise ValueError("TEST"), always from the except asyncio.CancelledError: block of the job() coroutine, I get the message! I get ValueError("TEST") by calling asyncio.Task.exception(), while I don't with the asyncio.CancelledError() one.

Is this really wanted? Sorry, but I still find this a lot confusing. Shouldn't it be better to return from the asyncio.Task.exception() the old one (containing the message) ? Or, otherwise, create a new instance of the exception for ALL the exception classes?

Thank you for your time, My Best Regards,

M.

On Thu, Oct 7, 2021 at 10:25 AM Thomas Grainger <report@bugs.python.org> wrote:

Thomas Grainger <tagrain@gmail.com> added the comment:

afaik this is intentional https://bugs.python.org/issue31033

nosy: +graingert

Python tracker <report@bugs.python.org> <https://bugs.python.org/issue45390>

msg403530 - (view)

Author: Chris Jerdonek (chris.jerdonek) * (Python committer)

Date: 2021-10-09 12:51

  1. Now: if I re-raise the asyncio.CancelledError as-is, I lose the message, if I call the asyncio.Task.exception() function.

Re-raise asyncio.CancelledError where? (And what do you mean by "re-raise"?) Call asyncio.Task.exception() where? This isn't part of your example, so it's not clear what you mean exactly.

msg403531 - (view)

Author: Marco Pagliaricci (pagliaricci.m)

Date: 2021-10-09 13:00

Chris, I'm attaching to this e-mail the code I'm referring to. As you can see, in line 10, I re-raise the asyncio.CancelledError exception with a message "TEST". That message is lost, due to the reasons we've talked about.

My point is that, if we substitute that line 10, with the commented line 11, and we comment the line 10, so we raise a ValueError("TEST") exception, as you can see, the message "TEST" is NOT LOST. I just find this counter-intuitive, and error-prone.

AT LEAST should be very well specified in the docs.

Regards, M.

On Sat, Oct 9, 2021 at 2:51 PM Chris Jerdonek <report@bugs.python.org> wrote:

Chris Jerdonek <chris.jerdonek@gmail.com> added the comment:

  1. Now: if I re-raise the asyncio.CancelledError as-is, I lose the message, if I call the asyncio.Task.exception() function.

Re-raise asyncio.CancelledError where? (And what do you mean by "re-raise"?) Call asyncio.Task.exception() where? This isn't part of your example, so it's not clear what you mean exactly.

Python tracker <report@bugs.python.org> <https://bugs.python.org/issue45390>

msg403532 - (view)

Author: Chris Jerdonek (chris.jerdonek) * (Python committer)

Date: 2021-10-09 13:06

I still don't see you calling asyncio.Task.exception() in your new attachment...

msg403533 - (view)

Author: Marco Pagliaricci (pagliaricci.m)

Date: 2021-10-09 13:16

Chris, ok, I have modified the snippet of code to better show what I mean. Still here, the message of the CancelledError exception is lost, but if I comment line 10, and uncomment line 11, so I throw a ValueError("TEST"), that "TEST" string will be printed, so the message is not lost. Again, I just find this behavior very counter-intuitive, and should be VERY WELL documented in the docs.

Thanks, M.

On Sat, Oct 9, 2021 at 3:06 PM Chris Jerdonek <report@bugs.python.org> wrote:

Chris Jerdonek <chris.jerdonek@gmail.com> added the comment:

I still don't see you calling asyncio.Task.exception() in your new attachment...

Python tracker <report@bugs.python.org> <https://bugs.python.org/issue45390>

msg403570 - (view)

Author: Chris Jerdonek (chris.jerdonek) * (Python committer)

Date: 2021-10-10 04:13

Here's a simplification of Marco's snippet to focus the discussion.

import asyncio

async def job(): # raise RuntimeError('error!') await asyncio.sleep(5)

async def main(): task = asyncio.create_task(job()) await asyncio.sleep(1) task.cancel('cancel job') await task

if name=="main": asyncio.run(main())

Running this pre-Python 3.9 gives something like this--

Traceback (most recent call last): File "test.py", line 15, in asyncio.run(main()) File "/.../python3.7/asyncio/runners.py", line 43, in run return loop.run_until_complete(main) File "/.../python3.7/asyncio/base_events.py", line 579, in run_until_complete return future.result() concurrent.futures._base.CancelledError

Running this with Python 3.9+ gives something like the following. The difference is that the traceback now starts at the sleep() call:

Traceback (most recent call last): File "/.../test.py", line 6, in job await asyncio.sleep(5) File "/.../python3.9/asyncio/tasks.py", line 654, in sleep return await future asyncio.exceptions.CancelledError: cancel job

During handling of the above exception, another exception occurred:

Traceback (most recent call last): File "/.../test.py", line 12, in main await task asyncio.exceptions.CancelledError

During handling of the above exception, another exception occurred:

Traceback (most recent call last): File "/.../test.py", line 15, in asyncio.run(main()) File "/.../python3.9/asyncio/runners.py", line 44, in run return loop.run_until_complete(main) File "/.../python3.9/asyncio/base_events.py", line 642, in run_until_complete return future.result() asyncio.exceptions.CancelledError

Uncommenting the RuntimeError turns it into this--

Traceback (most recent call last): File "/.../test.py", line 15, in asyncio.run(main()) File "/.../python3.9/asyncio/runners.py", line 44, in run return loop.run_until_complete(main) File "/.../python3.9/asyncio/base_events.py", line 642, in run_until_complete return future.result() File "/.../test.py", line 12, in main await task File "/.../test.py", line 5, in job raise RuntimeError('error!') RuntimeError: error!

I agree it would be a lot nicer if the original CancelledError('cancel job') could bubble up just like the RuntimeError does, instead of creating a new CancelledError at each await and chaining it to the previous CancelledError. asyncio's creation of a new CancelledError at each stage predates the PR that added the chaining, so this could be viewed as an evolution of the change that added the chaining.

I haven't checked to be sure, but the difference in behavior between CancelledError and other exceptions might be explained by the following lines: https://github.com/python/cpython/blob/3d1ca867ed0e3ae343166806f8ddd9739e568ab4/Lib/asyncio/tasks.py#L242-L250 You can see that for exceptions other than CancelledError, the exception is propagated by calling super().set_exception(exc), whereas with CancelledError, it is propagated by calling super().cancel() again.

Maybe this would even be an easy change to make. Instead of asyncio creating a new CancelledError and chaining it to the previous, asyncio can just raise the existing one. For the pure Python implementation at least, it may be as simple as making a change here, inside _make_cancelled_error(): https://github.com/python/cpython/blob/3d1ca867ed0e3ae343166806f8ddd9739e568ab4/Lib/asyncio/futures.py#L135-L142

msg413392 - (view)

Author: Andrew Svetlov (asvetlov) * (Python committer)

Date: 2022-02-17 09:26

I have a pull request for the issue. It doesn't use Future.set_exception() but creates a new CancelledError() with propagated message. The result is the same, except raised exceptions are not comparable by is check. As a benefit, _cancelled_exc works with the patch, exc.context is correctly set.

The patch is not backported because it changes existing behavior a little. I'd like to avoid a situation when third-party code works with Python 3.11+, 3.10.3+, and 3.9.11+ only.

msg413395 - (view)

Author: Marco Pagliaricci (pagliaricci.m)

Date: 2022-02-17 09:37

Andrew, many thanks for your time, solving this issue. I think your solution is the best to fix this little problem and I agree with you on backporting.

My Best Regards, and thanks again.

Marco

On Thu, Feb 17, 2022 at 10:29 AM Andrew Svetlov <report@bugs.python.org> wrote:

Andrew Svetlov <andrew.svetlov@gmail.com> added the comment:

I have a pull request for the issue. It doesn't use Future.set_exception() but creates a new CancelledError() with propagated message. The result is the same, except raised exceptions are not comparable by is check. As a benefit, _cancelled_exc works with the patch, exc.context is correctly set.

The patch is not backported because it changes existing behavior a little. I'd like to avoid a situation when third-party code works with Python 3.11+, 3.10.3+, and 3.9.11+ only.

Python tracker <report@bugs.python.org> <https://bugs.python.org/issue45390>

msg413397 - (view)

Author: Chris Jerdonek (chris.jerdonek) * (Python committer)

Date: 2022-02-17 10:23

Andrew, the approach I described would I feel be much better. It would result in more concise, less verbose tracebacks, as opposed to more verbose -- not just because the message won't be repeated, but also because it eliminates the unneeded creation of intermediate exceptions. It would also cause is checks to work, which is what we want since behaviorally it should be the same exception.

msg413679 - (view)

Author: Andrew Svetlov (asvetlov) * (Python committer)

Date: 2022-02-21 20:59

New changeset 4140bcb1cd76dec5cf8d398f4d0e86c438c987d0 by Andrew Svetlov in branch 'main': bpo-45390: Propagate CancelledError's message from cancelled task to its awaiter (GH-31383) https://github.com/python/cpython/commit/4140bcb1cd76dec5cf8d398f4d0e86c438c987d0

msg413779 - (view)

Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer)

Date: 2022-02-23 09:20

Seems a CancelledError message can be lost also in Condition.wait().

msg413794 - (view)

Author: Chris Jerdonek (chris.jerdonek) * (Python committer)

Date: 2022-02-23 14:56

For future reference, with Andrew's change merged above, the traceback for the example snippet in my message above: https://bugs.python.org/issue45390#msg403570 is now the following. Observe that (1) the call to sleep() continues to be present, but (2) without introducing two new intermediate CancelledErrors, which increase the verbosity of the traceback:

Traceback (most recent call last): File "/home/andrew/projects/cpython/exc_traceback.py", line 14, in asyncio.run(main()) ^^^^^^^^^^^^^^^^^^^ File "/home/andrew/projects/cpython/Lib/asyncio/runners.py", line 44, in run return loop.run_until_complete(main) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/andrew/projects/cpython/Lib/asyncio/base_events.py", line 640, in run_until_complete return future.result() ^^^^^^^^^^^^^^^ File "/home/andrew/projects/cpython/exc_traceback.py", line 11, in main await task ^^^^^^^^^^ File "/home/andrew/projects/cpython/exc_traceback.py", line 5, in job await asyncio.sleep(5) ^^^^^^^^^^^^^^^^^^^^^^ File "/home/andrew/projects/cpython/Lib/asyncio/tasks.py", line 619, in sleep return await future ^^^^^^^^^^^^ asyncio.exceptions.CancelledError: cancel job

(This is copied from Andrew's comment in the PR here: https://github.com/python/cpython/pull/31383#issuecomment-1046822899 )

Serhiy, can you provide a sample snippet for your case with output, like I did in my message linked above?

msg413808 - (view)

Author: Andrew Svetlov (asvetlov) * (Python committer)

Date: 2022-02-23 15:29

Serhiy is right, Condition.wait() has the following code:

    finally:
        # Must reacquire lock even if wait is cancelled
        cancelled = False
        while True:
            try:
                await self.acquire()
                break
            except exceptions.CancelledError:
                cancelled = True

        if cancelled:
            raise exceptions.CancelledError

It swallows CancelledError exceptions from waiters and re-raises CancelledError without the cancellation message.

msg413830 - (view)

Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer)

Date: 2022-02-23 17:27

Also Future.result() and Future.exception() can raise a CancelledError. So a CancelledError raised in a task may not contain a message passed to Task.cancel().

import asyncio import random

async def main(): fut = asyncio.Future() fut.cancel() async def job(): if random.random() < 0.5: await asyncio.sleep(2) fut.result() await asyncio.sleep(5) task = asyncio.create_task(job()) await asyncio.sleep(1) task.cancel("cancel task") await task

asyncio.run(main())

You need to catch a CancelledError raised in a coroutine and re-raise a new CancelledError with the specified cancel message if the task was cancelled.

msg413836 - (view)

Author: Thomas Grainger (graingert) *

Date: 2022-02-23 18:02

there could be multiple messages here

perhaps it could be:

        finally:
            # Must reacquire lock even if wait is cancelled
            cancelled = []
            while True:
                try:
                    await self.acquire()
                    break
                except exceptions.CancelledError as e:
                    cancelled.append(e)

            if len(cancelled) > 1:
                raise ExceptionGroup("Cancelled", cancelled)
            if cancelled:
                raise cancelled[0]

msg413838 - (view)

Author: Guido van Rossum (gvanrossum) * (Python committer)

Date: 2022-02-23 18:04

We should really stop appending to a closed issue.

Anyway, raising ExceptionGroup is backwards incompatible, since "except CancelledError" wouldn't cancel the group.

History

Date

User

Action

Args

2022-04-11 14:59:50

admin

set

github: 89553

2022-02-23 18:04:42

gvanrossum

set

messages: +

2022-02-23 18:02:00

graingert

set

messages: +

2022-02-23 17:27:45

serhiy.storchaka

set

messages: +

2022-02-23 15:29:45

asvetlov

set

messages: +

2022-02-23 14:56:27

chris.jerdonek

set

messages: +

2022-02-23 09:20:59

serhiy.storchaka

set

nosy: + serhiy.storchaka
messages: +

2022-02-22 09:46:40

asvetlov

set

status: open -> closed
resolution: fixed
stage: patch review -> resolved

2022-02-21 20:59:10

asvetlov

set

messages: +

2022-02-19 04:39:29

gvanrossum

set

nosy: + gvanrossum

2022-02-17 10:23:56

chris.jerdonek

set

messages: +

2022-02-17 09:37:54

pagliaricci.m

set

messages: +

2022-02-17 09:26:57

asvetlov

set

messages: +

2022-02-17 09:20:19

asvetlov

set

keywords: + patch
stage: patch review
pull_requests: + <pull%5Frequest29531>

2022-02-17 09:20:10

asvetlov

set

versions: + Python 3.11, - Python 3.9

2021-10-10 04:13:19

chris.jerdonek

set

messages: +

2021-10-09 13:16:22

pagliaricci.m

set

files: + task_bug.py

messages: +

2021-10-09 13:06:02

chris.jerdonek

set

messages: +

2021-10-09 13:00:14

pagliaricci.m

set

files: + task_bug.py

messages: +

2021-10-09 12:51:00

chris.jerdonek

set

messages: +

2021-10-09 07:38:23

pagliaricci.m

set

messages: +

2021-10-07 08:25:18

graingert

set

nosy: + graingert
messages: +

2021-10-07 06:46:58

chris.jerdonek

set

nosy: + chris.jerdonek
messages: +

2021-10-06 11:36:52

bjs

set

nosy: + bjs
messages: +

2021-10-06 11:30:42

pagliaricci.m

create