PEP 489: module m_traverse called with NULL module state
Hello, After debugging a crash on AppVeyor for a submitter's PR (see https://github.com/python/cpython/pull/4611 ), I came to the following diagnosis: converting the "atexit" module (which is a built-in C extension) to PEP 489 multiphase initialization can lead to its m_traverse function (and presumably also m_clear and m_free) to be called while not module state is yet registered: that is, `PyModule_GetState(self)` when called from m_traverse returns NULL! Is that an expected or known subtlety? Regards Antoine.
On 14 Dec. 2017 9:19 am, "Antoine Pitrou" <solipsis@pitrou.net> wrote: Hello, After debugging a crash on AppVeyor for a submitter's PR (see https://github.com/python/cpython/pull/4611 ), I came to the following diagnosis: converting the "atexit" module (which is a built-in C extension) to PEP 489 multiphase initialization can lead to its m_traverse function (and presumably also m_clear and m_free) to be called while not module state is yet registered: that is, `PyModule_GetState(self)` when called from m_traverse returns NULL! Is that an expected or known subtlety? Not that I'm aware of, so I'd be inclined to classify it as a bug in the way we're handling multi-phase initialisation unless/until we determine there's no way to preserve the existing invariant from the single phase case. Cheers, Nick.
On Thu, 14 Dec 2017 17:00:10 +1000 Nick Coghlan <ncoghlan@gmail.com> wrote:
On 14 Dec. 2017 9:19 am, "Antoine Pitrou" <solipsis@pitrou.net> wrote:
Hello,
After debugging a crash on AppVeyor for a submitter's PR (see https://github.com/python/cpython/pull/4611 ), I came to the following diagnosis: converting the "atexit" module (which is a built-in C extension) to PEP 489 multiphase initialization can lead to its m_traverse function (and presumably also m_clear and m_free) to be called while not module state is yet registered: that is, `PyModule_GetState(self)` when called from m_traverse returns NULL!
Is that an expected or known subtlety?
Not that I'm aware of, so I'd be inclined to classify it as a bug in the way we're handling multi-phase initialisation unless/until we determine there's no way to preserve the existing invariant from the single phase case.
Speaking of which, the doc is not very clear: is PEP 489 required for multi-interpreter support or is PyModule_GetState() sufficient? Regards Antoine.
On 12/14/2017 12:00 PM, Antoine Pitrou wrote:
On Thu, 14 Dec 2017 17:00:10 +1000 Nick Coghlan <ncoghlan@gmail.com> wrote:
On 14 Dec. 2017 9:19 am, "Antoine Pitrou" <solipsis@pitrou.net> wrote:
Hello,
After debugging a crash on AppVeyor for a submitter's PR (see https://github.com/python/cpython/pull/4611 ), I came to the following diagnosis: converting the "atexit" module (which is a built-in C extension) to PEP 489 multiphase initialization can lead to its m_traverse function (and presumably also m_clear and m_free) to be called while not module state is yet registered: that is, `PyModule_GetState(self)` when called from m_traverse returns NULL!
Is that an expected or known subtlety?
Thank you for looking into this, Antoine!
Not that I'm aware of, so I'd be inclined to classify it as a bug in the way we're handling multi-phase initialisation unless/until we determine there's no way to preserve the existing invariant from the single phase case.
Yes, it's a bug – at least in documentation. From initial investigation, the problem is that between the two phases of multi-phase init, module state is NULL, and Python code can run. This is expected, so I'm thinking m_traverse for all modules using multi-phase init should have a check for NULL. And this should be documented. Let's have Marcel run with this a bit further.
Speaking of which, the doc is not very clear: is PEP 489 required for multi-interpreter support or is PyModule_GetState() sufficient?
I'm not exactly sure what you're asking; which doc are you referring to? PEP 489 gives you good defaults, if you use it and avoid global state (roughly: C-level mutable static variables), then you should get multi-interpreter support for free in simple cases. It's also possible to use PyModule_GetState() and other APIs directly. However, I'd like to avoid solving subinterpreter support separately (and slightly differently) in each module. For a slightly bigger picture: as a part-time internship, Marcel is identifying where PEP 489 is inadequate, and solving the problems for the complex cases. This is part of better support for subinterpreter support in general. Going with a PEP 489-based solution with atexit would help us in that effort. I'm assuming fixing the atexit bug from 2009 [0] can be delayed a bit as issues with PEP 489 are investigated & solved. Does that sound fair? [0] https://bugs.python.org/issue6531
On Thu, 14 Dec 2017 15:05:02 +0100 Petr Viktorin <encukou@gmail.com> wrote:
PEP 489 gives you good defaults, if you use it and avoid global state (roughly: C-level mutable static variables), then you should get multi-interpreter support for free in simple cases. It's also possible to use PyModule_GetState() and other APIs directly. However, I'd like to avoid solving subinterpreter support separately (and slightly differently) in each module.
My question is: can you get multi-interpreter support *without* PEP 489? That is, using single-phase initialization and PyModule_GetState().
For a slightly bigger picture: as a part-time internship, Marcel is identifying where PEP 489 is inadequate, and solving the problems for the complex cases.
Is Marcel mentored by anyone in particular?
I'm assuming fixing the atexit bug from 2009 [0] can be delayed a bit as issues with PEP 489 are investigated & solved. Does that sound fair?
Probably, but the atexit bug deserves fixing in itself. If a fix is ready, it would be a pity not to let it in. Regards Antoine.
On Thu, Dec 14, 2017 at 12:00 PM, Antoine Pitrou <solipsis@pitrou.net> wrote:
On Thu, 14 Dec 2017 17:00:10 +1000 Nick Coghlan <ncoghlan@gmail.com> wrote:
On 14 Dec. 2017 9:19 am, "Antoine Pitrou" <solipsis@pitrou.net> wrote:
Hello,
After debugging a crash on AppVeyor for a submitter's PR (see https://github.com/python/cpython/pull/4611 ), I came to the following diagnosis: converting the "atexit" module (which is a built-in C extension) to PEP 489 multiphase initialization can lead to its m_traverse function (and presumably also m_clear and m_free) to be called while not module state is yet registered: that is, `PyModule_GetState(self)` when called from m_traverse returns NULL!
Is that an expected or known subtlety?
Not that I'm aware of, so I'd be inclined to classify it as a bug in the way we're handling multi-phase initialisation unless/until we determine there's no way to preserve the existing invariant from the single phase case.
Speaking of which, the doc is not very clear: is PEP 489 required for multi-interpreter support or is PyModule_GetState() sufficient?
Yes, it is possible to have proper subinterpreter support without multi-phase init.
On Tue, 19 Dec 2017 16:10:06 +0100 Petr Viktorin <encukou@gmail.com> wrote:
Speaking of which, the doc is not very clear: is PEP 489 required for multi-interpreter support or is PyModule_GetState() sufficient?
Yes, it is possible to have proper subinterpreter support without multi-phase init.
Thanks. I guess the C API docs need a user-friendly section laying out the various methods for initializing a module, and their various advantages :-) Regards Antoine.
On 12/19/2017 04:19 PM, Antoine Pitrou wrote:
On Tue, 19 Dec 2017 16:10:06 +0100 Petr Viktorin <encukou@gmail.com> wrote:
Speaking of which, the doc is not very clear: is PEP 489 required for multi-interpreter support or is PyModule_GetState() sufficient?
Yes, it is possible to have proper subinterpreter support without multi-phase init.
Thanks. I guess the C API docs need a user-friendly section laying out the various methods for initializing a module, and their various advantages :-)
That, or eventually remove multi-phase init's disadvantages, and have just one way to do it :)
participants (3)
-
Antoine Pitrou -
Nick Coghlan -
Petr Viktorin