[3.15] gh-159963: Don't hold parent package import locks while a submodule executes (GH-159965) (#160630)

gh-159963: Don't hold parent package import locks while a submodule executes (GH-159965)

Since gh-83065 (3.15.0), the first import of pkg.mod kept pkg's module
lock until pkg/mod.py had finished executing.  Threads that needed only
the finished pkg stayed blocked for that long, and concurrent first
imports could raise _DeadlockError or a spurious "partially initialized
module" ImportError, or hang, with no import cycle involved.

Parent packages are now imported, or waited for if another thread is
initializing them, before the submodule's module lock is taken, so only
that lock is held while the submodule executes.  The lock ordering
deadlock fixed by gh-83065 stays fixed.

importlib.import_module() now returns the partially initialized module
in a circular import split across threads, as the import statement
already did, instead of raising _DeadlockError.  That was long-standing
behavior rather than part of the 3.15.0 regression.
(cherry picked from commit 7159b15)

Co-authored-by: Gregory P. Smith <68491+gpshead@users.noreply.github.com>

Author: miss-islington

Doc/whatsnew/3.15.rst

@@ -2912,6 +2912,18 @@ that may require changes to your code.
 Notable changes in 3.15.2
 =========================
 
+importlib
+---------
+
+* Threads that simultaneously perform first-time ``import package.submodule``
+  imports no longer risk a deadlock or spurious import errors, a regression
+  introduced in 3.15.0 by the fix for :gh:`83065`.  A package's import lock is
+  now released as soon as its ``__init__`` returns.
+  :func:`importlib.import_module` now returns the partially initialized
+  module in a circular import split across threads, as the :keyword:`import`
+  statement already did, instead of raising ``_DeadlockError``.
+  (Contributed by Gregory P. Smith in :gh:`159963`.)
+
 ssl
 ---
 

Lib/importlib/_bootstrap.py

@@ -424,64 +424,6 @@ def __exit__(self, *args, **kwargs):
         self._lock.release()
 
 
-def _get_module_chain(name):
-    """Return the chain of dotted-name prefixes from root to leaf.
-
-    For example: 'a.b.c' -> ['a', 'a.b', 'a.b.c']
-    """
-    parts = name.split('.')
-    return ['.'.join(parts[:i+1]) for i in range(len(parts))]
-
-
-class _HierarchicalLockManager:
-    """Manages acquisition of multiple module locks in hierarchical order.
-
-    This prevents deadlocks by ensuring all threads acquire locks in the
-    same order (parent modules before child modules).
-    """
-
-    def __init__(self, name):
-        self._name = name
-        self._module_chain = _get_module_chain(name)
-        self._locks = []
-
-    def __enter__(self):
-        try:
-            for module_name in self._module_chain:
-                # Only acquire lock if module is not already fully loaded
-                module = sys.modules.get(module_name)
-                if (module is None or
-                    getattr(getattr(module, "__spec__", None),
-                            "_initializing", False)):
-                    lock = _get_module_lock(module_name)
-                    try:
-                        lock.acquire()
-                    except _DeadlockError:
-                        if module_name == self._name:
-                            raise
-                        # The parent is being initialised by a thread that
-                        # is (transitively) waiting on a lock we hold.
-                        # Apply the same policy as _lock_unlock_module():
-                        # accept a partially-initialised parent for circular
-                        # imports rather than failing the whole chain.
-                        continue
-                    self._locks.append((module_name, lock))
-        except:
-            # __exit__ is not called when __enter__ raises (e.g. _DeadlockError
-            # on the leaf lock, or KeyboardInterrupt), so release whatever we
-            # already hold to avoid permanently leaking held module locks.
-            for module_name, lock in reversed(self._locks):
-                lock.release()
-            self._locks.clear()
-            raise
-        return self
-
-    def __exit__(self, *args, **kwargs):
-        for module_name, lock in reversed(self._locks):
-            lock.release()
-        self._locks.clear()
-
-
 # The following two functions are for consumption by Python/import.c.
 
 def _get_module_lock(name):
@@ -1262,17 +1204,15 @@ def _sanity_check(name, package, level):
 
 def _find_and_load_unlocked(name, import_, *, lazy_submodule=False):
     path = None
-    sys.audit(
-        "import",
-        name,
-        path,
-        getattr(sys, "path", None),
-        getattr(sys, "meta_path", None),
-        getattr(sys, "path_hooks", None)
-    )
     parent = name.rpartition('.')[0]
     parent_spec = None
     if parent:
+        # _resolve_parents() has normally imported the parent already.  It can
+        # be missing again by now if its import failed in another thread or
+        # something removed it from sys.modules; it is then imported afresh,
+        # like any other module that is not in sys.modules.  That import runs
+        # while holding name's lock, the lock order _resolve_parents() exists
+        # to avoid; it is tolerated here because the case is so rare.
         if parent not in sys.modules:
             _call_with_frames_removed(import_, parent)
         # Crazy side-effects!
@@ -1330,31 +1270,90 @@ def _find_and_load_unlocked(name, import_, *, lazy_submodule=False):
 _NEEDS_LOADING = object()
 
 
