Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Oct 31, 2024

Conversation

Nico-Posada
Copy link
Contributor

@Nico-Posada Nico-Posada commented Oct 29, 2024

Fix for ref leak described in #126083.

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

Copy link

cpython-cla-bot bot commented Oct 29, 2024

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.

Lib/test/test_asyncio/test_tasks.py Outdated Show resolved Hide resolved
coro = coroutine_function()
task = asyncio.Task.__new__(asyncio.Task)

for _ in range(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?

Copy link
Member

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
Contributor Author

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.

Lib/test/test_asyncio/test_tasks.py Outdated Show resolved Hide resolved
@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 traceback
ok

Copy link
Member

@sobolevn sobolevn 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 :)

Lib/test/test_asyncio/test_tasks.py Show resolved Hide resolved
@willingc
Copy link
Contributor

Thanks for the detailed review @sobolevn

@Nico-Posada
Copy link
Contributor Author

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.

for _ in range(5):
try:
task.__init__(coro, loop=loop, context=obj, name=Break())
except Exception as e:
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, use with self.assertRaisesRegex(RuntimeError, 'break'):

Copy link
Contributor Author

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 _ in range(5):
    with self.assertRaisesRegex(RuntimeError, 'break'):
        task.__init__(coro, loop=loop, context=obj, name=Break())

Copy link
Contributor

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 catch and 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 catch
def foo():
	raise ReachableCode(1)

# what I don't want to catch yet
def bar():
	raise ReachableCode(2)

# where I call the thing I want to catch
with self.assertRaises(ReachableCode) as cm:
	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
Contributor Author

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

with self.assertRaisesRegex(ReachableCode, 'str here'):
    ...

and use a string in the arg instead of an int

Copy link
Contributor

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

@picnixz picnixz added needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels Oct 29, 2024
Copy link
Member

@ZeroIntensity ZeroIntensity 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

@sobolevn sobolevn 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! 🎉

@sobolevn sobolevn merged commit d07dcce into python:main Oct 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 request Oct 31, 2024
…lizing with new non-`None` context (pythonGH-126103)

(cherry picked from commit d07dcce6935364cab807e0df931ed09b088ade69)

Co-authored-by: Nico-Posada <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 31, 2024
…lizing with new non-`None` context (pythonGH-126103)

(cherry picked from commit d07dcce6935364cab807e0df931ed09b088ade69)

Co-authored-by: Nico-Posada <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Oct 31, 2024

GH-126229 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Oct 31, 2024
@bedevere-app
Copy link

bedevere-app bot commented Oct 31, 2024

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

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Oct 31, 2024
sobolevn pushed a commit that referenced this pull request Oct 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 commit d07dcce)

Co-authored-by: Nico-Posada <[email protected]>
sobolevn pushed a commit that referenced this pull request Oct 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 commit d07dcce)

Co-authored-by: Nico-Posada <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants