fix(connectors): export the package surface lazily to stop the import deadlock
`tools/connectors/__init__.py` imported all five of its submodules eagerly, so a
thread importing the package held the package's module lock while reaching for a
submodule that another thread already held. Python 3.14 aborts one of the two
with:
_frozen_importlib._DeadlockError: deadlock detected by
_ModuleLock('tools.connectors.tool')
which is the failure in the Group Chat hosted-room worker startup path
(`gateway/run_startup.py::_ensure_hosted_room_worker` -> `import tui_gateway.server`
-> `methods_connectors._install_update_hook` -> `from tools.connectors import
operation`) while the tool registry's discovery scan imports
`tools.connectors.tool` from another thread.
Exporting the cross-package surface through a module-level `__getattr__` (PEP 562)
removes the parent->child edge entirely: the package `__init__` now imports no
submodule, so it cannot hold its lock while waiting for one. Attribute access,
`from tools.connectors import x` and `__all__` are unchanged.
Measured on Python 3.14.7 by racing `import tools.connectors` (3 threads) against
`import tools.connectors.tool` (3 threads), 40 iterations each:
| tree | outcomes |
| --- | --- |
| before | 120 `_DeadlockError`, 0 successes |
| after | 0 `_DeadlockError`, 0 threads left alive, `import tools.connectors` loads no submodule |
Regression tests run in subprocesses so they assert on a pristine `sys.modules`
(`tests/tools/test_connectors_import_deadlock.py`).
Co-Authored-By: Paperclip <noreply@paperclip.ing>
(cherry picked from commit 1803f53b1ed34487ff6a60cdbde915726be21338)
This commit is contained in:
109
tests/tools/test_connectors_import_deadlock.py
Normal file
109
tests/tools/test_connectors_import_deadlock.py
Normal file
@@ -0,0 +1,109 @@
|
||||
"""Regression tests for the ``tools.connectors`` import surface.
|
||||
|
||||
The package is imported concurrently with its own submodules: the tool registry's discovery
|
||||
scan imports ``tools.connectors.tool`` while the gateway's Group Chat worker imports the
|
||||
package (``tui_gateway.methods_connectors`` -> ``tools.connectors``). When ``__init__``
|
||||
imported its submodules eagerly, one thread held the package's module lock while waiting for
|
||||
a submodule another thread already held, and Python 3.14 aborted the loser with
|
||||
``_frozen_importlib._DeadlockError: deadlock detected by _ModuleLock('tools.connectors.tool')``
|
||||
— the Group Chat worker then failed to start ("mutating Group Chat commands will fail closed").
|
||||
|
||||
Both checks run in a subprocess so the assertions see a pristine ``sys.modules`` and cannot
|
||||
perturb the test session's own imports.
|
||||
"""
|
||||
|
||||
import os
|
||||
import subprocess
|
||||
import sys
|
||||
from pathlib import Path
|
||||
|
||||
REPO_ROOT = Path(__file__).resolve().parents[2]
|
||||
|
||||
_NO_EAGER_SUBMODULE = """
|
||||
import sys
|
||||
sys.path.insert(0, REPO_ROOT)
|
||||
import tools.connectors # noqa: F401
|
||||
loaded = sorted(k for k in sys.modules if k.startswith("tools.connectors."))
|
||||
print("\\n".join(loaded))
|
||||
"""
|
||||
|
||||
_RACE = """
|
||||
import importlib, sys, threading
|
||||
sys.path.insert(0, REPO_ROOT)
|
||||
errors = []
|
||||
ITER = 25
|
||||
|
||||
def purge():
|
||||
for name in [k for k in list(sys.modules) if k.startswith("tools.connectors")]:
|
||||
sys.modules.pop(name, None)
|
||||
|
||||
def run(target):
|
||||
for _ in range(ITER):
|
||||
try:
|
||||
importlib.import_module(target)
|
||||
except Exception as exc: # noqa: BLE001
|
||||
errors.append((target, type(exc).__name__))
|
||||
purge()
|
||||
|
||||
threads = [threading.Thread(target=run, args=("tools.connectors",)) for _ in range(3)]
|
||||
threads += [threading.Thread(target=run, args=("tools.connectors.tool",)) for _ in range(3)]
|
||||
for t in threads:
|
||||
t.start()
|
||||
for t in threads:
|
||||
t.join(timeout=120)
|
||||
print("alive", sum(1 for t in threads if t.is_alive()))
|
||||
print("errors", [e for e in errors if e[1] != "KeyError"])
|
||||
"""
|
||||
|
||||
|
||||
def _run_snippet(snippet: str) -> subprocess.CompletedProcess:
|
||||
env = dict(os.environ)
|
||||
env["PYTHONPATH"] = str(REPO_ROOT)
|
||||
return subprocess.run(
|
||||
[sys.executable, "-c", snippet.replace("REPO_ROOT", repr(str(REPO_ROOT)))],
|
||||
capture_output=True,
|
||||
text=True,
|
||||
timeout=180,
|
||||
env=env,
|
||||
cwd=str(REPO_ROOT),
|
||||
)
|
||||
|
||||
|
||||
def test_package_import_does_not_load_submodules():
|
||||
"""Importing the package must not import any submodule: that eager edge is the cycle."""
|
||||
proc = _run_snippet(_NO_EAGER_SUBMODULE)
|
||||
assert proc.returncode == 0, proc.stderr
|
||||
assert proc.stdout.strip() == "", f"eagerly imported: {proc.stdout.strip()}"
|
||||
|
||||
|
||||
def test_concurrent_package_and_submodule_import_never_deadlocks():
|
||||
"""The two real-world import paths racing must not raise _DeadlockError."""
|
||||
proc = _run_snippet(_RACE)
|
||||
assert proc.returncode == 0, proc.stderr
|
||||
assert "alive 0" in proc.stdout, proc.stdout
|
||||
assert "errors []" in proc.stdout, proc.stdout
|
||||
|
||||
|
||||
def test_lazy_surface_matches_all_and_raises_for_unknown_names():
|
||||
import tools.connectors as connectors
|
||||
|
||||
assert sorted(connectors.__all__) == [
|
||||
"CONNECTOR_BATCH_SENTINEL",
|
||||
"MANAGE_CONNECTIONS_SCHEMA",
|
||||
"connector_describe",
|
||||
"connector_search_hits",
|
||||
"connectors_available",
|
||||
"dispatch_connector_batch",
|
||||
"dispatch_connector_call",
|
||||
"is_connector_name",
|
||||
"manage_connections",
|
||||
]
|
||||
for name in connectors.__all__:
|
||||
assert getattr(connectors, name) is not None
|
||||
assert set(connectors.__all__) <= set(dir(connectors))
|
||||
try:
|
||||
connectors.not_a_real_connector_name
|
||||
except AttributeError as exc:
|
||||
assert "not_a_real_connector_name" in str(exc)
|
||||
else: # pragma: no cover - the attribute must not resolve
|
||||
raise AssertionError("unknown attribute resolved")
|
||||
@@ -4,22 +4,51 @@ Only the names below are cross-package surface; imports beyond it need a design
|
||||
Siblings: ``contract`` (states, actors, transition table), ``operation`` (the record),
|
||||
``live`` (open operation per session), ``run`` (the lifecycle loop), ``managed`` / ``mcp``
|
||||
(per-kind hooks), ``targets``, ``search``, ``dispatch``, ``gateway/`` (HTTP wire + client).
|
||||
|
||||
The surface is exported lazily (PEP 562) on purpose: this package is imported concurrently
|
||||
from threads that also import its submodules directly (the tool registry's discovery scan
|
||||
imports ``tools.connectors.tool`` while the gateway's Group Chat worker imports this
|
||||
package). Eager ``from tools.connectors.<sub> import …`` lines made this module hold its own
|
||||
module lock while reaching for a submodule another thread already held, which Python 3.14
|
||||
reports as ``_frozen_importlib._DeadlockError: deadlock detected by
|
||||
_ModuleLock('tools.connectors.tool')`` and which aborts the Group Chat worker at startup. A
|
||||
package ``__init__`` that imports no submodule cannot hold that lock while waiting for one,
|
||||
so the cycle is gone. Attribute access, ``from tools.connectors import x`` and ``__all__``
|
||||
behave exactly as before.
|
||||
"""
|
||||
|
||||
from tools.connectors.dispatch import dispatch_connector_batch, dispatch_connector_call
|
||||
from tools.connectors.gateway.bridge import connector_describe, connector_search_hits
|
||||
from tools.connectors.gateway.config import connectors_available
|
||||
from tools.connectors.gateway.names import CONNECTOR_BATCH_SENTINEL, is_connector_name
|
||||
from tools.connectors.tool import MANAGE_CONNECTIONS_SCHEMA, manage_connections
|
||||
import importlib
|
||||
|
||||
__all__ = [
|
||||
"CONNECTOR_BATCH_SENTINEL",
|
||||
"MANAGE_CONNECTIONS_SCHEMA",
|
||||
"connector_describe",
|
||||
"connector_search_hits",
|
||||
"connectors_available",
|
||||
"dispatch_connector_batch",
|
||||
"dispatch_connector_call",
|
||||
"is_connector_name",
|
||||
"manage_connections",
|
||||
]
|
||||
# name -> module owning it: the single source of truth for __all__ and __getattr__.
|
||||
_LAZY_EXPORTS: "dict[str, str]" = {
|
||||
"CONNECTOR_BATCH_SENTINEL": "tools.connectors.gateway.names",
|
||||
"MANAGE_CONNECTIONS_SCHEMA": "tools.connectors.tool",
|
||||
"connector_describe": "tools.connectors.gateway.bridge",
|
||||
"connector_search_hits": "tools.connectors.gateway.bridge",
|
||||
"connectors_available": "tools.connectors.gateway.config",
|
||||
"dispatch_connector_batch": "tools.connectors.dispatch",
|
||||
"dispatch_connector_call": "tools.connectors.dispatch",
|
||||
"is_connector_name": "tools.connectors.gateway.names",
|
||||
"manage_connections": "tools.connectors.tool",
|
||||
}
|
||||
|
||||
__all__ = sorted(_LAZY_EXPORTS)
|
||||
|
||||
|
||||
def __getattr__(name: str):
|
||||
"""Resolve the public surface on first use (PEP 562).
|
||||
|
||||
Deliberately no eager import: see the module docstring for the import-deadlock this
|
||||
prevents. Module-level ``__getattr__`` is only called for names not already bound, so
|
||||
each submodule is imported at most once per process.
|
||||
"""
|
||||
module_name = _LAZY_EXPORTS.get(name)
|
||||
if module_name is None:
|
||||
raise AttributeError(f"module {__name__!r} has no attribute {name!r}")
|
||||
value = getattr(importlib.import_module(module_name), name)
|
||||
globals()[name] = value
|
||||
return value
|
||||
|
||||
|
||||
def __dir__() -> "list[str]":
|
||||
return sorted(set(globals()) | set(__all__))
|
||||
|
||||
Reference in New Issue
Block a user