Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 30 additions & 16 deletions Lib/importlib/_bootstrap.py
Original file line number Diff line number Diff line change
Expand Up @@ -890,8 +890,14 @@ def _exec(spec, module):
sys.modules[spec.name] = module
return module

def _load_unlocked(spec):
def _load_unlocked(spec, finish_load=None):
# A helper for direct use by the import system.
#
# If given, finish_load() is called with the loaded module just before
# spec._initializing is cleared. Other threads may use a module whose
# spec is no longer initializing without taking the import lock, so
# anything that has to be in place by then (such as binding the module
# on its parent package) belongs in finish_load().
module = module_from_spec(spec)

# This must be done before putting the module in sys.modules
Expand Down Expand Up @@ -920,6 +926,8 @@ def _load_unlocked(spec):
module = sys.modules.pop(spec.name)
sys.modules[spec.name] = module
_verbose_message('import {!r} # {!r}', spec.name, spec.loader)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A quick glance gives me the same concern around the sprinkling.

if finish_load is not None:
finish_load(module)
finally:
spec._initializing = False

Expand Down Expand Up @@ -1295,29 +1303,35 @@ def _find_and_load_unlocked(name, import_, *, lazy_submodule=False):
return None
raise ModuleNotFoundError(f'{_ERR_MSG_PREFIX}{name!r}', name=name)
else:
def finish_load(module):
# Called by _load_unlocked() while spec._initializing is still
# true, so that no thread can see the module as fully imported
# before it is reachable as an attribute of its parent.
if parent:
# Set the module as an attribute on its parent.
parent_module = sys.modules[parent]
try:
setattr(parent_module, child, module)
except AttributeError:
msg = (f"Cannot set an attribute on {parent!r} "
f"for child module {child!r}")
_warnings.warn(msg, ImportWarning)
# Set attributes to lazy submodules on the module.
try:
_imp._set_lazy_attributes(module, name)
except Exception as e:
msg = f"Cannot set lazy attributes on {name!r}: {e!r}"
_warnings.warn(msg, ImportWarning)

if parent_spec:
# Temporarily add child we are currently importing to parent's
# _uninitialized_submodules for circular import tracking.
parent_spec._uninitialized_submodules.append(child)
try:
module = _load_unlocked(spec)
module = _load_unlocked(spec, finish_load)
finally:
if parent_spec:
parent_spec._uninitialized_submodules.pop()
if parent:
# Set the module as an attribute on its parent.
parent_module = sys.modules[parent]
try:
setattr(parent_module, child, module)
except AttributeError:
msg = f"Cannot set an attribute on {parent!r} for child module {child!r}"
_warnings.warn(msg, ImportWarning)
# Set attributes to lazy submodules on the module.
try:
_imp._set_lazy_attributes(module, name)
except Exception as e:
msg = f"Cannot set lazy attributes on {name!r}: {e!r}"
_warnings.warn(msg, ImportWarning)
return module


Expand Down
114 changes: 114 additions & 0 deletions Lib/test/test_importlib/test_threaded_import.py
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,120 @@ def do_import(name):
errors, [],
f"Import(s) failed on iteration {i}: {errors}")

def test_lazy_submodule_not_visible_before_parent_setattr(self):
# gh-149728: a submodule must not be advertised as fully imported
# before it is bound as an attribute of its parent package. A
# thread seeing that window in a package that resolves its
# submodules from a module-level __getattr__ would re-enter
# __getattr__ for the same submodule over and over, until
# RecursionError.
os.makedirs(os.path.join(TESTFN, "lazypkg"))
self.addCleanup(shutil.rmtree, TESTFN)
with open(os.path.join(TESTFN, "lazypkg", "__init__.py"), "w") as f:
f.write("""if 1:
import sys
import threading
from types import ModuleType

# Set when the import system is about to bind 'sub' on this
# package, i.e. at the start of the window under test.
setattr_reached = threading.Event()
# Set by the probing thread once it is out of that window.
probe_finished = threading.Event()
# __spec__._initializing of 'lazypkg.sub', as seen from
# inside the window.
observed_initializing = []
# Whether the probing thread was still blocked when the
# window closed.
probe_blocked = []

_importing = threading.local()

class _Package(ModuleType):
def __setattr__(self, name, value):
if name == "sub":
observed_initializing.append(
value.__spec__._initializing)
setattr_reached.set()
# Hold the window open long enough to see what
# the other thread does with it. This is a cap,
# not a synchronisation point: a thread wrongly
# handed the module returns at once and sets
# probe_finished, while a thread correctly made
# to block cannot set it at all, since it waits
# on the module lock held right here.
probe_blocked.append(
not probe_finished.wait(0.1))
super().__setattr__(name, value)

def __getattr__(name):
if name != "sub":
raise AttributeError(name)
if getattr(_importing, "sub", False):
raise AssertionError(
"lazypkg.__getattr__ re-entered for 'sub': the "
"submodule was handed out as fully imported "
"before being bound on its parent package")
_importing.sub = True
try:
import lazypkg.sub as sub
finally:
_importing.sub = False
return sub

sys.modules[__name__].__class__ = _Package
""")
with open(os.path.join(TESTFN, "lazypkg", "sub.py"), "w") as f:
f.write("X = 42\n")

sys.path.insert(0, TESTFN)
self.addCleanup(sys.path.remove, TESTFN)
for mod in ("lazypkg", "lazypkg.sub"):
self.addCleanup(forget, mod)
importlib.invalidate_caches()

pkg = importlib.import_module("lazypkg")
results = {}

def importer():
try:
results["importer"] = pkg.sub
except BaseException as exc:
results["importer_error"] = exc

def prober():
pkg.setattr_reached.wait(support.SHORT_TIMEOUT)
try:
results["prober"] = pkg.sub
except BaseException as exc:
results["prober_error"] = exc
finally:
pkg.probe_finished.set()

threads = [threading.Thread(target=prober),
threading.Thread(target=importer)]
for thread in threads:
thread.start()
for thread in threads:
thread.join(timeout=support.SHORT_TIMEOUT)
for thread in threads:
self.assertFalse(thread.is_alive(), "thread deadlocked")

for key in ("importer_error", "prober_error"):
if key in results:
raise results[key]
sub = sys.modules["lazypkg.sub"]
self.assertIs(results["importer"], sub)
self.assertIs(results["prober"], sub)
# The submodule is bound on its parent while its spec still says it
# is initializing, so no thread can take the lock-free fast path
# for it before it is reachable through the parent package.
self.assertEqual(pkg.observed_initializing, [True])
# And the other thread really was held off for as long as that
# window was open, rather than let through to a package that does
# not have 'sub' yet.
self.assertEqual(pkg.probe_blocked, [True])


def setUpModule():
thread_info = threading_helper.threading_setup()
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Fix a race in the import system where a submodule was briefly visible as
fully imported before being set as an attribute of its parent package. A
package importing its submodules from a module-level ``__getattr__`` could
raise :exc:`RecursionError` when several threads first used the same
submodule.
Loading