Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-126083: Fix a reference leak inasyncio.Task when reinitializing with new non-None context#126103

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

Merged
sobolevn merged 4 commits intopython:mainfromNico-Posada:fix-issue-126083
Oct 31, 2024

Conversation

@Nico-Posada
Copy link
Contributor

@Nico-PosadaNico-Posada commentedOct 29, 2024•
edited by bedevere-appbot
Loading

Fix for ref leak described in#126083.

This is my first CPython PR so feedback is appreciated :)

@ghost
Copy link

ghost commentedOct 29, 2024•
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@willingc
Copy link
Contributor

Adding@sobolevn to the reviews since they may have some context from looking at the issue.

@Nico-Posada Congrats on working on tackling your first contribution.

coro=coroutine_function()
task=asyncio.Task.__new__(asyncio.Task)

for_inrange(10000):
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need 10000 iterations to find a leak?

Choose a reason for hiding this comment

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

Well, maybe not 10000, but finding leaks is certainly easier with more iterations. Maybe 1000 should do?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I changed it to 5, the bug would be triggered every time so with a low amount you should still be able to see the ref leak.

@willingc
Copy link
Contributor

Seeing this output when running tests locally:

test_proper_refcounts (test.test_asyncio.test_tasks.PyTask_PyFuture_SubclassTests.test_proper_refcounts) ... /Users/willingc/Code/cpython/Lib/unittest/case.py:606: RuntimeWarning: coroutine'coroutine_function' was never awaited  result =method()RuntimeWarning: Enable tracemalloc to get the object allocation tracebackok

Copy link
Member

@sobolevnsobolevn left a comment

Choose a reason for hiding this comment

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

To fix the warning :)

@willingc
Copy link
Contributor

Thanks for the detailed review@sobolevn

sobolevn reacted with heart emoji

@Nico-Posada
Copy link
ContributorAuthor

Ah that's annoying, I thought I was signed in to my GitHub account when I made that commit. Is there any way to revert this to change ownership? Not sure if I can resign the CLA with that account.

@ZeroIntensity
Copy link
Member

I've done that before too, yeah. I think the only way is to overwrite the commit locally and then force push it.

Nico-Posada reacted with thumbs up emoji

for_inrange(5):
try:
task.__init__(coro,loop=loop,context=obj,name=Break())
exceptExceptionase:
Copy link
Member

Choose a reason for hiding this comment

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

question: do we expect this exception in all cases? if so, usewith self.assertRaisesRegex(RuntimeError, 'break'):

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

So update it to be this?

for_inrange(5):withself.assertRaisesRegex(RuntimeError,'break'):task.__init__(coro,loop=loop,context=obj,name=Break())

sobolevn reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

FTR: In order to catch the correct exception in#126120, I used a custom exception + a special argument so that I'm sure that I catchand expect the correct one (and not just a random one because of something else). So what I did was something like:

# what I want to catchdeffoo():raiseReachableCode(1)# what I don't want to catch yetdefbar():raiseReachableCode(2)# where I call the thing I want to catchwithself.assertRaises(ReachableCode)ascm:something_that_calls_foo()self.assertEqual(len(cm.exception.args),1)self.assertIs(cm.exception.args[0],1)

I find this pattern quite good (maybe a bit too exhaustive) because I can really see what I'm catching.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I like the idea of the ReachableCode exception class, but I still think it's easier and more readable to just do

withself.assertRaisesRegex(ReachableCode,'str here'):    ...

and use a string in the arg instead of an int

Copy link
Member

Choose a reason for hiding this comment

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

I think I'll go with that as well to simplify the tests on my side (but not today).

@picnixzpicnixz added needs backport to 3.12only security fixes needs backport to 3.13bugs and security fixes labelsOct 29, 2024
Copy link
Member

@ZeroIntensityZeroIntensity left a comment

Choose a reason for hiding this comment

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

LGTM as well

Copy link
Member

@sobolevnsobolevn left a comment

Choose a reason for hiding this comment

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

Thanks you! Congrats on your first CPython PR! 🎉

willingc reacted with hooray emoji
@sobolevnsobolevn merged commitd07dcce intopython:mainOct 31, 2024
43 checks passed
@miss-islington-app
Copy link

Thanks@Nico-Posada for the PR, and@sobolevn for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestOct 31, 2024
…lizing with new non-`None` context (pythonGH-126103)(cherry picked from commitd07dcce)Co-authored-by: Nico-Posada <102486290+Nico-Posada@users.noreply.github.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestOct 31, 2024
…lizing with new non-`None` context (pythonGH-126103)(cherry picked from commitd07dcce)Co-authored-by: Nico-Posada <102486290+Nico-Posada@users.noreply.github.com>
@bedevere-app
Copy link

GH-126229 is a backport of this pull request to the3.13 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.13bugs and security fixes labelOct 31, 2024
@bedevere-app
Copy link

GH-126230 is a backport of this pull request to the3.12 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.12only security fixes labelOct 31, 2024
sobolevn pushed a commit that referenced this pull requestOct 31, 2024
…alizing with new non-`None` context (GH-126103) (#126229)gh-126083: Fix a reference leak in `asyncio.Task` when reinitializing with new non-`None` context (GH-126103)(cherry picked from commitd07dcce)Co-authored-by: Nico-Posada <102486290+Nico-Posada@users.noreply.github.com>
sobolevn pushed a commit that referenced this pull requestOct 31, 2024
…alizing with new non-`None` context (GH-126103) (#126230)gh-126083: Fix a reference leak in `asyncio.Task` when reinitializing with new non-`None` context (GH-126103)(cherry picked from commitd07dcce)Co-authored-by: Nico-Posada <102486290+Nico-Posada@users.noreply.github.com>
coro.close()
deltask

self.assertEqual(sys.getrefcount(obj),initial_refcount)
Copy link
Contributor

@kumaraditya303kumaraditya303Nov 2, 2024•
edited
Loading

Choose a reason for hiding this comment

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

Please avoid tests for exact refcounts, the test should test that no references are leaked rather than reference count should be equal, both are not same thing. In this particular case all of this seems unnecessary, our existing refleak checker is sufficient for this.

willingc reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Oh I was unaware of the existing refleak checker. Do you have any examples of where it's used so i can check it out?

willingc reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

You can runpython -m test XXX -R : (it's with-R :)

Nico-Posada and willingc reacted with thumbs up emoji
picnixz pushed a commit to picnixz/cpython that referenced this pull requestDec 8, 2024
ebonnal pushed a commit to ebonnal/cpython that referenced this pull requestJan 12, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@picnixzpicnixzpicnixz left review comments

@kumaraditya303kumaraditya303kumaraditya303 left review comments

@1st11st11st1 approved these changes

@willingcwillingcwillingc approved these changes

@sobolevnsobolevnsobolevn approved these changes

@ZeroIntensityZeroIntensityZeroIntensity approved these changes

@asvetlovasvetlovAwaiting requested review from asvetlovasvetlov is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@Nico-Posada@willingc@ZeroIntensity@1st1@sobolevn@picnixz@kumaraditya303
[8]ページ先頭
©2009-2025 Movatter.jp