Skip to content

Defer hooks registered while watched modules are initializing - #367

Draft
Rutvik552k wants to merge 2 commits into
GrahamDumpleton:developfrom
Rutvik552k:fix/defer-hooks-during-import-366
Draft

Rutvik552k wants to merge 2 commits into
GrahamDumpleton:developfrom
Rutvik552k:fix/defer-hooks-during-import-366

Conversation

@Rutvik552k

Copy link
Copy Markdown

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 raises AttributeError and 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:

  • Both new regression tests fail on untouched implementation at 6b4e844950213118b19e613248d2bc9469eb731e with 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.
  • isort checks and scoped Ruff E9/F63/F7/F82 checks pass for both changed files. Black 25.1.0 passes on the implementation; the test file has one unchanged formatting discrepancy in the existing string-hook test, reproduced on the base file. git diff --check passes.

The full just test version matrix was impractical in this constrained Windows workspace (just is unavailable); no other Python versions, PyPy, free-threaded stress tests, or clean-install variant matrix were run. Tests used the checkout through PYTHONPATH=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.

Comment thread src/wrapt/importer.py
Comment on lines +86 to +88
if name in _post_import_hooks and getattr(
getattr(module, "__spec__", None), "_initializing", False
):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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…).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@x42005e1f x42005e1f Oct 30, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants