From aee219f4558dda619bd86e4b0e028ce47a5e4b77 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Fri, 20 Sep 2024 10:27:34 +0200 Subject: [PATCH] gh-123880: Allow recursive import of single-phase-init modules (GH-123950) Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com> Co-authored-by: Brett Cannon --- Lib/test/test_import/__init__.py | 69 ++++++++++++++----- .../data/circular_imports/singlephase.py | 13 ++++ ...-09-12-16-16-24.gh-issue-123880.2-8vcj.rst | 2 + Modules/_testsinglephase.c | 63 ++++++++++++++++- Python/import.c | 18 +++-- Tools/c-analyzer/cpython/ignored.tsv | 1 + 6 files changed, 141 insertions(+), 25 deletions(-) create mode 100644 Lib/test/test_import/data/circular_imports/singlephase.py create mode 100644 Misc/NEWS.d/next/C_API/2024-09-12-16-16-24.gh-issue-123880.2-8vcj.rst diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py index 3d89d69955b..5d0d02480b3 100644 --- a/Lib/test/test_import/__init__.py +++ b/Lib/test/test_import/__init__.py @@ -111,6 +111,24 @@ def require_frozen(module, *, skip=True): def require_pure_python(module, *, skip=False): _require_loader(module, SourceFileLoader, skip) +def create_extension_loader(modname, filename): + # Apple extensions must be distributed as frameworks. This requires + # a specialist loader. + if is_apple_mobile: + return AppleFrameworkLoader(modname, filename) + else: + return ExtensionFileLoader(modname, filename) + +def import_extension_from_file(modname, filename, *, put_in_sys_modules=True): + loader = create_extension_loader(modname, filename) + spec = importlib.util.spec_from_loader(modname, loader) + module = importlib.util.module_from_spec(spec) + loader.exec_module(module) + if put_in_sys_modules: + sys.modules[modname] = module + return module + + def remove_files(name): for f in (name + ".py", name + ".pyc", @@ -1894,6 +1912,37 @@ class CircularImportTests(unittest.TestCase): str(cm.exception), ) + @requires_singlephase_init + @unittest.skipIf(_testsinglephase is None, "test requires _testsinglephase module") + def test_singlephase_circular(self): + """Regression test for gh-123950 + + Import a single-phase-init module that imports itself + from the PyInit_* function (before it's added to sys.modules). + Manages its own cache (which is `static`, and so incompatible + with multiple interpreters or interpreter reset). + """ + name = '_testsinglephase_circular' + helper_name = 'test.test_import.data.circular_imports.singlephase' + with uncache(name, helper_name): + filename = _testsinglephase.__file__ + # We don't put the module in sys.modules: that the *inner* + # import should do that. + mod = import_extension_from_file(name, filename, + put_in_sys_modules=False) + + self.assertEqual(mod.helper_mod_name, helper_name) + self.assertIn(name, sys.modules) + self.assertIn(helper_name, sys.modules) + + self.assertIn(name, sys.modules) + self.assertIn(helper_name, sys.modules) + self.assertNotIn(name, sys.modules) + self.assertNotIn(helper_name, sys.modules) + self.assertIs(mod.clear_static_var(), mod) + _testinternalcapi.clear_extension('_testsinglephase_circular', + mod.__spec__.origin) + def test_unwritable_module(self): self.addCleanup(unload, "test.test_import.data.unwritable") self.addCleanup(unload, "test.test_import.data.unwritable.x") @@ -1933,14 +1982,6 @@ class SubinterpImportTests(unittest.TestCase): os.set_blocking(r, False) return (r, w) - def create_extension_loader(self, modname, filename): - # Apple extensions must be distributed as frameworks. This requires - # a specialist loader. - if is_apple_mobile: - return AppleFrameworkLoader(modname, filename) - else: - return ExtensionFileLoader(modname, filename) - def import_script(self, name, fd, filename=None, check_override=None): override_text = '' if check_override is not None: @@ -2157,11 +2198,7 @@ class SubinterpImportTests(unittest.TestCase): def test_multi_init_extension_non_isolated_compat(self): modname = '_test_non_isolated' filename = _testmultiphase.__file__ - loader = self.create_extension_loader(modname, filename) - spec = importlib.util.spec_from_loader(modname, loader) - module = importlib.util.module_from_spec(spec) - loader.exec_module(module) - sys.modules[modname] = module + module = import_extension_from_file(modname, filename) require_extension(module) with self.subTest(f'{modname}: isolated'): @@ -2176,11 +2213,7 @@ class SubinterpImportTests(unittest.TestCase): def test_multi_init_extension_per_interpreter_gil_compat(self): modname = '_test_shared_gil_only' filename = _testmultiphase.__file__ - loader = self.create_extension_loader(modname, filename) - spec = importlib.util.spec_from_loader(modname, loader) - module = importlib.util.module_from_spec(spec) - loader.exec_module(module) - sys.modules[modname] = module + module = import_extension_from_file(modname, filename) require_extension(module) with self.subTest(f'{modname}: isolated, strict'): diff --git a/Lib/test/test_import/data/circular_imports/singlephase.py b/Lib/test/test_import/data/circular_imports/singlephase.py new file mode 100644 index 00000000000..05618bc72f9 --- /dev/null +++ b/Lib/test/test_import/data/circular_imports/singlephase.py @@ -0,0 +1,13 @@ +"""Circular import involving a single-phase-init extension. + +This module is imported from the _testsinglephase_circular module from +_testsinglephase, and imports that module again. +""" + +import importlib +import _testsinglephase +from test.test_import import import_extension_from_file + +name = '_testsinglephase_circular' +filename = _testsinglephase.__file__ +mod = import_extension_from_file(name, filename) diff --git a/Misc/NEWS.d/next/C_API/2024-09-12-16-16-24.gh-issue-123880.2-8vcj.rst b/Misc/NEWS.d/next/C_API/2024-09-12-16-16-24.gh-issue-123880.2-8vcj.rst new file mode 100644 index 00000000000..8a31c962ec7 --- /dev/null +++ b/Misc/NEWS.d/next/C_API/2024-09-12-16-16-24.gh-issue-123880.2-8vcj.rst @@ -0,0 +1,2 @@ +Fixed a bug that prevented circular imports of extension modules that use +single-phase initialization. diff --git a/Modules/_testsinglephase.c b/Modules/_testsinglephase.c index 066e0dbfb63..2c59085d15b 100644 --- a/Modules/_testsinglephase.c +++ b/Modules/_testsinglephase.c @@ -1,7 +1,7 @@ /* Testing module for single-phase initialization of extension modules -This file contains 8 distinct modules, meaning each as its own name +This file contains several distinct modules, meaning each as its own name and its own init function (PyInit_...). The default import system will only find the one matching the filename: _testsinglephase. To load the others you must do so manually. For example: @@ -12,9 +12,13 @@ filename = _testsinglephase.__file__ loader = importlib.machinery.ExtensionFileLoader(name, filename) spec = importlib.util.spec_from_file_location(name, filename, loader=loader) mod = importlib._bootstrap._load(spec) +loader.exec_module(module) +sys.modules[modname] = module ``` -Here are the 8 modules: +(The last two lines are just for completeness.) + +Here are the modules: * _testsinglephase * def: _testsinglephase_basic, @@ -163,6 +167,11 @@ Here are the 8 modules: * functions: none * import system: same as _testsinglephase_with_state +* _testsinglephase_circular + Regression test for gh-123880. + Does not have the common attributes & methods. + See test_singlephase_circular test.test_import.SinglephaseInitTests. + Module state: * fields @@ -740,3 +749,53 @@ PyInit__testsinglephase_with_state_check_cache_first(void) } return PyModule_Create(&_testsinglephase_with_state_check_cache_first); } + + +/****************************************/ +/* the _testsinglephase_circular module */ +/****************************************/ + +static PyObject *static_module_circular; + +static PyObject * +circularmod_clear_static_var(PyObject *self, PyObject *arg) +{ + PyObject *result = static_module_circular; + static_module_circular = NULL; + return result; +} + +static struct PyModuleDef _testsinglephase_circular = { + PyModuleDef_HEAD_INIT, + .m_name = "_testsinglephase_circular", + .m_doc = PyDoc_STR("Test module _testsinglephase_circular"), + .m_methods = (PyMethodDef[]) { + {"clear_static_var", circularmod_clear_static_var, METH_NOARGS, + "Clear the static variable and return its previous value."}, + {NULL, NULL} /* sentinel */ + } +}; + +PyMODINIT_FUNC +PyInit__testsinglephase_circular(void) +{ + if (!static_module_circular) { + static_module_circular = PyModule_Create(&_testsinglephase_circular); + if (!static_module_circular) { + return NULL; + } + } + static const char helper_mod_name[] = ( + "test.test_import.data.circular_imports.singlephase"); + PyObject *helper_mod = PyImport_ImportModule(helper_mod_name); + Py_XDECREF(helper_mod); + if (!helper_mod) { + return NULL; + } + if(PyModule_AddStringConstant(static_module_circular, + "helper_mod_name", + helper_mod_name) < 0) { + return NULL; + } + return Py_NewRef(static_module_circular); +} diff --git a/Python/import.c b/Python/import.c index 26ad84372ce..460b1fe225c 100644 --- a/Python/import.c +++ b/Python/import.c @@ -815,6 +815,8 @@ static int clear_singlephase_extension(PyInterpreterState *interp, // Currently, this is only used for testing. // (See _testinternalcapi.clear_extension().) +// If adding another use, be careful about modules that import themselves +// recursively (see gh-123880). int _PyImport_ClearExtension(PyObject *name, PyObject *filename) { @@ -1322,12 +1324,16 @@ _extensions_cache_set(PyObject *path, PyObject *name, value = entry == NULL ? NULL : (struct extensions_cache_value *)entry->value; - /* We should never be updating an existing cache value. */ - assert(value == NULL); if (value != NULL) { - PyErr_Format(PyExc_SystemError, - "extension module %R is already cached", name); - goto finally; + /* gh-123880: If there's an existing cache value, it means a module is + * being imported recursively from its PyInit_* or Py_mod_* function. + * (That function presumably handles returning a partially + * constructed module in such a case.) + * We can reuse the existing cache value; it is owned by the cache. + * (Entries get removed from it in exceptional circumstances, + * after interpreter shutdown, and in runtime shutdown.) + */ + goto finally_oldvalue; } newvalue = alloc_extensions_cache_value(); if (newvalue == NULL) { @@ -1392,6 +1398,7 @@ finally: cleanup_old_cached_def(&olddefbase); } +finally_oldvalue: extensions_lock_release(); if (key != NULL) { hashtable_destroy_str(key); @@ -2128,6 +2135,7 @@ error: } +// Used in _PyImport_ClearExtension; see notes there. static int clear_singlephase_extension(PyInterpreterState *interp, PyObject *name, PyObject *path) diff --git a/Tools/c-analyzer/cpython/ignored.tsv b/Tools/c-analyzer/cpython/ignored.tsv index fabb5de921a..b002ada1a8d 100644 --- a/Tools/c-analyzer/cpython/ignored.tsv +++ b/Tools/c-analyzer/cpython/ignored.tsv @@ -590,6 +590,7 @@ Modules/_testmultiphase.c - slots_nonmodule_with_exec_slots - Modules/_testmultiphase.c - testexport_methods - Modules/_testmultiphase.c - uninitialized_def - Modules/_testsinglephase.c - global_state - +Modules/_testsinglephase.c - static_module_circular - Modules/_xxtestfuzz/_xxtestfuzz.c - _fuzzmodule - Modules/_xxtestfuzz/_xxtestfuzz.c - module_methods - Modules/_xxtestfuzz/fuzzer.c - RE_FLAG_DEBUG -