GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||
GeezerVison is showing github.com — bpo-46771: Implement task cancel requests by Tinche · Pull Request #31513 · python/cpython Conversation
| |||||||||||||||||||||
|
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 if self._num_cancels_requested > 1: return False 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:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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!)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| @@ -217,14 +217,12 @@ def cancel(self, msg=None): | |||
| return True | |||
|
|
|||
| def cancelling(self): | |||
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Is the cancelling() the best name for returning the number or cancel requests?
It was good for bool flag, a counter needs better name IMHO.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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 call uncancel() and use its return value.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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 dropping cancelling() method entirely, it is not required by the current asyncio code.
We can consider adding the method later on-demand.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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 by Task.__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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Deal!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Great! Let me take another look if any of the method comments need rewrites. As for |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
@Tinche I forgot to mention: I'm waiting for you before I land it. If you don't mind I'd rather see the TaskGroup changes in a separate PR (since this PR is complete as-is and the TaskGroup improvements should probably be tagged with bpo-46752). |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@gvanrossum Roger, task group will be a different PR. I tweaked the docstrings, please merge if it's good. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Great! Will merge now (for real).
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Oh, you have to re-run clinic ( |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Congrats @Tinche on your first merged Python PR! Thanks for your patience. I hope this will ease the pain for Quattro. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thank you kindly. I will get started on a TaskGroup PR soon. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.