Skip to content

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

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
Merged
miss-islington merged 2 commits into from
Dec 4, 2019
Merged

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

miss-islington merged 2 commits into from
Dec 4, 2019

Conversation

@pablogsal
Copy link
Member

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

@pablogsal
Copy link
MemberAuthor

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

vstinner
vstinner reviewed Dec 4, 2019
View reviewed changes
@pablogsal
Copy link
MemberAuthor

pablogsal commented Dec 4, 2019

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

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

@vstinnervstinner 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-islingtonmiss-islington merged commit ac0e1c2 into python:masterDec 4, 2019
@pablogsalpablogsal 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
@shihai1991shihai1991 mentioned this pull request Aug 17, 2020
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinnervstinnervstinner approved these changes

Assignees

@pablogsalpablogsal

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