From bf66bce4ee642e0d14d2e289490360e60980b14e Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Mon, 24 Nov 2025 13:26:35 +0100 Subject: [PATCH] gh-141780: Make PyModule_FromSlotsAndSpec enable GIL if needed (GH-141785) --- Include/internal/pycore_import.h | 11 ++- Lib/test/test_capi/test_module.py | 3 +- Lib/test/test_import/__init__.py | 69 +++++++++++++++++++ Lib/test/test_importlib/util.py | 3 +- ...-11-20-13-18-57.gh-issue-141780.xDrVNr.rst | 2 + Modules/_testcapi/module.c | 23 +++++++ Modules/_testmultiphase.c | 31 +++++++++ Objects/moduleobject.c | 17 +++++ Python/import.c | 46 ++++++++----- 9 files changed, 183 insertions(+), 22 deletions(-) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2025-11-20-13-18-57.gh-issue-141780.xDrVNr.rst diff --git a/Include/internal/pycore_import.h b/Include/internal/pycore_import.h index d64a18bb09e0..4c8b8c0ed868 100644 --- a/Include/internal/pycore_import.h +++ b/Include/internal/pycore_import.h @@ -128,11 +128,18 @@ PyAPI_FUNC(int) _PyImport_ClearExtension(PyObject *name, PyObject *filename); // state of the module argument: // - If module is NULL or a PyModuleObject with md_gil == Py_MOD_GIL_NOT_USED, // call _PyEval_DisableGIL(). -// - Otherwise, call _PyEval_EnableGILPermanent(). If the GIL was not already -// enabled permanently, issue a warning referencing the module's name. +// - Otherwise, call _PyImport_EnableGILAndWarn // // This function may raise an exception. extern int _PyImport_CheckGILForModule(PyObject *module, PyObject *module_name); +// Assuming that the GIL is enabled from a call to +// _PyEval_EnableGILTransient(), call _PyEval_EnableGILPermanent(). +// If the GIL was not already enabled permanently, issue a warning referencing +// the module's name. +// Leave a message in verbose mode. +// +// This function may raise an exception. +extern int _PyImport_EnableGILAndWarn(PyThreadState *, PyObject *module_name); #endif #ifdef __cplusplus diff --git a/Lib/test/test_capi/test_module.py b/Lib/test/test_capi/test_module.py index 7ec23e637d7d..823e2ab6b2ef 100644 --- a/Lib/test/test_capi/test_module.py +++ b/Lib/test/test_capi/test_module.py @@ -3,7 +3,7 @@ import unittest import types -from test.support import import_helper, subTests +from test.support import import_helper, subTests, requires_gil_enabled # Skip this test if the _testcapi module isn't available. _testcapi = import_helper.import_module('_testcapi') @@ -25,6 +25,7 @@ def def_and_token(mod): ) class TestModFromSlotsAndSpec(unittest.TestCase): + @requires_gil_enabled("empty slots re-enable GIL") def test_empty(self): mod = _testcapi.module_from_slots_empty(FakeSpec()) self.assertIsInstance(mod, types.ModuleType) diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py index fd9750eae804..271361ae8164 100644 --- a/Lib/test/test_import/__init__.py +++ b/Lib/test/test_import/__init__.py @@ -65,8 +65,10 @@ except ImportError: _testmultiphase = None try: import _interpreters + import concurrent.interpreters except ModuleNotFoundError: _interpreters = None + concurrent = None try: import _testinternalcapi except ImportError: @@ -3407,6 +3409,39 @@ class ModexportTests(unittest.TestCase): self.assertEqual(module.__name__, modname) + @requires_subinterpreters + def test_from_modexport_gil_used(self): + # Test that a module with Py_MOD_GIL_USED (re-)enables the GIL. + # Do this in a new interpreter to avoid interfering with global state. + modname = '_test_from_modexport_gil_used' + filename = _testmultiphase.__file__ + interp = concurrent.interpreters.create() + self.addCleanup(interp.close) + queue = concurrent.interpreters.create_queue() + interp.prepare_main( + modname=modname, + filename=filename, + queue=queue, + ) + enabled_before = sys._is_gil_enabled() + interp.exec(f"""if True: + import sys + from test.support.warnings_helper import check_warnings + from {__name__} import import_extension_from_file + with check_warnings((".*GIL..has been enabled.*", RuntimeWarning), + quiet=True): + module = import_extension_from_file(modname, filename, + put_in_sys_modules=False) + queue.put(module.__name__) + queue.put(sys._is_gil_enabled()) + """) + + self.assertEqual(queue.get(), modname) + self.assertEqual(queue.get(), True) + self.assertTrue(queue.empty()) + + self.assertEqual(enabled_before, sys._is_gil_enabled()) + def test_from_modexport_null(self): modname = '_test_from_modexport_null' filename = _testmultiphase.__file__ @@ -3428,6 +3463,39 @@ class ModexportTests(unittest.TestCase): put_in_sys_modules=False) self.assertIsInstance(module, str) + @requires_subinterpreters + def test_from_modexport_create_nonmodule_gil_used(self): + # Test that a module with Py_MOD_GIL_USED (re-)enables the GIL. + # Do this in a new interpreter to avoid interfering with global state. + modname = '_test_from_modexport_create_nonmodule_gil_used' + filename = _testmultiphase.__file__ + interp = concurrent.interpreters.create() + self.addCleanup(interp.close) + queue = concurrent.interpreters.create_queue() + interp.prepare_main( + modname=modname, + filename=filename, + queue=queue, + ) + enabled_before = sys._is_gil_enabled() + interp.exec(f"""if True: + import sys + from test.support.warnings_helper import check_warnings + from {__name__} import import_extension_from_file + with check_warnings((".*GIL..has been enabled.*", RuntimeWarning), + quiet=True): + module = import_extension_from_file(modname, filename, + put_in_sys_modules=False) + queue.put(module) + queue.put(sys._is_gil_enabled()) + """) + + self.assertIsInstance(queue.get(), str) + self.assertEqual(queue.get(), True) + self.assertTrue(queue.empty()) + + self.assertEqual(enabled_before, sys._is_gil_enabled()) + def test_from_modexport_smoke(self): # General positive test for sundry features # (PyModule_FromSlotsAndSpec tests exercise these more carefully) @@ -3456,6 +3524,7 @@ class ModexportTests(unittest.TestCase): pass self.assertEqual(_testcapi.pytype_getmodulebytoken(Sub, token), module) + @requires_gil_enabled("empty slots re-enable GIL") def test_from_modexport_empty_slots(self): # Module to test that: # - no slots are mandatory for PyModExport diff --git a/Lib/test/test_importlib/util.py b/Lib/test/test_importlib/util.py index edbe78545a25..efbec667317d 100644 --- a/Lib/test/test_importlib/util.py +++ b/Lib/test/test_importlib/util.py @@ -15,7 +15,8 @@ import sys import tempfile import types -_testsinglephase = import_helper.import_module("_testsinglephase") +# gh-116303: Skip test module dependent tests if test modules are unavailable +import_helper.import_module("_testmultiphase") BUILTINS = types.SimpleNamespace() diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2025-11-20-13-18-57.gh-issue-141780.xDrVNr.rst b/Misc/NEWS.d/next/Core_and_Builtins/2025-11-20-13-18-57.gh-issue-141780.xDrVNr.rst new file mode 100644 index 000000000000..8700ac148244 --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2025-11-20-13-18-57.gh-issue-141780.xDrVNr.rst @@ -0,0 +1,2 @@ +Fix :c:macro:`Py_mod_gil` with API added in :pep:`793`: +:c:func:`!PyModule_FromSlotsAndSpec` and ``PyModExport`` hooks diff --git a/Modules/_testcapi/module.c b/Modules/_testcapi/module.c index 9349445351ea..7b5861bc08ed 100644 --- a/Modules/_testcapi/module.c +++ b/Modules/_testcapi/module.c @@ -27,6 +27,8 @@ module_from_slots_name(PyObject *self, PyObject *spec) { PyModuleDef_Slot slots[] = { {Py_mod_name, "currently ignored..."}, + {Py_mod_multiple_interpreters, Py_MOD_PER_INTERPRETER_GIL_SUPPORTED}, + {Py_mod_gil, Py_MOD_GIL_NOT_USED}, {0}, }; return PyModule_FromSlotsAndSpec(slots, spec); @@ -37,6 +39,8 @@ module_from_slots_doc(PyObject *self, PyObject *spec) { PyModuleDef_Slot slots[] = { {Py_mod_doc, "the docstring"}, + {Py_mod_multiple_interpreters, Py_MOD_PER_INTERPRETER_GIL_SUPPORTED}, + {Py_mod_gil, Py_MOD_GIL_NOT_USED}, {0}, }; return PyModule_FromSlotsAndSpec(slots, spec); @@ -47,6 +51,8 @@ module_from_slots_size(PyObject *self, PyObject *spec) { PyModuleDef_Slot slots[] = { {Py_mod_state_size, (void*)123}, + {Py_mod_multiple_interpreters, Py_MOD_PER_INTERPRETER_GIL_SUPPORTED}, + {Py_mod_gil, Py_MOD_GIL_NOT_USED}, {0}, }; PyObject *mod = PyModule_FromSlotsAndSpec(slots, spec); @@ -72,6 +78,8 @@ module_from_slots_methods(PyObject *self, PyObject *spec) { PyModuleDef_Slot slots[] = { {Py_mod_methods, a_methoddef_array}, + {Py_mod_multiple_interpreters, Py_MOD_PER_INTERPRETER_GIL_SUPPORTED}, + {Py_mod_gil, Py_MOD_GIL_NOT_USED}, {0}, }; return PyModule_FromSlotsAndSpec(slots, spec); @@ -90,6 +98,8 @@ module_from_slots_gc(PyObject *self, PyObject *spec) {Py_mod_state_traverse, noop_traverse}, {Py_mod_state_clear, noop_clear}, {Py_mod_state_free, noop_free}, + {Py_mod_multiple_interpreters, Py_MOD_PER_INTERPRETER_GIL_SUPPORTED}, + {Py_mod_gil, Py_MOD_GIL_NOT_USED}, {0}, }; PyObject *mod = PyModule_FromSlotsAndSpec(slots, spec); @@ -118,6 +128,8 @@ module_from_slots_token(PyObject *self, PyObject *spec) { PyModuleDef_Slot slots[] = { {Py_mod_token, (void*)&test_token}, + {Py_mod_multiple_interpreters, Py_MOD_PER_INTERPRETER_GIL_SUPPORTED}, + {Py_mod_gil, Py_MOD_GIL_NOT_USED}, {0}, }; PyObject *mod = PyModule_FromSlotsAndSpec(slots, spec); @@ -144,6 +156,8 @@ module_from_slots_exec(PyObject *self, PyObject *spec) { PyModuleDef_Slot slots[] = { {Py_mod_exec, simple_exec}, + {Py_mod_multiple_interpreters, Py_MOD_PER_INTERPRETER_GIL_SUPPORTED}, + {Py_mod_gil, Py_MOD_GIL_NOT_USED}, {0}, }; PyObject *mod = PyModule_FromSlotsAndSpec(slots, spec); @@ -175,6 +189,8 @@ module_from_slots_create(PyObject *self, PyObject *spec) { PyModuleDef_Slot slots[] = { {Py_mod_create, create_attr_from_spec}, + {Py_mod_multiple_interpreters, Py_MOD_PER_INTERPRETER_GIL_SUPPORTED}, + {Py_mod_gil, Py_MOD_GIL_NOT_USED}, {0}, }; return PyModule_FromSlotsAndSpec(slots, spec); @@ -205,6 +221,8 @@ module_from_slots_repeat_slot(PyObject *self, PyObject *spec) PyModuleDef_Slot slots[] = { {slot_id, "anything"}, {slot_id, "anything else"}, + {Py_mod_multiple_interpreters, Py_MOD_PER_INTERPRETER_GIL_SUPPORTED}, + {Py_mod_gil, Py_MOD_GIL_NOT_USED}, {0}, }; return PyModule_FromSlotsAndSpec(slots, spec); @@ -219,6 +237,8 @@ module_from_slots_null_slot(PyObject *self, PyObject *spec) } PyModuleDef_Slot slots[] = { {slot_id, NULL}, + {Py_mod_multiple_interpreters, Py_MOD_PER_INTERPRETER_GIL_SUPPORTED}, + {Py_mod_gil, Py_MOD_GIL_NOT_USED}, {0}, }; return PyModule_FromSlotsAndSpec(slots, spec); @@ -233,6 +253,8 @@ module_from_def_slot(PyObject *self, PyObject *spec) } PyModuleDef_Slot slots[] = { {slot_id, "anything"}, + {Py_mod_multiple_interpreters, Py_MOD_PER_INTERPRETER_GIL_SUPPORTED}, + {Py_mod_gil, Py_MOD_GIL_NOT_USED}, {0}, }; PyModuleDef def = { @@ -280,6 +302,7 @@ module_from_def_multiple_exec(PyObject *self, PyObject *spec) static PyModuleDef_Slot slots[] = { {Py_mod_exec, simple_exec}, {Py_mod_exec, another_exec}, + {Py_mod_multiple_interpreters, Py_MOD_PER_INTERPRETER_GIL_SUPPORTED}, {Py_mod_gil, Py_MOD_GIL_NOT_USED}, {0}, }; diff --git a/Modules/_testmultiphase.c b/Modules/_testmultiphase.c index cd2d7b655982..e286eaae820b 100644 --- a/Modules/_testmultiphase.c +++ b/Modules/_testmultiphase.c @@ -1024,6 +1024,20 @@ PyModExport__test_from_modexport(void) { static PyModuleDef_Slot slots[] = { {Py_mod_name, "_test_from_modexport"}, + {Py_mod_multiple_interpreters, Py_MOD_PER_INTERPRETER_GIL_SUPPORTED}, + {Py_mod_gil, Py_MOD_GIL_NOT_USED}, + {0}, + }; + return slots; +} + +PyMODEXPORT_FUNC +PyModExport__test_from_modexport_gil_used(void) +{ + static PyModuleDef_Slot slots[] = { + {Py_mod_name, "_test_from_modexport_gil_used"}, + {Py_mod_multiple_interpreters, Py_MOD_PER_INTERPRETER_GIL_SUPPORTED}, + {Py_mod_gil, Py_MOD_GIL_USED}, {0}, }; return slots; @@ -1073,6 +1087,21 @@ PyModExport__test_from_modexport_create_nonmodule(void) static PyModuleDef_Slot slots[] = { {Py_mod_name, "_test_from_modexport_create_nonmodule"}, {Py_mod_create, modexport_create_string}, + {Py_mod_multiple_interpreters, Py_MOD_PER_INTERPRETER_GIL_SUPPORTED}, + {Py_mod_gil, Py_MOD_GIL_NOT_USED}, + {0}, + }; + return slots; +} + +PyMODEXPORT_FUNC +PyModExport__test_from_modexport_create_nonmodule_gil_used(void) +{ + static PyModuleDef_Slot slots[] = { + {Py_mod_name, "_test_from_modexport_create_nonmodule"}, + {Py_mod_create, modexport_create_string}, + {Py_mod_multiple_interpreters, Py_MOD_PER_INTERPRETER_GIL_SUPPORTED}, + {Py_mod_gil, Py_MOD_GIL_USED}, {0}, }; return slots; @@ -1165,6 +1194,8 @@ PyModExport__test_from_modexport_smoke(void) {Py_mod_methods, methods}, {Py_mod_state_free, modexport_smoke_free}, {Py_mod_token, (void*)&modexport_smoke_test_token}, + {Py_mod_multiple_interpreters, Py_MOD_PER_INTERPRETER_GIL_SUPPORTED}, + {Py_mod_gil, Py_MOD_GIL_NOT_USED}, {0}, }; return slots; diff --git a/Objects/moduleobject.c b/Objects/moduleobject.c index 6c1c5f5eb89c..5a0b16ba5724 100644 --- a/Objects/moduleobject.c +++ b/Objects/moduleobject.c @@ -2,6 +2,7 @@ #include "Python.h" #include "pycore_call.h" // _PyObject_CallNoArgs() +#include "pycore_ceval.h" // _PyEval_EnableGILTransient() #include "pycore_dict.h" // _PyDict_EnablePerThreadRefcounting() #include "pycore_fileutils.h" // _Py_wgetcwd #include "pycore_import.h" // _PyImport_GetNextModuleIndex() @@ -522,6 +523,22 @@ module_from_def_and_spec( #undef COPY_COMMON_SLOT } +#ifdef Py_GIL_DISABLED + // For modules created directly from slots (not from a def), we enable + // the GIL here (pairing `_PyEval_EnableGILTransient` with + // an immediate `_PyImport_EnableGILAndWarn`). + // For modules created from a def, the caller is responsible for this. + if (!original_def && requires_gil) { + PyThreadState *tstate = _PyThreadState_GET(); + if (_PyEval_EnableGILTransient(tstate) < 0) { + goto error; + } + if (_PyImport_EnableGILAndWarn(tstate, nameobj) < 0) { + goto error; + } + } +#endif + /* By default, multi-phase init modules are expected to work under multiple interpreters. */ if (!has_multiple_interpreters_slot) { diff --git a/Python/import.c b/Python/import.c index de183de44da8..e91c95b40d94 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1561,25 +1561,11 @@ _PyImport_CheckGILForModule(PyObject* module, PyObject *module_name) if (!PyModule_Check(module) || ((PyModuleObject *)module)->md_requires_gil) { - if (_PyEval_EnableGILPermanent(tstate)) { - int warn_result = PyErr_WarnFormat( - PyExc_RuntimeWarning, - 1, - "The global interpreter lock (GIL) has been enabled to load " - "module '%U', which has not declared that it can run safely " - "without the GIL. To override this behavior and keep the GIL " - "disabled (at your own risk), run with PYTHON_GIL=0 or -Xgil=0.", - module_name - ); - if (warn_result < 0) { - return warn_result; - } + if (PyModule_Check(module)) { + assert(((PyModuleObject *)module)->md_token_is_def); } - - const PyConfig *config = _PyInterpreterState_GetConfig(tstate->interp); - if (config->enable_gil == _PyConfig_GIL_DEFAULT && config->verbose) { - PySys_FormatStderr("# loading module '%U', which requires the GIL\n", - module_name); + if (_PyImport_EnableGILAndWarn(tstate, module_name) < 0) { + return -1; } } else { @@ -1588,6 +1574,28 @@ _PyImport_CheckGILForModule(PyObject* module, PyObject *module_name) return 0; } + +int +_PyImport_EnableGILAndWarn(PyThreadState *tstate, PyObject *module_name) +{ + if (_PyEval_EnableGILPermanent(tstate)) { + return PyErr_WarnFormat( + PyExc_RuntimeWarning, + 1, + "The global interpreter lock (GIL) has been enabled to load " + "module '%U', which has not declared that it can run safely " + "without the GIL. To override this behavior and keep the GIL " + "disabled (at your own risk), run with PYTHON_GIL=0 or -Xgil=0.", + module_name + ); + } + const PyConfig *config = _PyInterpreterState_GetConfig(tstate->interp); + if (config->enable_gil == _PyConfig_GIL_DEFAULT && config->verbose) { + PySys_FormatStderr("# loading module '%U', which requires the GIL\n", + module_name); + } + return 0; +} #endif static PyThreadState * @@ -4796,6 +4804,8 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) _PyImport_GetModuleExportHooks(&info, fp, &p0, &ex0); if (ex0) { mod = import_run_modexport(tstate, ex0, &info, spec); + // Modules created from slots handle GIL enablement (Py_mod_gil slot) + // when they're created. goto cleanup; } if (p0 == NULL) { -- 2.47.3