From e062b15e2a755dc51a26ed3900a537d8c82a4177 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 13 Feb 2023 16:33:25 -0700 Subject: [PATCH 1/6] Add test_imp.ImportTests.test_singlephase_multiple_interpreters. --- Lib/test/test_imp.py | 57 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/Lib/test/test_imp.py b/Lib/test/test_imp.py index c85ab92307de78..8f18ebbf1f4829 100644 --- a/Lib/test/test_imp.py +++ b/Lib/test/test_imp.py @@ -1,19 +1,23 @@ import gc import importlib import importlib.util +import shutil import os import os.path import py_compile import sys +import tempfile from test import support from test.support import import_helper from test.support import os_helper from test.support import script_helper from test.support import warnings_helper +import textwrap import unittest import warnings imp = warnings_helper.import_deprecated('imp') import _imp +import _xxsubinterpreters as _interpreters OS_PATH_NAME = os.path.__name__ @@ -67,6 +71,17 @@ def setUp(self): self.test_strings = mod.test_strings self.test_path = mod.__path__ + def _copy_extension(self, name): + fileobj, pathname, _ = imp.find_module('_testsinglephase') + fileobj.close() + + dirname = tempfile.mkdtemp() + self.addCleanup(os_helper.rmtree, dirname) + + copied = os.path.join(dirname, os.path.basename(pathname)) + shutil.copyfile(pathname, copied) + return copied + # test_import_encoded_module moved to test_source_encoding.py def test_find_module_encoding(self): @@ -251,6 +266,48 @@ def test_issue16421_multiple_modules_in_one_dll(self): with self.assertRaises(ImportError): imp.load_dynamic('nonexistent', pathname) + @requires_load_dynamic + def test_singlephase_multiple_interpreters(self): + # Currently, for every single-phrase init module loaded + # in multiple interpreters, those interpreters share a + # PyModuleDef for that object, which can be a problem. + + # This single-phase module has global state, which is shared + # by the interpreters. + name = '_testsinglephase' + filename = self._copy_extension(name) + + interp1 = _interpreters.create(isolated=False) + self.addCleanup(_interpreters.destroy, interp1) + interp2 = _interpreters.create(isolated=False) + self.addCleanup(_interpreters.destroy, interp2) + + script = textwrap.dedent(f''' + from test.support import warnings_helper + imp = warnings_helper.import_deprecated('imp') + module = imp.load_dynamic({name!r}, {filename!r}) + + init_count = module.initialized_count() + if init_count != %d: + raise Exception(init_count) + + lookedup = module.look_up_self() + if lookedup is not module: + raise Exception((module, lookedup)) + ''') + # Use an interpreter that gets destroyed right away. + ret = support.run_in_subinterp(script % 1) + self.assertEqual(ret, 0) + + # The module's init func gets run again. + # The module's globals did not get destroyed. + _interpreters.run_string(interp1, script % 2) + + # The module's init func is not run again. + # The second interpreter copies the module's m_copy. + # However, globals are still shared. + _interpreters.run_string(interp2, script % 2) + @requires_load_dynamic def test_singlephase_variants(self): '''Exercise the most meaningful variants described in Python/import.c.''' From 0f9a66358840129cd4f2afc44972f2d4dbc96586 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 15 Feb 2023 10:33:55 -0700 Subject: [PATCH 2/6] Add _PyImport_ClearExtension(). --- Include/internal/pycore_import.h | 3 ++ Modules/_testinternalcapi.c | 15 +++++++ Python/import.c | 76 +++++++++++++++++++++++++++++++- 3 files changed, 92 insertions(+), 2 deletions(-) diff --git a/Include/internal/pycore_import.h b/Include/internal/pycore_import.h index da766253ef6b9c..6ee7356b41c021 100644 --- a/Include/internal/pycore_import.h +++ b/Include/internal/pycore_import.h @@ -153,6 +153,9 @@ PyAPI_DATA(const struct _frozen *) _PyImport_FrozenStdlib; PyAPI_DATA(const struct _frozen *) _PyImport_FrozenTest; extern const struct _module_alias * _PyImport_FrozenAliases; +// for testing +PyAPI_FUNC(int) _PyImport_ClearExtension(PyObject *name, PyObject *filename); + #ifdef __cplusplus } #endif diff --git a/Modules/_testinternalcapi.c b/Modules/_testinternalcapi.c index ba57719d92096b..632fac2de0c419 100644 --- a/Modules/_testinternalcapi.c +++ b/Modules/_testinternalcapi.c @@ -671,6 +671,20 @@ get_interp_settings(PyObject *self, PyObject *args) } +static PyObject * +clear_extension(PyObject *self, PyObject *args) +{ + PyObject *name = NULL, *filename = NULL; + if (!PyArg_ParseTuple(args, "OO:clear_extension", &name, &filename)) { + return NULL; + } + if (_PyImport_ClearExtension(name, filename) < 0) { + return NULL; + } + Py_RETURN_NONE; +} + + static PyMethodDef module_functions[] = { {"get_configs", get_configs, METH_NOARGS}, {"get_recursion_depth", get_recursion_depth, METH_NOARGS}, @@ -692,6 +706,7 @@ static PyMethodDef module_functions[] = { _TESTINTERNALCAPI_COMPILER_CODEGEN_METHODDEF _TESTINTERNALCAPI_OPTIMIZE_CFG_METHODDEF {"get_interp_settings", get_interp_settings, METH_VARARGS, NULL}, + {"clear_extension", clear_extension, METH_VARARGS, NULL}, {NULL, NULL} /* sentinel */ }; diff --git a/Python/import.c b/Python/import.c index ae27aaf56848d6..87981668a30505 100644 --- a/Python/import.c +++ b/Python/import.c @@ -632,6 +632,28 @@ exec_builtin_or_dynamic(PyObject *mod) { } +static int clear_singlephase_extension(PyInterpreterState *interp, + PyObject *name, PyObject *filename); + +// Currently, this is only used for testing. +// (See _testinternalcapi.clear_extension().) +int +_PyImport_ClearExtension(PyObject *name, PyObject *filename) +{ + PyInterpreterState *interp = _PyInterpreterState_GET(); + + /* Clearing a module's C globals is up to the module. */ + if (clear_singlephase_extension(interp, name, filename) < 0) { + return -1; + } + + // In the future we'll probably also make sure the extension's + // file handle (and DL handle) is closed (requires saving it). + + return 0; +} + + /*******************/ #if defined(__EMSCRIPTEN__) && defined(PY_CALL_TRAMPOLINE) @@ -766,8 +788,30 @@ _extensions_cache_set(PyObject *filename, PyObject *name, PyModuleDef *def) return 0; } +static int +_extensions_cache_delete(PyObject *filename, PyObject *name) +{ + PyObject *extensions = EXTENSIONS; + if (extensions == NULL) { + return 0; + } + PyObject *key = PyTuple_Pack(2, filename, name); + if (key == NULL) { + return -1; + } + if (PyDict_DelItem(extensions, key) < 0) { + if (!PyErr_ExceptionMatches(PyExc_KeyError)) { + Py_DECREF(key); + return -1; + } + PyErr_Clear(); + } + Py_DECREF(key); + return 0; +} + static void -_extensions_cache_clear(void) +_extensions_cache_clear_all(void) { Py_CLEAR(EXTENSIONS); } @@ -890,6 +934,34 @@ import_find_extension(PyThreadState *tstate, PyObject *name, return mod; } +static int +clear_singlephase_extension(PyInterpreterState *interp, + PyObject *name, PyObject *filename) +{ + PyModuleDef *def = _extensions_cache_get(filename, name); + if (def == NULL) { + if (PyErr_Occurred()) { + return -1; + } + return 0; + } + + /* Clear data set when the module was initially loaded. */ + def->m_base.m_init = NULL; + Py_CLEAR(def->m_base.m_copy); + // We leave m_index alone since there's no reason to reset it. + + /* Clear the PyState_*Module() cache entry. */ + if (_modules_by_index_check(interp, def->m_base.m_index) == NULL) { + if (_modules_by_index_clear(interp, def) < 0) { + return -1; + } + } + + /* Clear the cached module def. */ + return _extensions_cache_delete(filename, name); +} + /*******************/ /* builtin modules */ @@ -2633,7 +2705,7 @@ void _PyImport_Fini(void) { /* Destroy the database used by _PyImport_{Fixup,Find}Extension */ - _extensions_cache_clear(); + _extensions_cache_clear_all(); if (import_lock != NULL) { PyThread_free_lock(import_lock); import_lock = NULL; From 8ea76e5ccc223a28f819d33aeac08e3443d4567d Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 15 Feb 2023 12:16:33 -0700 Subject: [PATCH 3/6] Add _testsinglephase._clear_globals(). --- Lib/test/test_imp.py | 5 +++++ Modules/_testsinglephase.c | 39 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_imp.py b/Lib/test/test_imp.py index 8f18ebbf1f4829..c841cd102cb93b 100644 --- a/Lib/test/test_imp.py +++ b/Lib/test/test_imp.py @@ -317,6 +317,11 @@ def test_singlephase_variants(self): fileobj, pathname, _ = imp.find_module(basename) fileobj.close() + def clean_up(): + import _testsinglephase + _testsinglephase._clear_globals() + self.addCleanup(clean_up) + modules = {} def load(name): assert name not in modules diff --git a/Modules/_testsinglephase.c b/Modules/_testsinglephase.c index 9e8dd647ee761a..8d72cc08451a1f 100644 --- a/Modules/_testsinglephase.c +++ b/Modules/_testsinglephase.c @@ -17,12 +17,27 @@ typedef struct { PyObject *str_const; } module_state; + /* Process-global state is only used by _testsinglephase since it's the only one that does not support re-init. */ static struct { int initialized_count; module_state module; -} global_state = { .initialized_count = -1 }; +} global_state = { + +#define NOT_INITIALIZED -1 + .initialized_count = NOT_INITIALIZED, +}; + +static void clear_state(module_state *state); + +static void +clear_global_state(void) +{ + clear_state(&global_state.module); + global_state.initialized_count = NOT_INITIALIZED; +} + static inline module_state * get_module_state(PyObject *module) @@ -106,6 +121,7 @@ init_state(module_state *state) return -1; } + static int init_module(PyObject *module, module_state *state) { @@ -198,10 +214,28 @@ basic_initialized_count(PyObject *self, PyObject *Py_UNUSED(ignored)) } #define INITIALIZED_COUNT_METHODDEF \ - {"initialized_count", basic_initialized_count, METH_VARARGS, \ + {"initialized_count", basic_initialized_count, METH_NOARGS, \ basic_initialized_count_doc} +PyDoc_STRVAR(basic__clear_globals_doc, +"_clear_globals()\n\ +\n\ +Free all global state and set it to uninitialized."); + +static PyObject * +basic__clear_globals(PyObject *self, PyObject *Py_UNUSED(ignored)) +{ + assert(PyModule_GetDef(self)->m_size == -1); + clear_global_state(); + Py_RETURN_NONE; +} + +#define _CLEAR_GLOBALS_METHODDEF \ + {"_clear_globals", basic__clear_globals, METH_NOARGS, \ + basic__clear_globals_doc} + + /*********************************************/ /* the _testsinglephase module (and aliases) */ /*********************************************/ @@ -223,6 +257,7 @@ static PyMethodDef TestMethods_Basic[] = { SUM_METHODDEF, INITIALIZED_METHODDEF, INITIALIZED_COUNT_METHODDEF, + _CLEAR_GLOBALS_METHODDEF, {NULL, NULL} /* sentinel */ }; From ae709dbb50e57107c5cda736706ae9469e2c6fd7 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 15 Feb 2023 12:33:26 -0700 Subject: [PATCH 4/6] Do not copy _testsinglephase. --- Lib/test/test_imp.py | 43 +++++++++++++++++++++---------------------- 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/Lib/test/test_imp.py b/Lib/test/test_imp.py index c841cd102cb93b..aecc2264383331 100644 --- a/Lib/test/test_imp.py +++ b/Lib/test/test_imp.py @@ -1,12 +1,10 @@ import gc import importlib import importlib.util -import shutil import os import os.path import py_compile import sys -import tempfile from test import support from test.support import import_helper from test.support import os_helper @@ -17,6 +15,7 @@ import warnings imp = warnings_helper.import_deprecated('imp') import _imp +import _testinternalcapi import _xxsubinterpreters as _interpreters @@ -71,17 +70,6 @@ def setUp(self): self.test_strings = mod.test_strings self.test_path = mod.__path__ - def _copy_extension(self, name): - fileobj, pathname, _ = imp.find_module('_testsinglephase') - fileobj.close() - - dirname = tempfile.mkdtemp() - self.addCleanup(os_helper.rmtree, dirname) - - copied = os.path.join(dirname, os.path.basename(pathname)) - shutil.copyfile(pathname, copied) - return copied - # test_import_encoded_module moved to test_source_encoding.py def test_find_module_encoding(self): @@ -274,8 +262,20 @@ def test_singlephase_multiple_interpreters(self): # This single-phase module has global state, which is shared # by the interpreters. - name = '_testsinglephase' - filename = self._copy_extension(name) + import _testsinglephase + name = _testsinglephase.__name__ + filename = _testsinglephase.__file__ + + del sys.modules[name] + _testsinglephase._clear_globals() + _testinternalcapi.clear_extension(name, filename) + init_count = _testsinglephase.initialized_count() + assert init_count == -1, (init_count,) + + def clean_up(): + _testsinglephase._clear_globals() + _testinternalcapi.clear_extension(name, filename) + self.addCleanup(clean_up) interp1 = _interpreters.create(isolated=False) self.addCleanup(_interpreters.destroy, interp1) @@ -283,18 +283,17 @@ def test_singlephase_multiple_interpreters(self): self.addCleanup(_interpreters.destroy, interp2) script = textwrap.dedent(f''' - from test.support import warnings_helper - imp = warnings_helper.import_deprecated('imp') - module = imp.load_dynamic({name!r}, {filename!r}) + import _testsinglephase - init_count = module.initialized_count() + init_count = _testsinglephase.initialized_count() if init_count != %d: raise Exception(init_count) - lookedup = module.look_up_self() - if lookedup is not module: - raise Exception((module, lookedup)) + lookedup = _testsinglephase.look_up_self() + if lookedup is not _testsinglephase: + raise Exception((_testsinglephase, lookedup)) ''') + # Use an interpreter that gets destroyed right away. ret = support.run_in_subinterp(script % 1) self.assertEqual(ret, 0) From c5aced3af62cd7545e5f19ec003e52f7a08ca57e Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 15 Feb 2023 15:13:25 -0700 Subject: [PATCH 5/6] Add a check for post-load m_copy. --- Lib/test/test_imp.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_imp.py b/Lib/test/test_imp.py index aecc2264383331..7269f5aee2e4af 100644 --- a/Lib/test/test_imp.py +++ b/Lib/test/test_imp.py @@ -285,13 +285,19 @@ def clean_up(): script = textwrap.dedent(f''' import _testsinglephase + expected = %d init_count = _testsinglephase.initialized_count() - if init_count != %d: + if init_count != expected: raise Exception(init_count) lookedup = _testsinglephase.look_up_self() if lookedup is not _testsinglephase: raise Exception((_testsinglephase, lookedup)) + + # Attrs set after loading are not in m_copy. + if hasattr(_testsinglephase, 'spam'): + raise Exception(_testsinglephase.spam) + _testsinglephase.spam = expected ''') # Use an interpreter that gets destroyed right away. From e832631f3a937ebe373ce7455708e5036851e24f Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 15 Feb 2023 15:28:42 -0700 Subject: [PATCH 6/6] Check attrs in m_copy. --- Lib/test/test_imp.py | 6 ++++++ Modules/_testsinglephase.c | 10 ++++++++++ 2 files changed, 16 insertions(+) diff --git a/Lib/test/test_imp.py b/Lib/test/test_imp.py index 7269f5aee2e4af..e81eb6f0a86fe8 100644 --- a/Lib/test/test_imp.py +++ b/Lib/test/test_imp.py @@ -294,6 +294,12 @@ def clean_up(): if lookedup is not _testsinglephase: raise Exception((_testsinglephase, lookedup)) + # Attrs set in the module init func are in m_copy. + _initialized = _testsinglephase._initialized + initialized = _testsinglephase.initialized() + if _initialized != initialized: + raise Exception((_initialized, initialized)) + # Attrs set after loading are not in m_copy. if hasattr(_testsinglephase, 'spam'): raise Exception(_testsinglephase.spam) diff --git a/Modules/_testsinglephase.c b/Modules/_testsinglephase.c index 8d72cc08451a1f..565221c887e5ae 100644 --- a/Modules/_testsinglephase.c +++ b/Modules/_testsinglephase.c @@ -134,6 +134,16 @@ init_module(PyObject *module, module_state *state) if (PyModule_AddObjectRef(module, "str_const", state->str_const) != 0) { return -1; } + + double d = _PyTime_AsSecondsDouble(state->initialized); + PyObject *initialized = PyFloat_FromDouble(d); + if (initialized == NULL) { + return -1; + } + if (PyModule_AddObjectRef(module, "_initialized", initialized) != 0) { + return -1; + } + return 0; }