Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.7k
bpo-46771: Implement task cancel requests#31513
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
gvanrossum commentedFeb 22, 2022
So this needs to be reviewed carefully by@agronholm and@Tinche -- not so much regarding "code review" but regarding "does this address our concerns, does it fix the edge cases, can we implement Trio-like behavior in 3rd party libraries when we need it". I'd also like@cjerdonek to think about this. We also, separately, need to debate whether the lines ifself._num_cancels_requested>1:returnFalse should stay in or not. IIRC@agronholm has claimed that this (or the previous incarnation) broke several AnyIO tests. There are two aspects that would change if we removed these two lines:
|
gvanrossum left a comment
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.
The more I think about it, the more I believe we should keep the two lines in cancel(), so I am merging this as-is. Thanks@Tinche. (Your first PR merged into CPython, I believe -- congrats!)
| self._cancel_message=msg | ||
| returnTrue | ||
| defcancelling(self): |
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.
Is thecancelling() the best name for returning the number or cancel requests?
It was good for bool flag, a counter needs better name IMHO.
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.
Hm, do you have a suggestion? I was thinking that it's still usable as a "truthy" value. Usually when you're interested in the cancel count you should calluncancel() and use its return value.
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 have no good answer.cancel_requests_count() describes exact meaning but the name is too long.cancelling() can return bool but it looks like procrastination: why return less info than we really have.
The third option is droppingcancelling() method entirely, it is not required by the current asyncio code.
We can consider adding the method later on-demand.
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.
Note:.cancelling() doesn't exist in Python 3.10
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.
It's used in TaskGroup currently, so if we were to delete it we'd have to rewrite the code there in this same PR. It's also used byTask.__repr__() (via_task_repr_info()). But of those will work even though we upgraded it from a bool to an int. I'd say let's keep it for now, if we regret it we can rename or remove it before beta 1.
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.
Deal!
Tinche commentedFeb 23, 2022
Great! Let me take another look if any of the method comments need rewrites. As for |
gvanrossum commentedFeb 23, 2022
Yes, I am very much interested in your TaskGroup PR. There are a couple of things there that could be improved:
There's also some weirdness around |
gvanrossum commentedFeb 23, 2022 • edited by bedevere-bot
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by bedevere-bot
Uh oh!
There was an error while loading.Please reload this page.
Tinche commentedFeb 24, 2022
@gvanrossum Roger, task group will be a different PR. I tweaked the docstrings, please merge if it's good. |
gvanrossum left a comment
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.
Great! Will merge now (for real).
gvanrossum commentedFeb 24, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Oh, you have to re-run clinic ( |
gvanrossum commentedFeb 24, 2022
Congrats@Tinche on your first merged Python PR! Thanks for your patience. I hope this will ease the pain for Quattro. |
Tinche commentedFeb 24, 2022
Thank you kindly. I will get started on a TaskGroup PR soon. |
Uh oh!
There was an error while loading.Please reload this page.
Keep track of requested task cancellations as an int, instead of a bool.
https://bugs.python.org/issue46771