Skip to content

/ cpython Public

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

Jump to bottom

bpo-44856: Possible reference leak in error paths of update_bases() and __build_class__ #27647

Merged
merged 2 commits into from Aug 7, 2021
Merged

bpo-44856: Possible reference leak in error paths of update_bases() and __build_class__ #27647

merged 2 commits into from Aug 7, 2021

Conversation

@pablogsal
Copy link
Member

@pablogsal pablogsal commented Aug 7, 2021 •
edited by bedevere-bot

https://bugs.python.org/issue44856
@pablogsal pablogsal force-pushed the bpo-44856 branch 2 times, most recently from 18f0d16 to bfc121a Aug 7, 2021
@pablogsal pablogsal requested a review from ambv Aug 7, 2021
@pablogsal
Copy link
Member Author

@pablogsal pablogsal commented Aug 7, 2021 •
edited

With this PR:

❯ ./python -m test test_typing -R :
0:00:00 load avg: 0.54 Run tests sequentially
0:00:00 load avg: 0.54 [1/1] test_typing
beginning 9 repetitions
123456789
.........

== Tests result: SUCCESS ==

1 test OK.

Total duration: 1.0 sec
Tests result: SUCCESS

@pablogsal pablogsal changed the title bpo-44856: Fix a possible reference leak in error paths of update_bases() Possible reference leak in error paths of update_bases() and __build_class__ Aug 7, 2021
@pablogsal
bpo-44856: Fix reference leaks in error paths of __build_class__ and …
8d4fe18 
…update_bases()
@pablogsal pablogsal changed the title Possible reference leak in error paths of update_bases() and __build_class__ bpo-44856: Possible reference leak in error paths of update_bases() and __build_class__ Aug 7, 2021
@pablogsal
Copy link
Member Author

@pablogsal pablogsal commented Aug 7, 2021 •
edited

This could have been a nightmare to track down since the leak happens in very old code that is quite complex but funny enough, I tracked this super efficiently with the memory profiler that allows to track Python and C at the same time I am building at work with my team:

example (2)

CC: @pradyunsg @lkollar @mariocj89

@pablogsal
Update Misc/NEWS.d/next/Core and Builtins/2021-08-07-01-26-12.bpo-448…
0ecf91b 
…56.9rk3li.rst
Fidget-Spinner
Fidget-Spinner reviewed Aug 7, 2021
View changes
Copy link
Contributor

@Fidget-Spinner Fidget-Spinner left a comment

FWIW, I tested this on Windows and it also fixes the refleaks. Thanks Pablo, this LGTM.

python_d.exe -m test test_typing -R 3:3
0:00:00 Run tests sequentially
0:00:00 [1/1] test_typing
beginning 6 repetitions
123456
......

== Tests result: SUCCESS ==

1 test OK.

serhiy-storchaka
serhiy-storchaka approved these changes Aug 7, 2021
View changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

LGTM.

@ambv ambv merged commit a40675c into python:main Aug 7, 2021
13 checks passed
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Aug 7, 2021

Thanks @pablogsal for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9, 3.10.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Aug 7, 2021

Sorry @pablogsal and @ambv, I had trouble checking out the 3.10 backport branch.
Please backport using cherry_picker on command line.
cherry_picker a40675c659cd8c0699f85ee9ac31660f93f8c2f5 3.10

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Aug 7, 2021

GH-27651 is a backport of this pull request to the 3.9 branch.

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Aug 7, 2021

GH-27652 is a backport of this pull request to the 3.8 branch.

miss-islington added a commit to miss-islington/cpython that referenced this issue Aug 7, 2021
@miss-islington
bpo-44856: Possible reference leak in error paths of update_bases() a…
89a40f2 
…nd __build_class__ (pythonGH-27647)

(cherry picked from commit a40675c)

Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Aug 7, 2021

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

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Aug 7, 2021

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

miss-islington added a commit to miss-islington/cpython that referenced this issue Aug 7, 2021
@miss-islington
bpo-44856: Possible reference leak in error paths of update_bases() a…
291596f 
…nd __build_class__ (pythonGH-27647)

(cherry picked from commit a40675c)

Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>
ambv pushed a commit that referenced this issue Aug 7, 2021
@miss-islington
bpo-44856: Possible reference leak in error paths of update_bases() a…
0a42309 
…nd __build_class__ (GH-27647) (GH-27652)

(cherry picked from commit a40675c)

Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>
ambv pushed a commit that referenced this issue Aug 7, 2021
@miss-islington
bpo-44856: Possible reference leak in error paths of update_bases() a…
ed718e9 
…nd __build_class__ (GH-27647) (GH-27651)

(cherry picked from commit a40675c)

Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>
miss-islington added a commit that referenced this issue Aug 7, 2021
@miss-islington
bpo-44856: Possible reference leak in error paths of update_bases() a…
ac8f72c 
…nd __build_class__ (GH-27647)

(cherry picked from commit a40675c)

Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>
@sakibguy sakibguy mentioned this pull request Aug 7, 2021
Merged
JuniorJPDJ added a commit to JuniorJPDJ/cpython that referenced this issue Aug 12, 2021
@JuniorJPDJ
bpo-44856: Possible reference leak in error paths of update_bases() a…
d1383fe 
…nd __build_class__ (pythonGH-27647)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fidget-Spinner Fidget-Spinner

@serhiy-storchaka serhiy-storchaka

@vstinner vstinner

@ambv ambv

Assignees

@ambv ambv

Labels
CLA signed
Projects
None yet
Milestone
No milestone
7 participants
@pablogsal @miss-islington @bedevere-bot @serhiy-storchaka @Fidget-Spinner @ambv @the-knights-who-say-ni