Skip to content

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

Merged
ambv merged 2 commits intopython:mainfrom
pablogsal:bpo-44856
Aug 7, 2021
Merged

bpo-44856: Possible reference leak in error paths of update_bases() and __build_class__#27647
ambv merged 2 commits intopython:mainfrom
pablogsal:bpo-44856

Conversation

@pablogsal
Copy link
Member

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

@pablogsal pablogsal force-pushed the bpo-44856 branch 2 times, most recently from 18f0d16 to bfc121a Compare August 7, 2021 01:20
@pablogsal pablogsal requested a review from ambv August 7, 2021 01:22
@pablogsal
Copy link
Member Author

pablogsal commented Aug 7, 2021 •
edited
Loading

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 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 commented Aug 7, 2021 •
edited
Loading

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

Fidget-Spinner
Fidget-Spinner reviewed Aug 7, 2021
View reviewed changes
Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

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 reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

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

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 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 commented Aug 7, 2021

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

@bedevere-bot
Copy link

bedevere-bot commented Aug 7, 2021

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 7, 2021
@pablogsal @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>
@ambv ambv added needs backport to 3.10 only security fixes and removed needs backport to 3.10 only security fixes labels Aug 7, 2021
@miss-islington
Copy link
Contributor

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 bedevere-bot removed the needs backport to 3.10 only security fixes label Aug 7, 2021
@bedevere-bot
Copy link

bedevere-bot commented Aug 7, 2021

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 7, 2021
@pablogsal @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 pull request Aug 7, 2021
@miss-islington @pablogsal
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 pull request Aug 7, 2021
@miss-islington @pablogsal
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 pull request Aug 7, 2021
@miss-islington @pablogsal
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Fidget-Spinner Fidget-Spinner Fidget-Spinner left review comments

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@vstinner vstinner Awaiting requested review from vstinner

@ambv ambv Awaiting requested review from ambv

Assignees

@ambv ambv

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@pablogsal @miss-islington @bedevere-bot @serhiy-storchaka @Fidget-Spinner @ambv @the-knights-who-say-ni