+# Return whether module (a sys.modules value) is still executing.
+def _is_initializing(module):
+    return getattr(getattr(module, '__spec__', None), '_initializing', False)
+
+
+# Import, or wait for, the parent packages of name.
+#
+# _find_and_load() calls this before taking name's module lock, so that no
+# thread holds a submodule's lock while waiting for its parent.  This avoids
+# the deadlocks and import errors of gh-83065 and gh-159963.
+def _resolve_parents(name, import_):
+    # Wait for each ancestor package that another thread is still
+    # initializing, outermost first.  Importing beneath a package while its
+    # __init__ is part way through its own imports could give one of the two
+    # threads a partially initialized module.  Missing ancestors are not
+    # imported: only the immediate parent has to exist.
+    parts = name.split('.')
+    for i in range(1, len(parts) - 1):
+        ancestor = '.'.join(parts[:i])
+        if _is_initializing(sys.modules.get(ancestor)):
+            _lock_unlock_module(ancestor)
+    # Import the immediate parent, or wait for it likewise.
+    parent = name.rpartition('.')[0]
+    if parent:
+        module = sys.modules.get(parent, _NEEDS_LOADING)
+        if module is _NEEDS_LOADING or _is_initializing(module):
+            _call_with_frames_removed(import_, parent)
+
+
 def _find_and_load(name, import_, *, lazy_submodule=False):
     """Find and load the module."""
 
     # Optimization: we avoid unneeded module locking if the module
     # already exists in sys.modules and is fully initialized.
     module = sys.modules.get(name, _NEEDS_LOADING)
-    if (module is _NEEDS_LOADING or
-        getattr(getattr(module, "__spec__", None), "_initializing", False)):
-
-        if '.' in name:
-            lock_manager = _HierarchicalLockManager(name)
-        else:
-            lock_manager = _ModuleLockManager(name)
-
-        with lock_manager:
-            module = sys.modules.get(name, _NEEDS_LOADING)
-            if module is _NEEDS_LOADING:
-                return _find_and_load_unlocked(
-                    name, import_, lazy_submodule=lazy_submodule)
-
-        # Optimization: only call _bootstrap._lock_unlock_module() if
-        # module.__spec__._initializing is True.
-        # NOTE: because of this, initializing must be set *before*
-        # putting the new module in sys.modules.
-        _lock_unlock_module(name)
+    if module is _NEEDS_LOADING or _is_initializing(module):
+        if module is not _NEEDS_LOADING:
+            # Wait for the thread executing the module.
+            # NOTE: this relies on __spec__._initializing being set *before*
+            # the module is put in sys.modules.
+            lock = _get_module_lock(name)
+            try:
+                lock.acquire()
+            except _DeadlockError:
+                # A circular import split across threads: accept the
+                # partially initialized module.
+                pass
+            else:
+                # The module is gone if the thread loading it failed.  Look
+                # while still holding the lock, so that another thread cannot
+                # have begun loading it again.
+                try:
+                    module = sys.modules.get(name, _NEEDS_LOADING)
+                finally:
+                    lock.release()
+
+        if module is _NEEDS_LOADING:
+            sys.audit(
+                "import",
+                name,
+                None,
+                getattr(sys, "path", None),
+                getattr(sys, "meta_path", None),
+                getattr(sys, "path_hooks", None)
+            )
+            _resolve_parents(name, import_)
+            lock = _get_module_lock(name)
+            try:
+                lock.acquire()
+            except _DeadlockError:
+                # A circular import split across threads.  As in
+                # _lock_unlock_module(), accept a partially initialized
+                # module if there is one.
+                module = sys.modules.get(name, _NEEDS_LOADING)
+                if module is _NEEDS_LOADING:
+                    raise
+            else:
+                try:
+                    module = sys.modules.get(name, _NEEDS_LOADING)
+                    if module is _NEEDS_LOADING:
+                        return _find_and_load_unlocked(
+                            name, import_, lazy_submodule=lazy_submodule)
+                finally:
+                    lock.release()
     else:
         # Verify the module is still in sys.modules. Another thread may have
         # removed it (due to import failure) between our sys.modules.get()

Lib/test/test_importlib/import_/test_packages.py

@@ -1,5 +1,6 @@
 from test.test_importlib import util
 import sys
+import types
 import unittest
 from test.support import import_helper
 
@@ -100,6 +101,21 @@ def module_injection():
                 finally:
                     import_helper.unload(subname)
 
+    def test_missing_grandparent_not_imported(self):
+        # Only the immediate parent has to be present.  With a hand-made
+        # 'pkg.sub' package in sys.modules and no 'pkg' at all, importing
+        # pkg.sub.mod must neither fail nor try to import 'pkg'.
+        parent = types.ModuleType('pkg.sub')
+        parent.__path__ = []
+        with util.mock_spec('pkg.sub.mod') as mock:
+            with util.import_state(meta_path=[mock]):
+                with util.uncache('pkg', 'pkg.sub', 'pkg.sub.mod'):
+                    sys.modules['pkg.sub'] = parent
+                    module = self.__import__('pkg.sub.mod', fromlist=['*'])
+                    self.assertEqual(module.__name__, 'pkg.sub.mod')
+                    self.assertIs(parent.mod, module)
+                    self.assertNotIn('pkg', sys.modules)
+
 
 (Frozen_ParentTests,
  Source_ParentTests