gh-149728: Fix free-threaded race in importlib lazy-submodule fast path - #149729
gh-149728: Fix free-threaded race in importlib lazy-submodule fast path#149729SwayamInSync wants to merge 3 commits into
Conversation
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
| @@ -0,0 +1,7 @@ | |||
| Fix a race in :mod:`importlib._bootstrap` where the fast path in | |||
There was a problem hiding this comment.
no need to say the internal details, something like "fix race in importlib when importing module which defined getattr" should be enough
Fixes a race condition in importlib's module loading when using lazy submodules, preventing RecursionError in multi-threaded scenarios.
| @@ -926,8 +927,10 @@ def _load_unlocked(spec): | |||
| module = sys.modules.pop(spec.name) | |||
| sys.modules[spec.name] = module | |||
| _verbose_message('import {!r} # {!r}', spec.name, spec.loader) | |||
There was a problem hiding this comment.
I may not be the best person to review this, but sprinkling the spec._initializing = False calls through out seems a little fragile. I would suggest doing something like adding a finish_load = None parameter to _load_unlocked and if it's not None then you call it here after all of the the other initialization has succeeded.
Most of the call sites then don't change, but _find_and_load_unlocked and then move the parent module initialization into a nested function and pass that in.
There was a problem hiding this comment.
A quick glance gives me the same concern around the sprinkling.
brettcannon
left a comment
There was a problem hiding this comment.
The approach seems fragile.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Fixes #149728.
Cause
_load_unlockedclearsspec._initializingat the end of its body, before_find_and_load_unlockedrunssetattr(parent_module, child, module). Between those two events,sys.modules[name]is set and_initializing == False, butparent.__dict__[child]is still missing. The fast path in_find_and_loadreturns the module without taking the import lock once it sees_initializing == False, so under free-threaded CPython a second thread can observe this window.IMPORT_FROM 'child'on that thread doesgetattr(parent, 'child'), falls into a lazy__getattr__, runs the sameimport parent.child as childline, fast-paths again, and recurses toRecursionError. See the linked issue for the full walkthrough and reproducer.Change
Keep
spec._initializing == Trueuntil after the parent setattr in_find_and_load_unlocked:_load_unlockedno longer clears_initializingon the success path. Failure paths still clear it._find_and_load_unlockedclears_initializingin afinallyblock aftersetattr(parent_module, child, module)and_imp._set_lazy_attributes._load_unlocked(_loadand_builtin_from_name) have no parent setattr, so they clear_initializingin a localfinally.This preserves the invariant the fast path needs:
_initializing == Falseimplies the module is reachable viagetattr(parent_module, child).Test
Lib/test/test_importlib/test_threaded_import.py::ThreadedImportTests::test_lazy_submodule_getattr_no_recursionwidens the natural microsecond race window with onethreading.Eventand verifies that an observer thread does not recurse on the lazy__getattr__. Fails on the unpatched interpreter, passes on this branch. Fulltest_importlibsuite remains green (1220/1220).Notes
__getattr__#149728