[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[miss-islington]

miss-islington  added the comment:


New changeset 6ec325d48348fb52a421354ba563ff9c1f368054 by Miss Islington (bot) 
in branch '3.6':
bpo-32374: Ignore Python-level exceptions in test_bad_traverse (GH-7145)
https://github.com/python/cpython/commit/6ec325d48348fb52a421354ba563ff9c1f368054


--

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[miss-islington]

Change by miss-islington :


--
pull_requests: +6791

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[Petr Viktorin]

Petr Viktorin  added the comment:


New changeset 983c1ba94aef945386001932c5744f8ce9757fec by Petr Viktorin (Miss 
Islington (bot)) in branch '3.7':
bpo-32374: Ignore Python-level exceptions in test_bad_traverse (GH-7145) 
(GH-7150)
https://github.com/python/cpython/commit/983c1ba94aef945386001932c5744f8ce9757fec


--

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[miss-islington]

Change by miss-islington :


--
pull_requests: +6784

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[Petr Viktorin]

Petr Viktorin  added the comment:


New changeset 08c5aca9d13b24b35faf89ebd26fc348ae1731b2 by Petr Viktorin (Marcel 
Plch) in branch 'master':
bpo-32374: Ignore Python-level exceptions in test_bad_traverse (GH-7145)
https://github.com/python/cpython/commit/08c5aca9d13b24b35faf89ebd26fc348ae1731b2


--

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[STINNER Victor]

Change by STINNER Victor :


--
pull_requests: +6781

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[Petr Viktorin]

Petr Viktorin  added the comment:

> That's a weak test: if the script fails before calling 
> spec.loader.create_module(), the test also pass. If the function raises an 
> exception but don't crash, the test pass as well.

Marcel added PR 7145 for that -- it wraps things in try/except to catch 
Python-level exceptions sounds like a good solution to me.

--

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[Marcel Plch]

Change by Marcel Plch :


--
pull_requests: +6779

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[STINNER Victor]

STINNER Victor  added the comment:

"""
This particular kind of bad traverse is quite easy to write if an extension 
author doesn't read docs carefully, or has read an old version of them (before 
the fix here). And resulting errors aren't too obvious. So, there's code to 
crash early and loudly in "--with-pydebug" for simple oversight cases. See 
comment at the call site of bad_traverse_test in Objects/moduleobject.c
And since there's code, there's a test to make sure it works :)
"""

Oh ok, it makes sense. Maybe the test should test at least just before 
spec.loader.create_module(). Maybe using a print().

"Thanks! Didn't know about that one, will keep it in mind for next time!"

The problem is that by default, on Linux, we don't dump core files on the 
current directory. So such bug is silent on Linux. But it's commonly detected 
on FreeBSD since I configured the test runner to fail if it leaves a new file.

--

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[Petr Viktorin]

Petr Viktorin  added the comment:

> That's a weak test: if the script fails before calling 
> spec.loader.create_module(), the test also pass. If the function raises an 
> exception but don't crash, the test pass as well.

That's a good argument, thanks.
Marcel, would you mind adding a patch for that?

> More generally, I'm not sure about the idea of making sure that doing bad 
> stuff with traverse does crash. What is the purpose of the test?

This particular kind of bad traverse is quite easy to write if an extension 
author doesn't read docs carefully, or has read an old version of them (before 
the fix here). And resulting errors aren't too obvious. So, there's code to 
crash early and loudly in "--with-pydebug" for simple oversight cases. See 
comment at the call site of bad_traverse_test in Objects/moduleobject.c
And since there's code, there's a test to make sure it works :)

> In the meanwhile, I fixed bpo-33629 by adding 
> test.support.SuppressCrashReport().

Thanks! Didn't know about that one, will keep it in mind for next time!

--

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[miss-islington]

miss-islington  added the comment:


New changeset fc0356d2a34719df517a5056bf1a3709850776cf by Miss Islington (bot) 
in branch '3.6':
bpo-33629: Prevent coredump in test_importlib (GH-7090)
https://github.com/python/cpython/commit/fc0356d2a34719df517a5056bf1a3709850776cf


--

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[miss-islington]

miss-islington  added the comment:


New changeset d9eb22c67c38b45764dd924801c72092770d200f by Miss Islington (bot) 
in branch '3.7':
bpo-33629: Prevent coredump in test_importlib (GH-7090)
https://github.com/python/cpython/commit/d9eb22c67c38b45764dd924801c72092770d200f


--

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[STINNER Victor]

STINNER Victor  added the comment:

MultiPhaseExtensionModuleTests.test_bad_traverse() of 
Lib/test/test_importlib/extension/test_loader.py runs the following code:
---
import importlib.util as util
spec = util.find_spec('_testmultiphase')
spec.name = '_testmultiphase_with_bad_traverse'
m = spec.loader.create_module(spec)
---

And then check that the Python "failed": that the exit code is non-zero...

That's a weak test: if the script fails before calling 
spec.loader.create_module(), the test also pass. If the function raises an 
exception but don't crash, the test pass as well.

