Skip to content

gh-94972: document that shield users need to keep a reference to their task #96724

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.

Sign up for GitHub

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

Jump to bottom
Merged
gvanrossum merged 7 commits into from
Sep 10, 2022
Merged

gh-94972: document that shield users need to keep a reference to their task #96724

gvanrossum merged 7 commits into from
Sep 10, 2022

Conversation

@hendrikmakait
Copy link
Contributor

@hendrikmakait hendrikmakait commented Sep 9, 2022 •
edited
Loading

I will take over the original PR (#94973) from @graingert to get it over the finish line.

@bedevere-bot bedevere-bot added awaiting review docs Documentation in the Doc dir skip news labels Sep 9, 2022
@hendrikmakait hendrikmakait mentioned this pull request Sep 9, 2022
Closed
@ghost
Copy link

ghost commented Sep 9, 2022 •
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

gvanrossum
gvanrossum approved these changes Sep 9, 2022
View reviewed changes
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for finishing this! I'll merge now.

@gvanrossum
Copy link
Member

gvanrossum commented Sep 9, 2022

Oh wait, it's still in draft. I'll leave it up to you to remove draft mode; ping the PR and I'll merge it then.

@hendrikmakait hendrikmakait marked this pull request as ready for review September 10, 2022 06:54
@hendrikmakait
Copy link
Contributor Author

hendrikmakait commented Sep 10, 2022

@gvanrossum: Moved out of draft, thanks for the early review.

@gvanrossum gvanrossum merged commit 6281aff into python:main Sep 10, 2022
@gvanrossum gvanrossum added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Sep 10, 2022
@miss-islington
Copy link
Contributor

miss-islington commented Sep 10, 2022

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

@miss-islington
Copy link
Contributor

miss-islington commented Sep 10, 2022

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

@bedevere-bot
Copy link

bedevere-bot commented Sep 10, 2022

GH-96736 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Sep 10, 2022
@bedevere-bot
Copy link

bedevere-bot commented Sep 10, 2022

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

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Sep 10, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 10, 2022
@hendrikmakait @miss-islington
pythongh-94972: document that shield users need to keep a reference t…
e2f7c8c 
…o their task (pythonGH-96724)

Co-authored-by: Thomas Grainger <[email protected]>
Co-authored-by: Guido van Rossum <[email protected]>
(cherry picked from commit 6281aff)

Co-authored-by: Hendrik Makait <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 10, 2022
@hendrikmakait @miss-islington
pythongh-94972: document that shield users need to keep a reference t…
13de3da 
…o their task (pythonGH-96724)

Co-authored-by: Thomas Grainger <[email protected]>
Co-authored-by: Guido van Rossum <[email protected]>
(cherry picked from commit 6281aff)

Co-authored-by: Hendrik Makait <[email protected]>
@gvanrossum
Copy link
Member

gvanrossum commented Sep 10, 2022

Thanks Hendrik for finishing this up!

Thomas I hope you’re doing okay.

miss-islington added a commit that referenced this pull request Sep 10, 2022
@miss-islington @hendrikmakait
gh-94972: document that shield users need to keep a reference to thei…
9b71058 
…r task (GH-96724)

Co-authored-by: Thomas Grainger <[email protected]>
Co-authored-by: Guido van Rossum <[email protected]>
(cherry picked from commit 6281aff)

Co-authored-by: Hendrik Makait <[email protected]>
miss-islington added a commit that referenced this pull request Sep 10, 2022
@miss-islington @hendrikmakait
gh-94972: document that shield users need to keep a reference to thei…
335bd1e 
…r task (GH-96724)

Co-authored-by: Thomas Grainger <[email protected]>
Co-authored-by: Guido van Rossum <[email protected]>
(cherry picked from commit 6281aff)

Co-authored-by: Hendrik Makait <[email protected]>
@Dreamsorcerer
Copy link
Contributor

Dreamsorcerer commented May 2, 2023

Sorry that I'm bringing this up so late. But, is this actually needed?

It seems to me that shield() has a reference to the task, so this should not be needed (i.e. It returns outer, which has a reference to _outer_done_callback, which has a reference to inner in the enclosing scope, which is the task. As long as you have a reference to outer (i.e. by awaiting it), I don't think inner can be garbage collected).

If I'm wrong about that somewhere, shouldn't we just fix shield() to keep a reference, rather than asking the user to make this extra, non-obvious step every time they use it?

@graingert
Copy link
Contributor

graingert commented May 2, 2023

@Dreamsorcerer when the result of shield is cancelled the reference to the original task is dropped

@Dreamsorcerer
Copy link
Contributor

Dreamsorcerer commented May 2, 2023

Ah, yes, fair enough. Still feels like an explicit reference could be kept on outer to avoid this though.

But, with the proposed workaround, wouldn't the function that holds the task reference also likely be dropped (i.e. If the CancelledError bubbles up, the entire function scope will be discarded shortly thereafter)?
Seems like you'd need to put the reference somewhere globally or as an attribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@graingert graingert graingert approved these changes

@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

Assignees

No one assigned

Labels

docs Documentation in the Doc dir skip news topic-asyncio

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@hendrikmakait @gvanrossum @miss-islington @bedevere-bot @Dreamsorcerer @graingert @AlexWaygood