gh-140860: Clear audit hooks before destroying modules #140861
Conversation
StanFromIreland
requested review from
ZeroIntensity and
ericsnowcurrently
as code owners
Python/pylifecycle.c
Outdated
| if (_Py_IsMainInterpreter(tstate->interp)) { | ||
| Py_CLEAR(tstate->interp->audit_hooks); | ||
| } |
There was a problem hiding this comment.
Two things:
- This looks like valid per-interpreter state; why is it done only for the main interpreter?
- Clearing interpreter fields should generally happen in
PyInterpreterState_Clear, not_Py_Finalize.
There was a problem hiding this comment.
Also, why are we clearing it after running a GC collection?
There was a problem hiding this comment.
Clearing interpreter fields should generally happen in
PyInterpreterState_Clear, not_Py_Finalize.
It is too late then, once the module is destroyed it leaks.
There was a problem hiding this comment.
I don't follow. A module object should clear all references it owns before it's destroyed. Even if it isn't, Py_CLEAR on its own shouldn't fix that.
Can you walk me through the leak to help me understand what's going on?
There was a problem hiding this comment.
In the original repro, interp->audit_hooks holds references to the hook, hook, which seems to prevent time.struct_time's deallocation by the GC when the module is destroyed.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
There was a problem hiding this comment.
This doesn't seem like the right fix. See this comment on a similar issue; rearranging Py_CLEAR calls will probably lead to other issues.
As far as I can tell, the leak here comes from the fact that the audit hook isn't in the last generation when calling _PyGC_CollectNoFail for the last time. That can be solved by adding an extra garbage collection in PyInterpreterState_Clear, but that doesn't address the case for when a finalizer installs new audit hooks after we've already cleared them.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
I see, as such I will close. |
3 participants
Uh oh!
There was an error while loading. Please reload this page.