More generally, I'm not sure about the idea of making sure that doing bad stuff 
with traverse does crash. What is the purpose of the test?

In the meanwhile, I fixed bpo-33629 by adding 
test.support.SuppressCrashReport().

I'm not asking to do something. Maybe it's fine to keep the current test.

--

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[miss-islington]

Change by miss-islington :


--
pull_requests: +6741

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[miss-islington]

Change by miss-islington :


--
pull_requests: +6739

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[STINNER Victor]

STINNER Victor  added the comment:


New changeset 483000e164ec68717d335767b223ae31b4b720cf by Victor Stinner in 
branch 'master':
bpo-33629: Prevent coredump in test_importlib (GH-7090)
https://github.com/python/cpython/commit/483000e164ec68717d335767b223ae31b4b720cf


--

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[STINNER Victor]

STINNER Victor  added the comment:

MultiPhaseExtensionModuleTests.test_bad_traverse() of 
Lib/test/test_importlib/extension/test_loader.py does create a coredump file. 
The script run by the test trigger a segfault. Is it deliberate? See bpo-33629 
and my PR 7090.

--
nosy: +vstinner

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[STINNER Victor]

Change by STINNER Victor :


--
pull_requests: +6726

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[Petr Viktorin]

Change by Petr Viktorin :


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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[miss-islington]

miss-islington  added the comment:


New changeset 1da0479f687613a43620430616f4db87e2ed4423 by Miss Islington (bot) 
in branch '3.6':
bpo-32374:  m_traverse may be called with m_state=NULL (GH-5140)
https://github.com/python/cpython/commit/1da0479f687613a43620430616f4db87e2ed4423


--

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[miss-islington]

miss-islington  added the comment:


New changeset 136905fffd5f77395f80e3409630c11756b5469c by Miss Islington (bot) 
in branch '3.7':
bpo-32374:  m_traverse may be called with m_state=NULL (GH-5140)
https://github.com/python/cpython/commit/136905fffd5f77395f80e3409630c11756b5469c


--
nosy: +miss-islington

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[miss-islington]

Change by miss-islington :


--
pull_requests: +5890

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[miss-islington]

Change by miss-islington :


--
pull_requests: +5891

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[Nick Coghlan]

Nick Coghlan  added the comment:


New changeset c2b0b12d1a137ada1023ab7c10b8d9a0249d95f9 by Nick Coghlan (Marcel 
Plch) in branch 'master':
bpo-32374:  m_traverse may be called with m_state=NULL (GH-5140)
https://github.com/python/cpython/commit/c2b0b12d1a137ada1023ab7c10b8d9a0249d95f9


--

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[Marcel Plch]

Marcel Plch  added the comment:

I have created a PR at https://github.com/python/cpython/pull/5140
It contains documentation, plus, in --with-pydebug mode, it calls m_traverse to 
catch easy cases of not handling the m_state==NULL case early.

--

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[Marcel Plch]

Change by Marcel Plch :


--
pull_requests: +4999

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[Nick Coghlan]

Nick Coghlan  added the comment:

Yep, the requirement for supporting multiple interpreters is just that you 
either avoid relying on C global state entirely, or else correctly synchronise 
access to it. Multi-phase initialisation just provides a few nudges in the 
direction of doing that more consistently.

--

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[Antoine Pitrou]

Change by Antoine Pitrou :


--
versions: +Python 3.6

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[Antoine Pitrou]

Change by Antoine Pitrou :


--
assignee:  -> docs@python
components: +Documentation
nosy: +docs@python
type:  -> behavior

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[Antoine Pitrou]

Antoine Pitrou  added the comment:

By the way, I think you forgot to answer my question on python-dev:

> can you get multi-interpreter support *without* PEP 489?  That is, using 
> single-phase initialization and PyModule_GetState().

The doc currently isn't very clear about this.

--
nosy: +pitrou

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[Antoine Pitrou]

Change by Antoine Pitrou :


--
nosy: +scoder

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[Antoine Pitrou]

Change by Antoine Pitrou :


--
nosy: +ncoghlan

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[Marcel Plch]

Change by Marcel Plch :


--
keywords: +patch
pull_requests: +4821
stage:  -> patch review

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[Petr Viktorin]

Petr Viktorin  added the comment:

Marcel, could you look into this?

--
nosy: +Dormouse759

___
Python tracker 

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

[issue32374] Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL

[Petr Viktorin]

New submission from Petr Viktorin :

After the create phase of multiphase initialization, the per-module state is 
NULL and the module object is reachable by the garbage collector. Between the 
create and exec phases, Python code is run and garbage collection can be 
triggered.
So, any custom m_traverse/m_clear/m_free function must be prepared to handle 
m_state being NULL. This is currently not well documented.

It might be useful to insert a call m_traverse after the first phase, at least 
in --with-pydebug mode, so the potential error gets triggered early.

--
components: Extension Modules
messages: 308647
nosy: encukou
priority: normal
severity: normal
status: open
title: Document that m_traverse for multi-phase initialized modules can be 
called with m_state=NULL
versions: Python 3.7

___
Python tracker 

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