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 — gh-109955 : Update state transition comments for asyncio.Task by kristjanvalur · Pull Request #109910 · python/cpython

Conversation

@kristjanvalur

@kristjanvalur kristjanvalur commented Sep 26, 2023 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

The in-line comments for the state transition of asyncio.Task objects has been out of date
for quite some time.

  • _fut_waiter persist in a done state on the Task between becoming done and until __step() is executed.
  • There is an intermediate state 2a where __wakeup() is scheduled, followed by 2b when __step() is scheduled.
  • An unmentioned state exists for the Task, namely the running state.
  • _step() and _wakeup() have been __step() and __wakeup() for quite some time.

This PR attempts to rectify the situation.

  • Issue: Inline comments about Task state in asyncio.tasks are out of date #109955

Sorry, something went wrong.

Uh oh!

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

All reactions
@kristjanvalur
Update state transition comment for Task
3b9b4cf
@kristjanvalur
kristjanvalur force-pushed the kristjan/task-comments branch from 093b5c5 to 3b9b4cf Compare September 26, 2023 16:51
@kristjanvalur
update comment with states 2a and 2b
3cc8ca9
@kristjanvalur kristjanvalur mentioned this pull request Sep 27, 2023
Inline comments about Task state in asyncio.tasks are out of date #109955
Closed
@kristjanvalur kristjanvalur changed the title Update state transition comments for asyncio.Task gh-109955 : Update state transition comments for asyncio.Task Sep 27, 2023
@kristjanvalur
kristjanvalur marked this pull request as ready for review September 27, 2023 09:43
@kristjanvalur
kristjanvalur requested review from 1st1, asvetlov, gvanrossum, kumaraditya303 and willingc as code owners September 27, 2023 09:43
@bedevere-app bedevere-app Bot added the awaiting review label Sep 27, 2023
@AlexWaygood AlexWaygood added the skip news label Sep 27, 2023
AA-Turner
AA-Turner reviewed Sep 27, 2023
View reviewed changes
Comment thread Lib/asyncio/tasks.py Resolved

Uh oh!

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

gvanrossum
gvanrossum reviewed Sep 27, 2023
View reviewed changes
Comment thread Lib/asyncio/tasks.py Resolved

Uh oh!

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

Comment thread Lib/asyncio/tasks.py Outdated
Comment on lines +90 to +91
# The transition from 2a to 2b happens when __wakeup() is executed,
# scheduling __step() to be called, leaving _fut_waiter in place.

@gvanrossum gvanrossum Sep 27, 2023

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.

