Skip to content

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

Merged
miss-islington merged 2 commits intopython:masterfrom
pablogsal:bpo-38962-4
Dec 4, 2019
Merged

bpo-38962: Fix reference leak in the per-subinterpreter gc#17457
miss-islington merged 2 commits intopython:masterfrom
pablogsal:bpo-38962-4

Conversation

@pablogsal
Copy link
Member

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

@pablogsal
Copy link
Member Author

pablogsal commented Dec 3, 2019

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

pablogsal commented Dec 4, 2019

@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

miss-islington commented Dec 4, 2019

@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