[issue39824] Multi-phase extension module (PEP 489): don't call m_traverse, m_clear nor m_free before the module state is allocated

STINNER Victor added the comment: > Cython doesn't make complete use of PEP-489 yet, specifically not of the > module state feature (nor module subclasses). This change looks good from my > side. Good idea, Victor. Thanks for the confirmation Stefan ;-) --

[issue39824] Multi-phase extension module (PEP 489): don't call m_traverse, m_clear nor m_free before the module state is allocated

Stefan Behnel added the comment: > I think Cython makes use of PEP-489 so unless I am missing something all > generated extensions use PEP-489 structures. Cython doesn't make complete use of PEP-489 yet, specifically not of the module state feature (nor module subclasses). This change looks

[issue39824] Multi-phase extension module (PEP 489): don't call m_traverse, m_clear nor m_free before the module state is allocated

miss-islington added the comment: New changeset 13397ee47d23fda2e8d4bef40c1df986457673d1 by Hai Shi in branch 'master': bpo-39824: Convert PyModule_GetState() to get_module_state() (GH-19076) https://github.com/python/cpython/commit/13397ee47d23fda2e8d4bef40c1df986457673d1 -- nosy:

[issue39824] Multi-phase extension module (PEP 489): don't call m_traverse, m_clear nor m_free before the module state is allocated

Change by hai shi : -- pull_requests: +18432 pull_request: https://github.com/python/cpython/pull/19076 ___ Python tracker ___ ___

[issue39824] Multi-phase extension module (PEP 489): don't call m_traverse, m_clear nor m_free before the module state is allocated

STINNER Victor added the comment: Thanks Petr and Nick for the review ;-) Pablo Galindo Salgado: > I think Cython makes use of PEP-489 so unless I am missing something all > generated extensions use PEP-489 structures. Alright. I still consider that my change is correct and will no harm

[issue39824] Multi-phase extension module (PEP 489): don't call m_traverse, m_clear nor m_free before the module state is allocated

STINNER Victor added the comment: New changeset 5b1ef200d31a74a9b478d0217d73ed0a659a8a06 by Victor Stinner in branch 'master': bpo-39824: module_traverse() don't call m_traverse if md_state=NULL (GH-18738) https://github.com/python/cpython/commit/5b1ef200d31a74a9b478d0217d73ed0a659a8a06

[issue39824] Multi-phase extension module (PEP 489): don't call m_traverse, m_clear nor m_free before the module state is allocated

Pablo Galindo Salgado added the comment: I think Cython makes use of PEP-489 so unless I am missing something all generated extensions use PEP-489 structures. -- ___ Python tracker

[issue39824] Multi-phase extension module (PEP 489): don't call m_traverse, m_clear nor m_free before the module state is allocated

STINNER Victor added the comment: I updated PR 18738 to document the incompatible change in What's New In Python 3.9. Sadly, I expect that almost no third-party extension module implement the PEP 489 yet. So I expect that little or no third-party code is impacted in pratice. > the module

[issue39824] Multi-phase extension module (PEP 489): don't call m_traverse, m_clear nor m_free before the module state is allocated

Nick Coghlan added the comment: Petr's point that any subclass state should be managed in the subclass cleanup functions is a good one, so I withdraw my concern: * custom module subclasses should clean up like any other class instance * the module slots are then only needed if the module

[issue39824] Multi-phase extension module (PEP 489): don't call m_traverse, m_clear nor m_free before the module state is allocated

STINNER Victor added the comment: Stefan Behnel: as the 3rd author of the PEP 489, what's your call on this issue? -- nosy: +scoder ___ Python tracker ___

[issue39824] Multi-phase extension module (PEP 489): don't call m_traverse, m_clear nor m_free before the module state is allocated

STINNER Victor added the comment: > Proposed PR checking if the module state is NULL: > > * PR 18358: _locale module > * PR 18608: audioop module > * PR 18613: binascii Petr suggested to not hold these PRs with this controversial issue. I agree, so I merged these 3 PRs. --

[issue39824] Multi-phase extension module (PEP 489): don't call m_traverse, m_clear nor m_free before the module state is allocated

Petr Viktorin added the comment: If you use a module subclass that needs some additional C-level infrastructure, it would be more appropriate to override tp_clear/tp_free directly. IMO limiting m_clear/m_free to work just with the module state won't hurt. But it is an API change.

[issue39824] Multi-phase extension module (PEP 489): don't call m_traverse, m_clear nor m_free before the module state is allocated

STINNER Victor added the comment: > So it isn't valid to skip calling the cleanup functions just because md_state > is NULL - we have no idea what Py_mod_create might have done that needs to be > cleaned up. In your example, I don't see what m_clear/m_free would be supposed to clear/free.

[issue39824] Multi-phase extension module (PEP 489): don't call m_traverse, m_clear nor m_free before the module state is allocated

hai shi added the comment: > we have no idea what Py_mod_create might have done that needs to be cleaned > up. Looks like no extension module author use `Py_mod_create` slots now. -- ___ Python tracker

[issue39824] Multi-phase extension module (PEP 489): don't call m_traverse, m_clear nor m_free before the module state is allocated

Nick Coghlan added the comment: One of the intended use cases for Py_mod_create is to return instances of ModuleType subclasses rather than straight ModuleType instances. And those are definitely legal to define: >>> import __main__ >>> class MyModule(type(__main__)): pass ... >>> m =

[issue39824] Multi-phase extension module (PEP 489): don't call m_traverse, m_clear nor m_free before the module state is allocated

STINNER Victor added the comment: Proposed PR checking if the module state is NULL: * PR 18358: _locale module * PR 18608: audioop module * PR 18613: binascii -- ___ Python tracker

[issue39824] Multi-phase extension module (PEP 489): don't call m_traverse, m_clear nor m_free before the module state is allocated

STINNER Victor added the comment: > I propose to change module_traverse(), module_clear() and module_dealloc() to > not call m_traverse(), m_clear() and m_free() if md_state is NULL and m_size > > 0. Note: This change also means that m_traverse, m_clear and m_free are no longer called if

[issue39824] Multi-phase extension module (PEP 489): don't call m_traverse, m_clear nor m_free before the module state is allocated

Change by STINNER Victor : -- title: Multi-phase extension module (PEP 489): don't call m_traverse, m_clear nor m_free if md_state is NULL -> Multi-phase extension module (PEP 489): don't call m_traverse, m_clear nor m_free before the module state is allocated