-
-
Notifications
You must be signed in to change notification settings - Fork 30.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bpo-45390: Propagate CancelledError's message from cancelled task to its awaiter #31383
Conversation
I don't think this is the right approach to addressing this issue and was considered when first implementing the |
Co-authored-by: Serhiy Storchaka <[email protected]>
@cjerdonek do you suggest switching to |
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. |
@asvetlov It's also possible that the correct change is to leave that section of the code alone and instead change |
Sorry, bare |
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. |
@cjerdonek please review again |
Thanks a lot, @asvetlov. That looks great. Would you mind also running the example I gave at the beginning of the following message: 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()) |
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 |
Test added.
the output:
|
@@ -843,6 +843,9 @@ Task Object | |||
.. versionchanged:: 3.9 | |||
Added the *msg* parameter. | |||
|
|||
.. versionchanged:: 3.11 | |||
The ``msg`` parameter is propagated from cancelled task to its awaiter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the msg parameter or the exception? Is the latter an implementation detail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, then I'm okay with what you had.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once Serhiy agrees with this I am fine with it. He is an excellent reviewer.
@@ -843,6 +843,9 @@ Task Object | |||
.. versionchanged:: 3.9 | |||
Added the *msg* parameter. | |||
|
|||
.. versionchanged:: 3.11 | |||
The ``msg`` parameter is propagated from cancelled task to its awaiter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@gvanrossum whether the cancellation message is a design error or not -- it is 'half broken' now. |
Thanks for all your work on this, @asvetlov. |
Thank you guys for careful review. |
https://bugs.python.org/issue45390