Skip to content

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

Merged
gvanrossum merged 3 commits intopython:mainfrom
frostbyte134:asyncio_open_connection_refcycle
Nov 22, 2022
Merged

gh-88863: clear ref cycles to resolve leak when asyncio.open_connection raises#95739
gvanrossum merged 3 commits intopython:mainfrom
frostbyte134:asyncio_open_connection_refcycle

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

@ghost
Copy link

ghost commented Aug 6, 2022 •
edited by ghost
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!

@gvanrossum gvanrossum merged commit 995f617 into python:main Nov 22, 2022
@miss-islington
Copy link
Contributor

miss-islington commented Nov 22, 2022

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

miss-islington commented Nov 22, 2022

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

miss-islington commented Nov 22, 2022

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

gvanrossum commented Nov 22, 2022

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

bedevere-bot commented Nov 23, 2022

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

bedevere-bot commented Nov 23, 2022

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

frostbyte134 commented Nov 23, 2022

@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