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

bpo-38962: Fix reference leak in the per-subinterpreter gc #17457

Merged
merged 2 commits into from
Dec 4, 2019
Merged

bpo-38962: Fix reference leak in the per-subinterpreter gc #17457

merged 2 commits into from
Dec 4, 2019

Conversation

pablogsal
Copy link
Member

@pablogsal pablogsal commented Dec 3, 2019 •
edited by miss-islington
Loading

@pablogsal
Copy link
Member Author

I'm not super convinced about this PR, but indeed it fixes the reference leaks in test__xxsubinterpreters (when all other PRs in https://bugs.python.org/issue38962 are applied first):

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

== Tests result: SUCCESS ==

1 test OK.

Total duration: 17.6 sec
Tests result: SUCCESS

@pablogsal pablogsal requested a review from vstinner December 3, 2019 22:15
vstinner
vstinner reviewed Dec 4, 2019
View reviewed changes
@pablogsal
Copy link
Member Author

@vstinner I have modified the PR to trigger the collection only on sub interpreters. Can you check it out?

@pablogsal pablogsal mentioned this pull request Dec 4, 2019
Closed
@pablogsal pablogsal self-assigned this Dec 4, 2019
vstinner
vstinner reviewed Dec 4, 2019
View reviewed changes
@pablogsal pablogsal force-pushed the bpo-38962-4 branch 2 times, most recently from 0e8f8c5 to 4f9a89b Compare December 4, 2019 11:29
vstinner
vstinner approved these changes Dec 4, 2019
View reviewed changes
Copy link
Member

@vstinner vstinner 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 I wrote, I'm not fully satisfied by calling _PyGC_CollectNoFail() after PyInterpreterState_Clear() because of the risk of bugs in objects finalizers, but we cannot fix all Python finalization issues at once. Let's unblock the buildbots, and only later investigate for a better design.

@miss-islington
Copy link
Contributor

@pablogsal: Status check is done, and it's a success ✅ .

@miss-islington miss-islington merged commit ac0e1c2 into python:master Dec 4, 2019
@pablogsal pablogsal deleted the bpo-38962-4 branch December 4, 2019 11:51
jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019
@pablogsal
bpo-38962: Fix reference leak in the per-subinterpreter gc (pythonGH-…
dbc4070 
…17457)

https://bugs.python.org/issue38962



Automerge-Triggered-By: @pablogsal
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
@pablogsal @shihai1991
bpo-38962: Fix reference leak in the per-subinterpreter gc (pythonGH-…
f9c3578 
…17457)

https://bugs.python.org/issue38962



Automerge-Triggered-By: @pablogsal
@shihai1991 shihai1991 mentioned this pull request Aug 17, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner approved these changes

Assignees

@pablogsal pablogsal

Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pablogsal @miss-islington @vstinner @the-knights-who-say-ni @bedevere-bot