-
-
Notifications
You must be signed in to change notification settings - Fork 31.6k
Document that m_traverse for multi-phase initialized modules can be called with m_state=NULL #76555
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
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. 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. |
Marcel, could you look into this? |
By the way, I think you forgot to answer my question on python-dev:
The doc currently isn't very clear about this. |
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. |
I have created a PR at #5140 |
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. |
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. |
That's a good argument, thanks.
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
Thanks! Didn't know about that one, will keep it in mind for next time! |
""" 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. |
Marcel added PR 7145 for that -- it wraps things in try/except to catch Python-level exceptions sounds like a good solution to me. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: