gh-151408: Fix bug with lazy importing a previously imported and removed module#151412
gh-151408: Fix bug with lazy importing a previously imported and removed module#151412brittanyrey wants to merge 5 commits into
Conversation
brittanyrey
requested review from
DinoV,
FFY00,
Yhg1s,
brettcannon,
ericsnowcurrently,
markshannon,
ncoghlan,
pablogsal and
warsaw
as code owners
| submod = import_get_module(tstate, fullmodname); | ||
| } | ||
| Py_DECREF(fullmodname); | ||
| if (submod != NULL) { |
There was a problem hiding this comment.
sys.modules[fullmodname] can be None. This returns it as the binding instead of raising ModuleNotFoundError; we need to handle the sentinel explicitly.
| Py_DECREF(attr); | ||
| } | ||
| else { | ||
| obj = _PyEval_ImportFrom(tstate, from, lz->lz_attr); |
There was a problem hiding this comment.
Small nit: on the not-found path we look up lz->lz_attr here and then _PyEval_ImportFrom looks it up again, so a package __getattr__ runs twice, no? Not a blocker, but worth a comment if we keep it.
| return Py_NewRef(attr); | ||
| } | ||
|
|
||
| int is_package = PyObject_HasAttrWithError(package, &_Py_ID(__path__)); |
There was a problem hiding this comment.
Nit: do we have a test for the non-package branch? We dropped test_from_import_module_attr_from_non_package in the last commit, so the is_package <= 0 and matches <= 0 paths here look uncovered now. Maybe add a small lazy from test where the parent is a plain module exposing a module-valued attribute.
| return NULL; | ||
| } | ||
| if (ret != NULL) { | ||
| PyObject *resolved = _PyImport_ResolveLazyImportFromAttr( |
There was a problem hiding this comment.
This re-import runs while executing the lazy from statement, before bar is accessed. We should keep a lazy proxy here and repair it during reification.
|
|
||
| PyObject *submod = import_get_module(tstate, fullmodname); | ||
| if (submod == NULL && !_PyErr_Occurred(tstate)) { | ||
| PyObject *imported = PyImport_ImportModuleLevelObject( |
There was a problem hiding this comment.
This bypasses the __import__ captured by the lazy object, so custom import hooks are ignored. We need to pass the captured hook through this path.
| return NULL; | ||
| } | ||
|
|
||
| int matches = (attr_name != NULL && PyUnicode_Check(attr_name)) |
There was a problem hiding this comment.
__name__ == fullmodname does not prove this is an importable submodule. Packages can export synthetic modules, so this turns a valid from-import into ModuleNotFoundError.
| return matches < 0 ? NULL : Py_NewRef(attr); | ||
| } | ||
|
|
||
| PyObject *submod = import_get_module(tstate, fullmodname); |
There was a problem hiding this comment.
This can return a module that is still initializing in another thread. We need the initialization wait and recheck from PyImport_GetModule().
2 participants
Fix
This PR ensures that a submodule is fully re-imported instead of relying on pre-existing cached submodule objects.
Testing
This PR adds several tests for TDD that fail prior to the
ceval.cchanges.Also, after this PR lands--
test_traceno longer fails withlazy_imports=allas documented in the attached issue.#151408