classification
Title: Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL
Type: behavior Stage: resolved
Components: Documentation, Extension Modules Versions: Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: docs@python Nosy List: Dormouse759, docs@python, miss-islington, ncoghlan, petr.viktorin, pitrou, scoder, vstinner
Priority: normal Keywords: patch

Created on 2017-12-19 14:40 by petr.viktorin, last changed 2018-05-28 16:37 by miss-islington. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 4611 Dormouse759, 2017-12-19 14:42
PR 5140 merged Dormouse759, 2018-01-09 15:59
PR 6128 merged miss-islington, 2018-03-17 05:41
PR 6129 merged miss-islington, 2018-03-17 05:42
PR 7090 merged vstinner, 2018-05-24 02:13
PR 7101 merged miss-islington, 2018-05-24 20:20
PR 7102 merged miss-islington, 2018-05-24 20:21
PR 7145 merged Dormouse759, 2018-05-28 09:06
PR 7147 closed vstinner, 2018-05-28 10:53
PR 7150 merged miss-islington, 2018-05-28 12:11
PR 7155 merged miss-islington, 2018-05-28 15:32
Messages (19)
msg308647 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2017-12-19 14:40
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.
msg308648 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2017-12-19 14:41
Marcel, could you look into this?
msg308650 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-12-19 14:46
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.
msg309052 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-12-26 00:26
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.
msg309709 - (view) Author: Marcel Plch (Dormouse759) * Date: 2018-01-09 16:11
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.
msg313987 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-03-17 05:41
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
msg313988 - (view) Author: miss-islington (miss-islington) Date: 2018-03-17 06:03
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
msg313989 - (view) Author: miss-islington (miss-islington) Date: 2018-03-17 06:29
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
msg317506 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-05-24 02:15
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.
msg317615 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-05-24 20:19
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
msg317616 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-05-24 20:24
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.
msg317621 - (view) Author: miss-islington (miss-islington) Date: 2018-05-24 20:45
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
msg317625 - (view) Author: miss-islington (miss-islington) Date: 2018-05-24 21:07
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
msg317660 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2018-05-25 08:02
> 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!
msg317684 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-05-25 15:48
"""
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.
msg317834 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2018-05-28 09:16
> 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.
msg317840 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2018-05-28 12:11
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
msg317844 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2018-05-28 12:52
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
msg317885 - (view) Author: miss-islington (miss-islington) Date: 2018-05-28 16:37
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
History
Date User Action Args
2018-05-28 16:37:14miss-islingtonsetmessages: + msg317885
2018-05-28 15:32:17miss-islingtonsetpull_requests: + pull_request6791
2018-05-28 12:52:09petr.viktorinsetmessages: + msg317844
2018-05-28 12:11:37miss-islingtonsetpull_requests: + pull_request6784
2018-05-28 12:11:23petr.viktorinsetmessages: + msg317840
2018-05-28 10:53:25vstinnersetpull_requests: + pull_request6781
2018-05-28 09:16:45petr.viktorinsetmessages: + msg317834
2018-05-28 09:06:09Dormouse759setpull_requests: + pull_request6779
2018-05-25 15:48:02vstinnersetmessages: + msg317684
2018-05-25 08:02:54petr.viktorinsetmessages: + msg317660
2018-05-24 21:07:46miss-islingtonsetmessages: + msg317625
2018-05-24 20:45:00miss-islingtonsetmessages: + msg317621
2018-05-24 20:24:02vstinnersetmessages: + msg317616
2018-05-24 20:21:52miss-islingtonsetpull_requests: + pull_request6741
2018-05-24 20:20:56miss-islingtonsetpull_requests: + pull_request6739
2018-05-24 20:19:37vstinnersetmessages: + msg317615
2018-05-24 02:15:59vstinnersetnosy: + vstinner
messages: + msg317506
2018-05-24 02:13:45vstinnersetpull_requests: + pull_request6726
2018-05-15 21:20:58petr.viktorinsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2018-03-17 06:29:32miss-islingtonsetmessages: + msg313989
2018-03-17 06:03:58miss-islingtonsetnosy: + miss-islington
messages: + msg313988
2018-03-17 05:42:30miss-islingtonsetpull_requests: + pull_request5891
2018-03-17 05:41:48miss-islingtonsetpull_requests: + pull_request5890
2018-03-17 05:41:29ncoghlansetmessages: + msg313987
2018-01-09 16:11:34Dormouse759setmessages: + msg309709
2018-01-09 15:59:35Dormouse759setpull_requests: + pull_request4999
2017-12-26 00:26:21ncoghlansetmessages: + msg309052
2017-12-19 14:47:42pitrousetassignee: docs@python

type: behavior
components: + Documentation
nosy: + docs@python
2017-12-19 14:47:33pitrousetversions: + Python 3.6
2017-12-19 14:46:32pitrousetnosy: + pitrou
messages: + msg308650
2017-12-19 14:45:28pitrousetnosy: + scoder
2017-12-19 14:45:19pitrousetnosy: + ncoghlan
2017-12-19 14:42:42Dormouse759setkeywords: + patch
stage: patch review
pull_requests: + pull_request4821
2017-12-19 14:41:21petr.viktorinsetnosy: + Dormouse759
messages: + msg308648
2017-12-19 14:40:38petr.viktorincreate