8000 gh-123880: Allow recursive import of single-phase-init modules (GH-12… · python/cpython@aee219f · GitHub
[go: up one dir, main page]
Skip to content

Commit aee219f

Browse files
encukouhauntsaninjabrettcannon
authored
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 <brett@python.org>
1 parent 3e36e5a commit aee219f

File tree

6 files changed

+141
-25
lines changed

6 files changed

+141
-25
lines changed

‎Lib/test/test_import/__init__.py

Lines changed: 51 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,24 @@ def require_frozen(module, *, skip=True):
111111
def require_pure_python(module, *, skip=False):
112112
_require_loader(module, SourceFileLoader, skip)
113113

114+
def create_extension_loader(modname, filename):
115+
# Apple extensions must be distributed as frameworks. This requires
116+
# a specialist loader.
117+
if is_apple_mobile:
118+
return AppleFrameworkLoader(modname, filename)
119+
else:
120+
return ExtensionFileLoader(modname, filename)
121+
122+
def import_extension_from_file(modname, filename, *, put_in_sys_modules=True):
123+
loader = create_extension_loader(modname, filename)
124+
spec = importlib.util.spec_from_loader(modname, loader)
125+
module = importlib.util.module_from_spec(spec)
126+
loader.exec_module(module)
127+
if put_in_sys_modules:
128+
sys.modules[modname] = module
129+
return module
130+
131+
114132
def remove_files(name):
115133
for f in (name + ".py",
116134
name + ".pyc",
@@ -1894,6 +1912,37 @@ def test_absolute_circular_submodule(self):
18941912
str(cm.exception),
18951913
)
18961914

1915+
@requires_singlephase_init
1916+
@unittest.skipIf(_testsinglephase is None, "test requires _testsinglephase module")
1917+
def test_singlephase_circular(self):
1918+
"""Regression test for gh-123950
1919+
1920+
Import a single-phase-init module that imports itself
1921+
from the PyInit_* function (before it's added to sys.modules).
1922+
Manages its own cache (which is `static`, and so incompatible
1923+
with multiple interpreters or interpreter reset).
1924+
"""
1925+
name = '_testsinglephase_circular'
1926+
helper_name = 'test.test_import.data.circular_imports.singlephase'
1927+
with uncache(name, helper_name):
1928+
filename = _testsinglephase.__file__
1929+
# We don't put the module in sys.modules: that the *inner*
1930+
# import should do that.
1931+
mod = import_extension_from_file(name, filename,
1932+
put_in_sys_modules=False)
1933+
1934+
self.assertEqual(mod.helper_mod_name, helper_name)
1935+
self.assertIn(name, sys.modules)
1936+
self.assertIn(helper_name, sys.modules)
1937+
1938+
self.assertIn(name, sys.modules)
1939+
self.assertIn(helper_name, sys.modules)
1940+
self.assertNotIn(name, sys.modules)
1941+
self.assertNotIn(helper_name, sys.modules)
1942+
self.assertIs(mod.clear_static_var(), mod)
1943+
_testinternalcapi.clear_extension('_testsinglephase_circular',
1944+
mod.__spec__.origin)
1945+
18971946
def test_unwritable_module(self):
18981947
self.addCleanup(unload, "test.test_import.data.unwritable")
18991948
self.addCleanup(unload, "test.test_import.data.unwritable.x")
@@ -1933,14 +1982,6 @@ def pipe(self):
19331982
os.set_blocking(r, False)
19341983
return (r, w)
19351984

1936-
def create_extension_loader(self, modname, filename):
1937-
# Apple extensions must be distributed as frameworks. This requires
1938-
# a specialist loader.
1939-
if is_apple_mobile:
1940-
return AppleFrameworkLoader(modname, filename)
1941-
else:
1942-
return ExtensionFileLoader(modname, filename)
1943-
19441985
def import_script(self, name, fd, filename=None, check_override=None):
19451986
override_text = ''
19461987
if check_override is not None:
@@ -2157,11 +2198,7 @@ def test_multi_init_extension_compat(self):
21572198
def test_multi_init_extension_non_isolated_compat(self):
21582199
modname = '_test_non_isolated'
21592200
filename = _testmultiphase.__file__
2160-
loader = self.create_extension_loader(modname, filename)
2161-
spec = importlib.util.spec_from_loader(modname, loader)
2162-
module = importlib.util.module_from_spec(spec)
2163-
loader.exec_module(module)
2164-
sys.modules[modname] = module
2201+
module = import_extension_from_file(modname, filename)
21652202

21662203
require_extension(module)
21672204
with self.subTest(f'{modname}: isolated'):
@@ -2176,11 +2213,7 @@ def test_multi_init_extension_non_isolated_compat(self):
21762213
def test_multi_init_extension_per_interpreter_gil_compat(self):
21772214
modname = '_test_shared_gil_only'
21782215
filename = _testmultiphase.__file__
2179-
loader = self.create_extension_loader(modname, filename)
2180-
spec = importlib.util.spec_from_loader(modname, loader)
2181-
module = importlib.util.module_from_spec(spec)
2182-
loader.exec_module(module)
2183-
sys.modules[modname] = module
2216+
module = import_extension_from_file(modname, filename)
21842217

21852218
require_extension(module)
21862219
with self.subTest(f'{modname}: isolated, strict'):

‎Lib/test/test_import/data/circular_imports/singlephase.py

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
"""Circular import involving a single-phase-init extension.
2+
3+
This module is imported from the _testsinglephase_circular module from
4+
_testsinglephase, and imports that module again.
5+
"""
6+
7+
import importlib
8+
import _testsinglephase
9+
from test.test_import import import_extension_from_file
10+
11+
name = '_testsinglephase_circular'
12+
filename = _testsinglephase.__file__
13+
mod = import_extension_from_file(name, filename)

‎Misc/NEWS.d/next/C_API/2024-09-12-16-16-24.gh-issue-123880.2-8vcj.rst

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
Fixed a bug that prevented circular imports of extension modules that use
2+
single-phase initialization.

‎Modules/_testsinglephase.c

Lines changed: 61 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11

22
/* Testing module for single-phase initialization of extension modules
33
4-
This file contains 8 distinct modules, meaning each as its own name
4+
This file contains several distinct modules, meaning each as its own name
55
and its own init function (PyInit_...). The default import system will
66
only find the one matching the filename: _testsinglephase. To load the
77
others you must do so manually. For example:
@@ -12,9 +12,13 @@ filename = _testsinglephase.__file__
1212
loader = importlib.machinery.ExtensionFileLoader(name, filename)
1313
spec = importlib.util.spec_from_file_location(name, filename, loader=loader)
1414
mod = importlib._bootstrap._load(spec)
15+
loader.exec_module(module)
16+
sys.modules[modname] = module
1517
```
1618
17-
Here are the 8 modules:
19+
(The last two lines are just for completeness.)
20+
21+
Here are the modules:
1822
1923
* _testsinglephase
2024
* def: _testsinglephase_basic,
@@ -163,6 +167,11 @@ Here are the 8 modules:
163167
* functions: none
164168
* import system: same as _testsinglephase_with_state
165169
170+
* _testsinglephase_circular
171+
Regression test for gh-123880.
172+
Does not have the common attributes & methods.
173+
See test_singlephase_circular test.test_import.SinglephaseInitTests.
174+
166175
Module state:
167176
168177
* fields
@@ -740,3 +749,53 @@ PyInit__testsinglephase_with_state_check_cache_first(void)
740749
}
741750
return PyModule_Create(&_testsinglephase_with_state_check_cache_first);
742751
}
752+
753+
754+
/****************************************/
755+
/* the _testsinglephase_circular module */
756+
/****************************************/
757+
758+
static PyObject *static_module_circular;
759+
760+
static PyObject *
761+
circularmod_clear_static_var(PyObject *self, PyObject *arg)
762+
{
763+
PyObject *result = static_module_circular;
764+
static_module_circular = NULL;
765+
return result;
766+
}
767+
768+
static struct PyModuleDef _testsinglephase_circular = {
769+
PyModuleDef_HEAD_INIT,
770+
.m_name = "_testsinglephase_circular",
771+
.m_doc = PyDoc_STR("Test module _testsinglephase_circular"),
772+
.m_methods = (PyMethodDef[]) {
773+
{"clear_static_var", circularmod_clear_static_var, METH_NOARGS,
774+
"Clear the static variable and return its previous value."},
775+
{NULL, NULL} /* sentinel */
776+
}
777+
};
778+
779+
PyMODINIT_FUNC
780+
PyInit__testsinglephase_circular(void)
781+
{
782+
if (!static_module_circular) {
783+
static_module_circular = PyModule_Create(&_testsinglephase_circular);
784+
if (!static_module_circular) {
785+
return NULL;
786+
}
787+
}
788+
static const char helper_mod_name[] = (
789+
"test.test_import.data.circular_imports.singlephase");
790+
PyObject *helper_mod = PyImport_ImportModule(helper_mod_name);
791+
Py_XDECREF(helper_mod);
792+
if (!helper_mod) {
793+
return NULL;
794+
}
795+
if(PyModule_AddStringConstant(static_module_circular,
796+
"helper_mod_name",
797+
helper_mod_name) < 0) {
798+
return NULL;
799+
}
800+
return Py_NewRef(static_module_circular);
801+
}

‎Python/import.c

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -815,6 +815,8 @@ static int clear_singlephase_extension(PyInterpreterState *interp,
815815

816816
// Currently, this is only used for testing.
817817
// (See _testinternalcapi.clear_extension().)
818+
// If adding another use, be careful about modules that import themselves
819+
// recursively (see gh-123880).
818820
int
819821
_PyImport_ClearExtension(PyObject *name, PyObject *filename)
820822
{
@@ -1322,12 +1324,16 @@ _extensions_cache_set(PyObject *path, PyObject *name,
13221324
value = 10000 entry == NULL
13231325
? NULL
13241326
: (struct extensions_cache_value *)entry->value;
1325-
/* We should never be updating an existing cache value. */
1326-
assert(value == NULL);
13271327
if (value != NULL) {
1328-
PyErr_Format(PyExc_SystemError,
1329-
"extension module %R is already cached", name);
1330-
goto finally;
1328+
/* gh-123880: If there's an existing cache value, it means a module is
1329+
* being imported recursively from its PyInit_* or Py_mod_* function.
1330+
* (That function presumably handles returning a partially
1331+
* constructed module in such a case.)
1332+
* We can reuse the existing cache value; it is owned by the cache.
1333+
* (Entries get removed from it in exceptional circumstances,
1334+
* after interpreter shutdown, and in runtime shutdown.)
1335+
*/
1336+
goto finally_oldvalue;
13311337
}
13321338
newvalue = alloc_extensions_cache_value();
13331339
if (newvalue == NULL) {
@@ -1392,6 +1398,7 @@ _extensions_cache_set(PyObject *path, PyObject *name,
13921398
cleanup_old_cached_def(&olddefbase);
13931399
}
13941400

1401+
finally_oldvalue:
13951402
extensions_lock_release();
13961403
if (key != NULL) {
13971404
hashtable_destroy_str(key);
@@ -2128,6 +2135,7 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0,
21282135
}
21292136

21302137

2138+
// Used in _PyImport_ClearExtension; see notes there.
21312139
static int
21322140
clear_singlephase_extension(PyInterpreterState *interp,
21332141
PyObject *name, PyObject *path)

‎Tools/c-analyzer/cpython/ignored.tsv

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -590,6 +590,7 @@ Modules/_testmultiphase.c - slots_nonmodule_with_exec_slots -
590590
Modules/_testmultiphase.c - testexport_methods -
591591
Modules/_testmultiphase.c - uninitialized_def -
592592
Modules/_testsinglephase.c - global_state -
593+
Modules/_testsinglephase.c - static_module_circular -
593594
Modules/_xxtestfuzz/_xxtestfuzz.c - _fuzzmodule -
594595
Modules/_xxtestfuzz/_xxtestfuzz.c - module_methods -
595596
Modules/_xxtestfuzz/fuzzer.c - RE_FLAG_DEBUG -

0 commit comments

Comments
 (0)
0