GeezerVison is showing github.com — Fix Refleak in test_import · Issue #102251 · python/cpython
Fix Refleak in test_import #102251DescriptionCommit 096d009 (for gh-101758) resulted in refleak failures. It's unclear if the leak is new or if the test simply exposed it. In 984f8ab, I skipped the leaking tests until we can fix the leak.
UPDATE: I've updates the tests to only skip when checking for refleaks. I've also added a few more tests.
Context
(expand)
(Also see gh-101758.)
Loading an extension module involves meaningfully different code paths depending on the content of the PyModuleDef. There's the difference between legacy (single-phase-init, PEP 3121) and modern (multi-phase-init, PEP 489) modules. For single-phase init, there are those that support being loaded more than once (m_size >= 0) and those that can't (m_size == -1). I've added several long comments in import.c explaining about single-phase init modules in detail. I also added some tests to verify the behavior of the single-phase-init cases more thoroughly. Those are the leaking tests.
Relevant state:
PyModuleDef.m_size - the size of the module's per-interpreter state (PyModuleObject.md_state)
- single-phase init and multi-phase init modules can have a value of 0 or greater
- such modules must not have any process-global state
- only single-phase init modules can have a value of -1, which means the module does not support being loaded more than once
- such modules may have process-global state
PyModuleDef.m_base.m_index - the index into PyInterpreterState.imports.modules_by_index (same index used for every interpreter)
- set by
PyModuleDef_Init() (e.g. when the module is created)
PyModuleDef.m_base.m_copy - a copy of the __dict__ of the last time a module was loaded using this def (only single-phase init where m_size is -1)
- set exclusively by
fix_up_extension()
- used exclusively by
import_find_extension()
- cleared by (replaced in)
fix_up_extension() if m_copy was already set (e.g. multiple interpreters, multiple modules in same file using same def)
- cleared by
_PyImport_ClearModulesByIndex() during interpreter finalization
_PyRuntime.imports.extensions - a dict mapping (filename, name) to PyModuleDef
- entry set exclusively by
fix_up_extension() (only for single-phase init modules)
- entry used exclusively by
import_find_extension()
- entry cleared by
_PyImport_Fini() at runtime finalization
interp->imports.modules_by_index - a list of single-phase-init modules loaded in this interpreter; lookups (e.g. PyState_FindModule()) use PyModuleDef.m_base.m_index
- entry set normally only by
fix_up_extension() and import_find_extension() (only single-phase init modules)
- (entry also set by
PyState_AddModule())
- used exclusively by
PyState_FindModule()
- entry cleared normally by
_PyImport_ClearModulesByIndex() during interpreter finalization
- (entry also cleared by
PyState_RemoveModule())
Code path when loading a single-phase-init module for the first time (in import.c, unless otherwise noted):
imp.load_dynamic()
importlib._boostrap._load() (using a spec with ExtensionFileLoader)
ExtensionFileLoader.create_module() (in _boostrap_external.py)
_imp.create_dynamic() (_imp_create_dynamic_impl())
import_find_extension() (not found in _PyRuntime.imports.extensions)_PyImport_LoadDynamicModuleWithSpec() (importdl.c)
_PyImport_FindSharedFuncptr()<module init func from loaded binary>()
- sets process-global state (only where
m_size == -1)
PyModule_Create()- populates per-interpreter module state (only where
m_size > 0)
- sets module attrs (on
__dict__)
- sets
def->m_base.m_init (only needed for single-phase-init where m_size >=0)
_PyImport_FixupExtensionObject()
- sets
sys.modules[spec.name]
fix_up_extension()
- set
interp->imports.modules_by_index[def->m_base.m_index]
- clear
def->m_base.m_copy (only if set and only if m_size == -1)
- set
def->m_base.m_copy to a copy of the module's __dict__ (only if m_size == -1)
- set
_PyRuntime.imports.extensions[(filename, name)]
- sets missing module attrs (e.g.
__file__)
During testing we use a helper to erase (nearly) any evidence of the module having been imported before. That means clearing the state described above.
Here's the code path:
_testinternalcapi.clear_extension() (Modules/_testinternalcapi.c)
_PyImport_ClearExtension()
clear_singlephase_extension()
- (only if there's a
_PyRuntime.imports.extensions entry)
- clear the module def's
m_copy
- replace the
interp->imports.modules_by_index entry with Py_None
- delete the
_PyRuntime.imports.extensions entry
Linked PRs
Activityericsnowcurrently commented on Feb 25, 2023 FWIW, I've spent quite a bit of time over the last week trying to find the leak, without success.
The following test method simplifies things to make it easier to chase the leak:
def test_singlephase_cleared_globals(self):
name = '_testsinglephase'
fileobj, filename, _ = imp.find_module(name)
fileobj.close()
# Load the module.
mod = imp.load_dynamic(name, filename)
# Clear the module's process-global state.
mod._clear_globals()
self.assertEqual(mod.initialized(), 0)
self.assertEqual(mod.initialized_count(), -1)
# Clean up all traces of the module in the import state.
del sys.modules[name]
_testinternalcapi.clear_extension(name, filename)Run:
./python -m test test_imp -m test_singlephase_cleared_globals./python -m test test_imp -m test_singlephase_cleared_globals -R 3:3
(UPDATE: That's basically what I later added in test_imp.SinglephaseInitTests.test_cleared_globals,
so ./python -m test test_imp -m test_cleared_globals -R 3:3.)
Observations:
- that test passes but leaks 1 object
- if you comment out the last line (
_testinternalcapi.clear_extension()) then there is no leak (the module is actually loaded the first refleak iteration and then uses the module def's m_copy for the other 5 iterations)
- you get that same outcome if you make
clear_singlephase_extension() (in import.c) a noop or even return right before Py_CLEAR(def->m_base.m_copy);
- that outcome implies that the leak is mostly likely something that isn't getting cleaned up in
clear_singlephase_extension(), either directly causing the leak or leading to a different code path
The following demonstrates that the leak does not exist between _testinternalcapi.clear_extension() and clear_singlephase_extension() (in import.c):
def test_singlephase_not_loaded(self):
name = '_testsinglephase'
fileobj, filename, _ = imp.find_module(name)
fileobj.close()
_testinternalcapi.clear_extension(name, filename)ericsnowcurrently commented on Feb 27, 2023 I'm unassigning myself, but we'll make sure this gets addressed within the next few weeks. warsaw commented on Feb 27, 2023 Just be aware that I am working on removing test_imp.py as part of the removal of import related deprecations ticket. changed the title [-]Fix Refleak in test_imp[/-] [+]Fix Refleak in test_import[/+] on May 1, 2023added 2 commits that reference this issue on May 31, 2023gh-102251: Explicitly free state for test modules with state in test_… ericsnowcurrently commented on Jun 23, 2023 vstinner commented on Jun 28, 2023 On Python 3.12, test_import.test_concurrency() leaks on Windows:
vstinner@WIN C:\victor\python\3.12>python -m test test_import -m test_concurrency -R 3:3
(...)
test_import leaked [22, 36, 28] references, sum=86
sunmy2019 commented on Jun 28, 2023
On Python 3.12, test_import.test_concurrency() leaks on Windows:
vstinner@WIN C:\victor\python\3.12>python -m test test_import -m test_concurrency -R 3:3
(...)
test_import leaked [22, 36, 28] references, sum=86
I think this is an essentially new issue. See #104796 (comment)
I would suggest opening a new one and closing this. vstinner commented on Jun 28, 2023
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|