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.

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

gh-88863: clear ref cycles to resolve leak when asyncio.open_connection raises #95739

Merged
merged 3 commits into from
Nov 22, 2022
Merged

gh-88863: clear ref cycles to resolve leak when asyncio.open_connection raises #95739

merged 3 commits into from
Nov 22, 2022

Conversation

frostbyte134
Copy link
Contributor

@frostbyte134 frostbyte134 commented Aug 6, 2022 •
edited by bedevere-bot
Loading

Break reference cycles to resolve #88863 by removing local exception and future (which has exception as its member var) instances from the frame. I believe those changes will not have bad effects, as they are just removing local variables which are not used anymore.

  • In Linux/MacOS, changes in asyncio.selector_events / asyncio.base_events are relevant (except for the last change in asyncio.base_events.server)
  • In Windows, changes in asyncio.windows_events / asyncio.base_events are relevant

Checked in Windows 10/Mac Monterey/Centos 7.9

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Aug 6, 2022 •
edited
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@frostbyte134 frostbyte134 mentioned this pull request Aug 6, 2022
Closed
kumaraditya303
kumaraditya303 requested changes Oct 28, 2022
View reviewed changes
Copy link
Contributor

@kumaraditya303 kumaraditya303 left a comment

Choose a reason for hiding this comment

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

A rebase is required otherwise LGTM

@kumaraditya303 kumaraditya303 added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Oct 28, 2022
@frostbyte134
break ref cycles when asyncio.open_connection raises
b5bf956 
Break reference cycles to resolve memory leak, by
removing local exception and future instances from the frame
@frostbyte134 frostbyte134 force-pushed the asyncio_open_connection_refcycle branch from a94e8c1 to b5bf956 Compare November 21, 2022 17:56
@frostbyte134
Copy link
Contributor Author

frostbyte134 commented Nov 21, 2022 •
edited
Loading

Thank you for the review @kumaraditya303, and sorry for my late response.
I rebased the PR.

gvanrossum
gvanrossum approved these changes Nov 22, 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.

LGTM, I will fix the grammar in the NEWS file and merge. Thank you for your patch!

Misc/NEWS.d/next/Library/2022-08-06-12-18-07.gh-issue-88863.NnqsuJ.rst Outdated Show resolved Hide resolved
@gvanrossum gvanrossum merged commit 995f617 into python:main Nov 22, 2022
@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

Sorry, @frostbyte134 and @gvanrossum, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 995f6170c78570eca818f7e7dbd8a7661c171a81 3.11

@miss-islington
Copy link
Contributor

Sorry @frostbyte134 and @gvanrossum, I had trouble checking out the 3.10 backport branch.
Please retry by removing and re-adding the "needs backport to 3.10" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker 995f6170c78570eca818f7e7dbd8a7661c171a81 3.10

@gvanrossum
Copy link
Member

If you want to try manually backporting these, please go ahead.

frostbyte134 added a commit to frostbyte134/cpython that referenced this pull request Nov 23, 2022
@frostbyte134
[3.11] pythongh-88863: Clear ref cycles to resolve leak when asyncio.…
c812c49 
…open_connection raises (pythonGH-95739)

Break reference cycles to resolve memory leak, by
removing local exception and future instances from the frame.
(cherry picked from commit 995f617)

Co-authored-by: Dong Uk, Kang <nailbrainz@gmail.com>
frostbyte134 added a commit to frostbyte134/cpython that referenced this pull request Nov 23, 2022
@frostbyte134
[3.10] pythongh-88863: Clear ref cycles to resolve leak when asyncio.…
5e913d5 
…open_connection raises (pythonGH-95739)

Break reference cycles to resolve memory leak, by
removing local exception and future instances from the frame.
(cherry picked from commit 995f617)

Co-authored-by: Dong Uk, Kang <nailbrainz@gmail.com>
@bedevere-bot
Copy link

GH-99721 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 Nov 23, 2022
@bedevere-bot
Copy link

GH-99722 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 Nov 23, 2022
@frostbyte134
Copy link
Contributor Author

@gvanrossum I just created 2 manual backport PRs to 3.10 and 3.11. Thank you for the review and fix!

This was referenced Nov 23, 2022
Merged
Merged
gvanrossum pushed a commit that referenced this pull request Nov 23, 2022
@frostbyte134
[3.11] gh-88863: Clear ref cycles to resolve leak when asyncio.open_c…
24fad64 
…onnection raises (GH-95739) (#99721)

Break reference cycles to resolve memory leak, by
removing local exception and future instances from the frame.
(cherry picked from commit 995f617)

Co-authored-by: Dong Uk, Kang <nailbrainz@gmail.com>
gvanrossum pushed a commit that referenced this pull request Nov 23, 2022
@frostbyte134
[3.10] gh-88863: Clear ref cycles to resolve leak when asyncio.open_c…
b191bc3 
…onnection raises (GH-95739) (#99722)

Break reference cycles to resolve memory leak, by
removing local exception and future instances from the frame.
(cherry picked from commit 995f617)

Co-authored-by: Dong Uk, Kang <nailbrainz@gmail.com>
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

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

@gvanrossum gvanrossum

Labels
topic-asyncio
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Memory leak when asyncio.open_connection raise
5 participants
@frostbyte134 @miss-islington @gvanrossum @bedevere-bot @kumaraditya303