Skip to content

bpo-36333: Fix leak in _PyRuntimeState_Fini#12400

Merged
vstinner merged 1 commit intopython:masterfrom
matrixise:bpo-36333-fix_refleak__PyRuntimeState_Fini
Mar 19, 2019
Merged

bpo-36333: Fix leak in _PyRuntimeState_Fini#12400
vstinner merged 1 commit intopython:masterfrom
matrixise:bpo-36333-fix_refleak__PyRuntimeState_Fini

Conversation

@matrixise
Copy link
Member

@matrixise matrixise commented Mar 18, 2019 •
edited by bedevere-bot
Loading

@matrixise matrixise mentioned this pull request Mar 18, 2019
Closed
vstinner
vstinner approved these changes Mar 18, 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, but I would prefer that @ericsnowcurrently review it since he added _PyRuntime.xidregistry (and its mutex).

@vstinner
Copy link
Member

vstinner commented Mar 18, 2019

Python 3.7 is not affected: it doesn't have _PyRuntime.xidregistry.

eamanu
eamanu approved these changes Mar 18, 2019
View reviewed changes
Copy link
Contributor

@eamanu eamanu left a comment

Choose a reason for hiding this comment

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

LGTM!

ericsnowcurrently
ericsnowcurrently approved these changes Mar 18, 2019
View reviewed changes
Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks for working on this!

@ericsnowcurrently
Copy link
Member

ericsnowcurrently commented Mar 18, 2019

FWIW, _PyRuntime.ceval.pending.lock may be leaking too.

@vstinner
Copy link
Member

vstinner commented Mar 18, 2019

FWIW, _PyRuntime.ceval.pending.lock may be leaking too.

@matrixise: Can you try to free _PyRuntime.ceval.pending.lock in _PyEval_FiniThreads()?

@matrixise
Copy link
Member Author

matrixise commented Mar 18, 2019

@ericsnowcurrently I will check in an other PR. Thanks for the review

@matrixise
Copy link
Member Author

matrixise commented Mar 18, 2019

Ok i will do it

@vstinner vstinner merged commit 943395f into python:master Mar 19, 2019
@vstinner
Copy link
Member

vstinner commented Mar 19, 2019

I merged this PR, thanks @matrixise.

Oh, there are now too many leaks... I merged https://bugs.python.org/issue36356 PR and I wrote PR #12432 to free the pending lock, to have a better view of remaining leaks.

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

@ericsnowcurrently ericsnowcurrently ericsnowcurrently approved these changes

+1 more reviewer

@eamanu eamanu eamanu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@matrixise @vstinner @ericsnowcurrently @eamanu @the-knights-who-say-ni @bedevere-bot