bpo-31901: atexit callbacks should be run at subinterpreter shutdown · Pull Request #4611 · python/cpython (original) (raw)
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
This PR moves the atexit callbacks from Python runtime context to interpreter state.
Also, Py_EndInterpreter() now calls atexit callbacks of the interpreter.
These changes make it possible to implement PEP-489 multiphase initialization for atexit module, which is also done in this PR.
https://bugs.python.org/issue31901
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A nit, but it would seem to me more conventional to take the user data (the PyObject *) as second argument.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it actually necessary to use C code for this? As opposed to calling support.run_in_subinterp from test_atexit (as already done in two other tests).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, didn't know about this thing, it is rewritten in Python now.
Thank you for the PR! I posted a couple comments.
There was a crash on AppVeyor. I've retried the bulld in case it's an unrelated sporadic issue (?).
Looks like the AppVeyor crash isn't sporadic after all (and your PR is the only one to trigger it). Do you have a Windows machine or VM to test?
By the way, analyzing this PR led me to #4826 :-)
Unfortunately, I don't have any Windows machine.
About #4826, the _Py_PyAtExit should really not be called from anywhere else than atexit module, otherwise, some sort of list instead of single pointer would be needed. Should I have a look at that as a part of this PR?
PR #4826 was merged, so you just have to rebase/merge this PR on git master (you will have to resolve conflicts, which should be relatively easy here).
And about the Windows crash?
If it persists after git master is merged in, I'll try to reproduce on my Windows VM (if I manage to unstuck it).
Marcel Plch added 2 commits
Ok, I think I've found the culprit for the Windows crash: now that you're using PEP 489 multiphase init support, modstate can be NULL in m_traverse and m_clear (and probably m_free too), so you have to guard against this.
@pitrou this PR is ready for review
This looks good to me. I'm gonna merge soon if no other core developer chimes in.
The documentation issue is tracked in bpo-32374.
This was referenced
Aug 24, 2018