[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]

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 ;-)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[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]

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 good from my side. 
Good idea, Victor.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[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]

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: +miss-islington

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[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]

Change by hai shi :


--
pull_requests: +18432
pull_request: https://github.com/python/cpython/pull/19076

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[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]

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 anyone ;-)

--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[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]

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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[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]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[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]

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 slots are then only needed if the module state actually gets 
> populated, so we can safely skip them if it never even gets allocated

That's also my understanding.

> custom module subclasses should clean up like any other class instance

That sounds like a reasonable compromise to me.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[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]

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 state actually gets 
populated, so we can safely skip them if it never even gets allocated

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[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]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[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]

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[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]

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[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]

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.

I don't see how a module can store data without md_state which would require 
m_clear/m_free to clear/free such data. module_clear() continue to clear Python 
objects of the PyModuleObject structure with PR 18738.

Would you mind to elaborate?

The intent of PR 18738 is to simplify the implementation of C extension modules 
which implements the PEP 489 and uses a module state (md_state > 0).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[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]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[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]

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 = MyModule('example')
>>> m


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.

It would *probably* be legitimate to skip calling the cleanup functions when 
there's no Py_mod_create slot defined, but then the rules for "Do I need to 
account for md_state potentially being NULL or not?" are getting complicated 
enough that the safest option for a module author is to always assume that 
md_state might be NULL and handle that case appropriately.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[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]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[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]

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 md_state is set to NULL. But it never occurs in practice.

module_dealloc() calls PyMem_FREE(m->md_state) but it doesn't set md_state to 
NULL. It's not needed, since the module memory is deallocated anyway.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[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]

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

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com