Repository navigation
Defer hooks registered while watched modules are initializing - #367
Rutvik552k wants to merge 2 commits into
Conversation
| if name in _post_import_hooks and getattr( | ||
| getattr(module, "__spec__", None), "_initializing", False | ||
| ): |
There was a problem hiding this comment.
Probably, just name in _post_import_hooks is enough here, since notify_module_loaded(), as you mentioned in your comment, removes the list of hooks once notification starts (which, by the way, for some reason contradicts the first comment in the module…).
There was a problem hiding this comment.
I checked the queue-only condition against a loaded-module case: register a pending hook, then insert an already initialized types.ModuleType into sys.modules, and register another hook. Both the original implementation and this patch run the new hook immediately; checking only queue membership leaves it queued. I retained the initialization guard to preserve that existing behavior and added test_loaded_module_with_pending_hook_runs_new_hook_immediately.
You are right about the introductory comment: I corrected it to describe removing the entry before notification. This follow-up changes only that comment and the regression test. The hook test file passes (12 tests), and the new test also passes with extensions disabled (1 passed, 11 deselected). The latter run printed an ignored pyreadline3 console-destructor warning at interpreter shutdown; no baseline comparison or pre-existing-warning claim is made. The previously recorded broader results are reused because the runtime implementation is unchanged; the full version matrix remains unrun.
This assessment and follow-up used AI assistance; no human review is claimed.
There was a problem hiding this comment.
Both the original implementation and this patch run the new hook immediately; checking only queue membership leaves it queued.
But is this behavior really worth preserving? With it, the old hook is never fired! If the new hook is left in the queue, the firing order will not be violated, and the user is also more likely to notice that something went wrong.
When a module already watched by wrapt registers another hook during its import, the new hook runs against the partially initialized module in
sys.modules. Reading an attribute defined later in the module body raisesAttributeErrorand can abort the import.Keep the new hook in the existing pending queue while the module spec is initializing. Once notification removes that queue, registrations from inside another hook continue to run immediately. The tests exercise a circular-import registration, reentrant registration during notification, and registration from another thread while the module body is paused.
Related: #366. This addresses the case where wrapt already watches the module before import begins. It does not address installing the first hook after an unwrapped loader has started importing the module.
Validation on Windows, CPython 3.12.13:
6b4e844950213118b19e613248d2bc9469eb731ewith missing-attribute errors on partially initialized modules.pytest -p no:cacheprovider tests/core -q -r s: C extension enabled, 1377 passed, 6 skipped;WRAPT_DISABLE_EXTENSIONS=true, 1375 passed, 8 skipped. Skips cover Python 3.14 deferred annotations and, for pure Python, two C-only paths.pytest -p no:cacheprovider tests/conftest.py -q: 37 passed, using the repository-pinned mypy 1.20.1.mypy --python-version 3.12 src/wrapt: no issues in 14 source files.git diff --checkpasses.The full
just testversion matrix was impractical in this constrained Windows workspace (justis unavailable); no other Python versions, PyPy, free-threaded stress tests, or clean-install variant matrix were run. Tests used the checkout throughPYTHONPATH=src, pytest 8.4.2 with plugin autoload disabled, and an in-place C extension build. No stubs changed.AI assistance was used to develop the patch, tests and review. No human review is claimed.