bpo-45390: Propagate CancelledError's message from cancelled task to its awaiter

[asvetlov]

https://bugs.python.org/issue45390

[cjerdonek]

I don't think this is the right approach to addressing this issue and was considered when first implementing the msg argument. I described what I think should be done in the tracker: https://bugs.python.org/issue45390#msg403570
It will have the desired effect and result in more concise, less verbose tracebacks.

[asvetlov]

@cjerdonek do you suggest switching to set_exception() usage?
I can do it easily.

[cjerdonek]

@cjerdonek do you suggest switching to set_exception() usage?

Yes, thank you, @asvetlov. I believe that's the change. I'm not sure if any other changes would need to be made, but I suspect it would be small.

[cjerdonek]

@asvetlov It's also possible that the correct change is to leave that section of the code alone and instead change _make_cancelled_error() to return self._cancelled_exc when self._cancelled_exc is set instead of creating a new CancelledError. I would need to study the code more carefully to be sure. Either way, I suspect the code change would be small.

[asvetlov]

Sorry, bare set_exception() doesn't work: it switched the state to FINISHED, not CANCELLED.
Tweaking _make_cencelled_error() to return self._cancelled_exc if present breaks tests that checks exception chaining (get_innermost_context() calls different stack depth).
I'm not sure how is it important.
You can experiment with the PR easily as you have the write access to pull requests.

[cjerdonek]

Tweaking _make_cencelled_error() to return self._cancelled_exc if present breaks tests that checks exception chaining (get_innermost_context() calls different stack depth).

Yes, that's fine and is the expected (and desired) result of the change. It will eliminate unneeded chaining and instead result in a single exception bubbling up. Thus, test expectations expecting a chain of length greater than 1 will instead have a chain of length 1.

The important things to check are that the line at which the exception starts (origin of the exception) is preserved, and that the message was set correctly.

[asvetlov]

@cjerdonek please review again

[cjerdonek]

Thanks a lot, @asvetlov. That looks great. Would you mind also running the example I gave at the beginning of the following message:
https://bugs.python.org/issue45390#msg403570
and pasting the output into a comment here? That way people will be able to see the result of this PR from an end-user perspective and compare it to the outputs I pasted into that same message in the tracker.

The example is as follows for convenience:

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())

[cjerdonek]

I think my only comment on the code is if at least one of the tests that previously involved chaining (ideally all of them) could somehow check that the traceback is starting where the exception was first raised, as opposed to dropping those inner frames. Previously, that was being checked in the tests by the depth. But since those depths are now all 0 (which is what we want), we need to use a different way of checking that the origin of the exception is getting preserved in the traceback. Maybe this could be done by modifying get_innermost_context() to also return something about the innermost frame, and then checking that is as expected.

[asvetlov]

Test added.

exc_traceback.py:

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())

the output:

Traceback (most recent call last):
  File "/home/andrew/projects/cpython/exc_traceback.py", line 14, in <module>
    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

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @asvetlov for commit 0e99dc8 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

[serhiy-storchaka]

Only the msg parameter or the exception? Is the latter an implementation detail?

[gvanrossum]

IIUC there are no guarantees about the identity of the exception object.

My personal opinion is that the whole "cancelled message" concept was a mistake, but it's too late to roll it back.

[cjerdonek]

I don't think msg needs to be mentioned in the versionchanged, IMO. The PR is more about eliminating unneeded intermediate CancelledError's, resulting in a more compact traceback, etc. In other words, this PR would be useful even without the msg argument. It's more a side effect that (in most cases), the msg will bubble out.

[asvetlov]

Could you propose better text, please?
The change is user-faced, the most visible difference is keeping/swallowing the cancellation message on crossing tasks border.

[cjerdonek]

Okay, then I'm okay with what you had.

[gvanrossum]

Once Serhiy agrees with this I am fine with it. He is an excellent reviewer.

[gvanrossum]

IIUC there are no guarantees about the identity of the exception object.

My personal opinion is that the whole "cancelled message" concept was a mistake, but it's too late to roll it back.

[asvetlov]

@gvanrossum whether the cancellation message is a design error or not -- it is 'half broken' now.
The PR fixes one part of the problem. Another part is two (or more) .cancel(msg) calls in the same loop iteration but with different cancellation messages.
Let me please create a separate issue for discussion, I have a feeling that that fix is independent of the current PR.

[cjerdonek]

Thanks for all your work on this, @asvetlov.

[asvetlov]

Thank you guys for careful review.