This confuses me. Once __wakeup() executes, it proceeds without waiting to call __step() -- there's no scheduling involved that I can see in the source code (unless the .c extension is different). The call to future.result() immediately produces a value or raises -- it doesn't block (there's no await there). I do think it is possible to enter step 2b without going through 2a when __step() is scheduled directly by __init__(). So maybe it's clearer to say that we go through either 2a or 2b?

Sorry, something went wrong.

Uh oh!

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

All reactions

@kristjanvalur kristjanvalur Sep 27, 2023

Copy link
Copy Markdown
Contributor Author

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 may have got my wires crossed here, because it is all confusing. Initially, I thought __wakeup() was called directly (futures just calling registered callbacks) and _wakeup scheduling __step,.. But then it turns out that futures schedule their callbacks... I'll have another look.

Sorry, something went wrong.

Uh oh!

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

All reactions

@kristjanvalur kristjanvalur Sep 27, 2023

Copy link
Copy Markdown
Contributor Author

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.

Yes, you were right. So, there is no 2a or2b anymore just 2.
The main point I'm trying to get across here is that there can be a _fut_waiter present in a done state, i.e. the
mere presence of a _fut_waiter does not indicate that the Task is still blocked. I hope I'm not making this sound more complicated than it is.

Sorry, something went wrong.

Uh oh!

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

All reactions

@gvanrossum gvanrossum Sep 27, 2023

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 mere presence of a _fut_waiter does not indicate that the Task is still blocked

Yes, that's crucial information.

Sorry, something went wrong.

Uh oh!

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

All reactions
kristjanvalur and others added 2 commits September 27, 2023 21:17
@kristjanvalur @AA-Turner
Update Lib/asyncio/tasks.py
e3f07bb
Co-authored-by: Adam Turner <[email protected]>
@kristjanvalur
Fix incorrect detail. __wakeup() calls __step() directly
729a291
gvanrossum
gvanrossum approved these changes Sep 27, 2023
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.

I'll merge it now. Thanks!

Sorry, something went wrong.

Uh oh!

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

kristjanvalur reacted with hooray emoji
All reactions
  • 🎉 1 reaction
@bedevere-app bedevere-app Bot added awaiting core review and removed awaiting review labels Sep 27, 2023
@gvanrossum gvanrossum added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Sep 27, 2023
@gvanrossum
gvanrossum merged commit 45cf5b0 into python:main Sep 27, 2023
@miss-islington

miss-islington commented Sep 27, 2023

Copy link
Copy Markdown
Contributor

Thanks @kristjanvalur for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-app bedevere-app Bot removed the awaiting core review label Sep 27, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 27, 2023
@kristjanvalur @AA-Turner @miss-islington
pythongh-109955 : Update state transition comments for asyncio.Task (p…
9669321
…ythonGH-109910)

(cherry picked from commit 45cf5b0)

Co-authored-by: Kristján Valur Jónsson <[email protected]>
Co-authored-by: Adam Turner <[email protected]>
@bedevere-app

bedevere-app Bot commented Sep 27, 2023

Copy link
Copy Markdown

GH-109992 is a backport of this pull request to the 3.12 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-app bedevere-app Bot removed the needs backport to 3.12 only security fixes label Sep 27, 2023
@bedevere-app

bedevere-app Bot commented Sep 27, 2023

Copy link
Copy Markdown

GH-109993 is a backport of this pull request to the 3.11 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-app bedevere-app Bot removed the needs backport to 3.11 only security fixes label Sep 27, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 27, 2023
@kristjanvalur @AA-Turner @miss-islington
pythongh-109955 : Update state transition comments for asyncio.Task (p…
f04c4d5
…ythonGH-109910)

(cherry picked from commit 45cf5b0)

Co-authored-by: Kristján Valur Jónsson <[email protected]>
Co-authored-by: Adam Turner <[email protected]>
@kristjanvalur
kristjanvalur deleted the kristjan/task-comments branch September 27, 2023 22:25
csm10495 pushed a commit to csm10495/cpython that referenced this pull request Sep 28, 2023
@kristjanvalur @AA-Turner @csm10495
pythongh-109955 : Update state transition comments for asyncio.Task (p…
bd9e1cb
…ython#109910)

Co-authored-by: Adam Turner <[email protected]>
vstinner pushed a commit that referenced this pull request Sep 29, 2023
@miss-islington @kristjanvalur @AA-Turner
[3.11] gh-109955 : Update state transition comments for asyncio.Task (G…
4b97c72
…H-109910) (#109993)

gh-109955 : Update state transition comments for asyncio.Task (GH-109910)
(cherry picked from commit 45cf5b0)

Co-authored-by: Kristján Valur Jónsson <[email protected]>
Co-authored-by: Adam Turner <[email protected]>
Yhg1s pushed a commit that referenced this pull request Oct 2, 2023
@miss-islington @kristjanvalur @AA-Turner
[3.12] gh-109955 : Update state transition comments for asyncio.Task (G…
dd67e59
…H-109910) (#109992)

gh-109955 : Update state transition comments for asyncio.Task (GH-109910)
(cherry picked from commit 45cf5b0)

Co-authored-by: Kristján Valur Jónsson <[email protected]>
Co-authored-by: Adam Turner <[email protected]>
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@kristjanvalur @AA-Turner @Glyphack
pythongh-109955 : Update state transition comments for asyncio.Task (p…
729adb0
…ython#109910)

Co-authored-by: Adam Turner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AA-Turner AA-Turner AA-Turner left review comments
@gvanrossum gvanrossum gvanrossum approved these changes
@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner
@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner
@kumaraditya303 kumaraditya303 Awaiting requested review from kumaraditya303 kumaraditya303 is a code owner
@willingc willingc Awaiting requested review from willingc willingc 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

@kristjanvalur @miss-islington @gvanrossum @AA-Turner @AlexWaygood
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 01402520 — 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.