GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-46771: Implement task cancel requests by Tinche · Pull Request #31513 · python/cpython

Conversation

@Tinche

@Tinche Tinche commented Feb 22, 2022 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Keep track of requested task cancellations as an int, instead of a bool.

https://bugs.python.org/issue46771

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Tinche
Implement task cancel requests
a231718
@Tinche
Tinche requested review from 1st1 and asvetlov as code owners February 22, 2022 23:29
@bedevere-bot bedevere-bot added the awaiting review label Feb 22, 2022
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Feb 22, 2022
@Tinche Tinche mentioned this pull request Feb 22, 2022
bpo-46771: Implement asyncio context managers for handling timeouts #31394
Merged
@gvanrossum

gvanrossum commented Feb 22, 2022

Copy link
Copy Markdown
Member

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:

  1. The cancel message. See https://bugs.python.org/issue46829. In 3.10, after t.cancel("first"); t.cancel("2nd") the cancel message ended up being set to "2nd"; with 3.11 main it ends up "first". If we remove these two lines it would become "2nd" again.
  2. How many CancelledError exceptions will the task see? In 3.10, the task may see multiple cancellations -- but it may not. In particular, if the second cancel() call finds the must-cancel flag already set, only one cancellation will be delivered. However, if the first cancellation is delivered, and the task catches the CancelledError and then uses await to do some cleanup, in 3.10 (and again if we remove those two lines) that cleanup await will be cancelled; but if we keep those two lines, that await will be shielded.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gvanrossum
gvanrossum approved these changes Feb 23, 2022
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Feb 23, 2022
asvetlov
asvetlov reviewed Feb 23, 2022
View reviewed changes
Comment thread Lib/asyncio/tasks.py
@@ -217,14 +217,12 @@ def cancel(self, msg=None):
return True

def cancelling(self):

@asvetlov asvetlov Feb 23, 2022

Copy link
Copy Markdown
Contributor

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 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.

Uh oh!

There was an error while loading. Please reload this page.

ambv reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@gvanrossum gvanrossum Feb 23, 2022

Copy link
Copy Markdown
Member

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 call uncancel() and use its return value.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@asvetlov asvetlov Feb 23, 2022

Copy link
Copy Markdown
Contributor

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 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@asvetlov asvetlov Feb 23, 2022

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@gvanrossum gvanrossum Feb 23, 2022

Copy link
Copy Markdown
Member

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 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@asvetlov asvetlov Feb 23, 2022

Copy link
Copy Markdown
Contributor

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@gvanrossum gvanrossum added the skip news label Feb 23, 2022
asvetlov
asvetlov approved these changes Feb 23, 2022
View reviewed changes
@Tinche

Tinche commented Feb 23, 2022

Copy link
Copy Markdown
Contributor Author

Great! Let me take another look if any of the method comments need rewrites.

As for cancelling(), while thinking about this PR I came to the same conclusion as you folks (that it's not needed) but I left it in. In the process I rewrote the TaskGroup very slightly to not use it (and potentially cancel its children only once). We're leaving cancelling(), but would this TaskGroup patch also be interesting?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gvanrossum

gvanrossum commented Feb 23, 2022

Copy link
Copy Markdown
Member

Yes, I am very much interested in your TaskGroup PR. There are a couple of things there that could be improved:

  • It doesn't always preserve the cancel message (since it does propagate_cancellation_error = et instead of propagate_cancellation_error = exc -- I don't know why not, could be a carry-over from the pre-3.9 days when it didn't matter).
  • There's weirdness around self._base_error. It seems that if __aexit__() receives a CancelledError (which is a base error) then that will always be raised (L124) before we get to raise propagate_cancellation_error (L130)?
  • We could, if we wanted, distinguish between a child task being cancelled by TaskGroup._abort()` and it being cancelled for some other reason. Currently, if it's the latter, the CancellationError from the child task is swallowed and treated as if it exited successfully.

There's also some weirdness around self._on_completed_fut. It is set to None twice (once in the while loop and once after the loop has finished). But this is not related to cancellation (and the issue is benign).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gvanrossum

gvanrossum commented Feb 23, 2022 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

@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).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Tinche
Tweak task docstrings
ab5bce9
@Tinche

Tinche commented Feb 24, 2022

Copy link
Copy Markdown
Contributor Author

@gvanrossum Roger, task group will be a different PR. I tweaked the docstrings, please merge if it's good.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gvanrossum
gvanrossum reviewed Feb 24, 2022
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Tinche
Run make regen-all
532a5be
@gvanrossum

gvanrossum commented Feb 24, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Oh, you have to re-run clinic (make clinic) and commit and push the resulting changes. (Done)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gvanrossum
gvanrossum merged commit 7fce106 into python:main Feb 24, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Feb 24, 2022
@gvanrossum

gvanrossum commented Feb 24, 2022

Copy link
Copy Markdown
Member

Congrats @Tinche on your first merged Python PR! Thanks for your patience. I hope this will ease the pain for Quattro.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@achimnol achimnol mentioned this pull request Feb 24, 2022
Add shutdown timeout to PersistentTaskGroup achimnol/aiotools#33
Open
@asvetlov
asvetlov deleted the issue-46771-cancel-requests-2 branch February 24, 2022 09:56
@Tinche

Tinche commented Feb 24, 2022

Copy link
Copy Markdown
Contributor Author

Thank you kindly. I will get started on a TaskGroup PR soon.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Tinche Tinche mentioned this pull request Feb 25, 2022
bpo-46752: Taskgroup tweaks #31559
Merged
@gvanrossum gvanrossum mentioned this pull request Mar 1, 2022
asyncio.Task: .cancelling() and .uncancel() now return int, not bool python/typeshed#7405
Merged
@gvanrossum gvanrossum mentioned this pull request Apr 27, 2022
Introduce task groups to asyncio and change task cancellation semantics #90908
Closed
@gvanrossum gvanrossum mentioned this pull request May 24, 2022
Implement asyncio.timeout() context manager #90927
Closed
@coderabbitai coderabbitai Bot mentioned this pull request Jul 26, 2026
0.5.3 3dg1luk43/ha_washdata#327
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossum gvanrossum gvanrossum approved these changes
@asvetlov asvetlov asvetlov approved these changes
@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@Tinche @gvanrossum @asvetlov @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.