diff --git a/packages/reflex-base/news/6933.bugfix.md b/packages/reflex-base/news/6933.bugfix.md new file mode 100644 index 00000000000..a73624371e0 --- /dev/null +++ b/packages/reflex-base/news/6933.bugfix.md @@ -0,0 +1 @@ +Loading `rxconfig.py` no longer swaps out `sys.path` for the duration of the import, which made concurrent first-time imports in other threads fail with `ModuleNotFoundError` (e.g. the lazy `granian` import while the backend starts). The cwd is now prepended and removed afterwards, a stale `rxconfig` module from a previously loaded project directory no longer fakes the existence check, and a module another thread imports while `rxconfig.py` loads is no longer mistaken for one of its dependencies and evicted. diff --git a/packages/reflex-base/src/reflex_base/config.py b/packages/reflex-base/src/reflex_base/config.py index f11fd4fb01d..3f224e59174 100644 --- a/packages/reflex-base/src/reflex_base/config.py +++ b/packages/reflex-base/src/reflex_base/config.py @@ -7,7 +7,8 @@ import sys import threading import urllib.parse -from collections.abc import Sequence +from collections.abc import Iterator, Sequence +from contextlib import contextmanager from importlib.util import find_spec from pathlib import Path from types import ModuleType @@ -808,12 +809,80 @@ def _set_persistent(self, **kwargs): _config_module_deps: set[str] = set() +class _ImportRecorder: + """Meta-path finder that records import attempts made on one thread. + + Never resolves anything. Recording per thread keeps imports other threads + happen to make during the window out of the rxconfig dep set, which a plain + sys.modules diff cannot tell apart from rxconfig's own imports. + """ + + def __init__(self) -> None: + """Initialize the recorder as inactive.""" + self._thread: int | None = None + self.names: set[str] = set() + + def start(self) -> None: + """Start recording imports made on the current thread.""" + self.names.clear() + self._thread = threading.get_ident() + + def stop(self) -> None: + """Stop recording; names stay readable.""" + self._thread = None + + def find_spec(self, fullname: str, path: Any = None, target: Any = None) -> None: + """Record the import attempt without resolving it. + + Args: + fullname: The module being imported. + path: Unused. + target: Unused. + """ + if self._thread is not None and self._thread == threading.get_ident(): + self.names.add(fullname) + + +_import_recorder = _ImportRecorder() + + +@contextmanager +def _record_imports() -> Iterator[_ImportRecorder]: + """Record imports made on the current thread while rxconfig loads. + + Yields: + The recorder, readable after the block. + """ + # Installed in place and never removed. Both ways of taking it back out are + # unsafe: importlib._find_spec iterates the list object it read from + # sys.meta_path (it only copies it since 3.14), so an in-place removal can + # make a concurrent lookup skip a real finder, and rebinding the list drops + # whatever another thread inserted meanwhile — reflex.components installs a + # redirect finder on first import, and losing it is permanent. + if _import_recorder not in sys.meta_path: + sys.meta_path.insert(0, _import_recorder) + _import_recorder.start() + try: + yield _import_recorder + finally: + _import_recorder.stop() + + def _get_config() -> Config: """Import rxconfig.py fresh and return its config object. Returns: The app config. """ + # Never cache rxconfig or its project-local dependencies — each load goes + # to disk so different RegistrationContexts hold independent Config + # instances resolved against the current project. Evict before probing: + # find_spec answers from sys.modules, so modules left behind by another + # project directory would fake the existence check below. + sys.modules.pop(constants.Config.MODULE, None) + for dep in _config_module_deps: + sys.modules.pop(dep, None) + _config_module_deps.clear() # only import the module if it exists. If a module spec exists then # the module exists. spec = find_spec(constants.Config.MODULE) @@ -821,27 +890,20 @@ def _get_config() -> Config: # we need this condition to ensure that a ModuleNotFound error is not thrown when # running unit/integration tests or during `reflex init`. return Config(app_name="", _skip_plugins_checks=True) - # Never cache rxconfig or its project-local dependencies — each load goes - # to disk so different RegistrationContexts hold independent Config - # instances resolved against the current project. - sys.modules.pop(constants.Config.MODULE, None) - for dep in _config_module_deps: - sys.modules.pop(dep, None) - _config_module_deps.clear() - before = set(sys.modules) - try: - rxconfig = importlib.import_module(constants.Config.MODULE) - finally: - # Record even on failure so a retry evicts partially-imported deps. - project_root = Path.cwd() - for name in set(sys.modules) - before: - origin = getattr(sys.modules[name], "__file__", None) - if ( - origin - and (path := Path(origin)).is_relative_to(project_root) - and "site-packages" not in path.parts - ): - _config_module_deps.add(name) + with _record_imports() as recorder: + try: + rxconfig = importlib.import_module(constants.Config.MODULE) + finally: + # Record even on failure so a retry evicts partially-imported deps. + project_root = Path.cwd() + for name in recorder.names: + origin = getattr(sys.modules.get(name), "__file__", None) + if ( + origin + and (path := Path(origin)).is_relative_to(project_root) + and "site-packages" not in path.parts + ): + _config_module_deps.add(name) return rxconfig.config @@ -872,30 +934,27 @@ def get_state_auto_setters() -> bool: def _load_config() -> Config: - """Load the config from rxconfig.py with cwd on sys.path. + """Load the config from rxconfig.py with cwd prepended to sys.path. + + Prepending (not replacing sys.path) keeps concurrent imports in other + threads working while rxconfig resolves from the app directory first. Returns: The app config. """ with _load_config_lock: - orig_sys_path = sys.path.copy() - sys.path.clear() - sys.path.append(str(Path.cwd())) + # A fresh str object, so the exact inserted entry can be removed by + # identity: rxconfig.py may itself add or remove equal cwd entries, + # which removal by value could confuse with caller-owned ones. + cwd = str(Path.cwd()) + sys.path.insert(0, cwd) try: return _get_config() - except Exception: - # If the module import fails, try to import with the original sys.path. - sys.path.extend(orig_sys_path) - return _get_config() finally: - # Find any entries added to sys.path by rxconfig.py itself. - extra_paths = [ - p for p in sys.path if p not in orig_sys_path and p != str(Path.cwd()) - ] - # Restore the original sys.path. - sys.path.clear() - sys.path.extend(extra_paths) - sys.path.extend(orig_sys_path) + for i, entry in enumerate(sys.path): + if entry is cwd: + del sys.path[i] + break def get_config(reload: bool = False) -> Config: diff --git a/tests/units/test_config.py b/tests/units/test_config.py index 3fddf1d8090..fc8ae9b66a1 100644 --- a/tests/units/test_config.py +++ b/tests/units/test_config.py @@ -1,8 +1,12 @@ import logging import multiprocessing import os +import sys +import textwrap import threading import time +import types +from collections.abc import Generator from pathlib import Path from typing import Any @@ -892,6 +896,190 @@ def worker(i: int) -> None: assert all(config is results[0] for config in results) +def test_load_config_keeps_sys_path_usable_for_other_threads( + monkeypatch: pytest.MonkeyPatch, +): + """Importing an unrelated module while rxconfig loads must succeed. + + _load_config used to clear sys.path down to the cwd for the duration of + the rxconfig import, so any concurrent first-time import in another + thread (e.g. the lazy granian import when the backend starts) failed + with ModuleNotFoundError. + + Args: + monkeypatch: The pytest monkeypatch fixture. + """ + inside_load = threading.Event() + release_load = threading.Event() + + def blocking_get_config() -> rx.Config: + inside_load.set() + release_load.wait(timeout=5) + return rx.Config(app_name="racer") + + monkeypatch.setattr(reflex_base.config, "_get_config", blocking_get_config) + # A stdlib module that nothing imports by default; drop it so the import + # below walks sys.path again. + monkeypatch.delitem(sys.modules, "colorsys", raising=False) + sys_path_before = sys.path.copy() + + loader = threading.Thread(target=reflex_base.config._load_config) + loader.start() + try: + assert inside_load.wait(timeout=5) + import colorsys # noqa: F401 + finally: + release_load.set() + loader.join(timeout=5) + assert not loader.is_alive() + # The temporarily prepended cwd entry was removed again. + assert sys.path == sys_path_before + + +def test_load_config_keeps_caller_owned_cwd_entry(monkeypatch: pytest.MonkeyPatch): + """A pre-existing cwd entry survives even if rxconfig removes one itself. + + The cleanup must only take back the entry _load_config prepended, not a + caller-owned equal entry. + + Args: + monkeypatch: The pytest monkeypatch fixture. + """ + cwd = str(Path.cwd()) + monkeypatch.setattr(sys, "path", [cwd, *sys.path]) + caller_owned = sys.path.count(cwd) + + def removing_get_config() -> rx.Config: + sys.path.remove(cwd) + return rx.Config(app_name="pathological") + + monkeypatch.setattr(reflex_base.config, "_get_config", removing_get_config) + reflex_base.config._load_config() + assert sys.path.count(cwd) == caller_owned + + +@pytest.fixture +def clean_config_modules() -> Generator[None, None, None]: + """Drop the modules and dep records a real rxconfig load leaves behind. + + Yields: + None, once the module table is clean. + """ + names = ("rxconfig", "side_module") + try: + yield + finally: + for name in names: + sys.modules.pop(name, None) + reflex_base.config._config_module_deps.clear() + + +def test_concurrent_import_not_recorded_as_rxconfig_dep( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, clean_config_modules: None +): + """A project-local module imported by another thread mid-load is not evicted. + + Dependency recording used to diff sys.modules around the rxconfig import, + so a concurrent import from another thread was misattributed to rxconfig + and evicted from sys.modules on the next config load. + + Args: + tmp_path: The pytest tmp_path fixture. + monkeypatch: The pytest monkeypatch fixture. + clean_config_modules: Cleanup for modules left behind by the load. + """ + (tmp_path / "rxconfig.py").write_text( + textwrap.dedent( + """ + import _config_race_gate + import reflex as rx + + _config_race_gate.in_load.set() + _config_race_gate.release.wait(timeout=5) + config = rx.Config(app_name="depapp") + """ + ) + ) + (tmp_path / "side_module.py").write_text("value = 42\n") + gate = types.ModuleType("_config_race_gate") + gate.in_load = threading.Event() # pyright: ignore[reportAttributeAccessIssue] + gate.release = threading.Event() # pyright: ignore[reportAttributeAccessIssue] + monkeypatch.setitem(sys.modules, "_config_race_gate", gate) + monkeypatch.chdir(tmp_path) + monkeypatch.delitem(sys.modules, "side_module", raising=False) + + loader = threading.Thread(target=reflex_base.config._load_config) + loader.start() + try: + assert gate.in_load.wait(timeout=5) + # Import a project-local module from this thread while rxconfig loads. + import side_module # noqa: F401 # pyright: ignore[reportMissingImports] + finally: + gate.release.set() + loader.join(timeout=5) + assert not loader.is_alive() + + assert "side_module" not in reflex_base.config._config_module_deps + # A second load must not evict the concurrently imported module. + gate.release.set() + reflex_base.config._load_config() + assert "side_module" in sys.modules + + +def test_record_imports_never_rebinds_meta_path(): + """Recording must mutate sys.meta_path in place, never rebind it. + + Rebinding drops finders another thread inserted while the replacement list + was being built. reflex.components installs its redirect finder on first + import, and losing it makes every later reflex.components.* import fail + with ModuleNotFoundError for the rest of the process. + """ + meta_path = sys.meta_path + with reflex_base.config._record_imports(): + assert sys.meta_path is meta_path + assert sys.meta_path is meta_path + + +def test_load_config_survives_rxconfig_rebuilding_meta_path( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, clean_config_modules: None +): + """A load succeeds even if rxconfig.py rebuilds sys.meta_path. + + The recorder is dropped by the rebuild, so the next load has to reinstall + it rather than assume it is still there. + + Args: + tmp_path: The pytest tmp_path fixture. + monkeypatch: The pytest monkeypatch fixture. + clean_config_modules: Cleanup for modules left behind by the load. + """ + (tmp_path / "rxconfig.py").write_text( + textwrap.dedent( + """ + import sys + import reflex as rx + from reflex_base.config import _import_recorder + + sys.meta_path = [f for f in sys.meta_path if f is not _import_recorder] + config = rx.Config(app_name="metapathapp") + """ + ) + ) + monkeypatch.chdir(tmp_path) + meta_path = sys.meta_path + contents_before = meta_path.copy() + try: + config = reflex_base.config._load_config() + assert config.app_name == "metapathapp" + assert reflex_base.config._import_recorder not in sys.meta_path + # The next load reinstalls the recorder, so deps are recorded again. + reflex_base.config._load_config() + assert "rxconfig" in reflex_base.config._config_module_deps + finally: + meta_path[:] = contents_before + sys.meta_path = meta_path + + def test_get_config_reload_deprecated(mocker: MockerFixture): """get_config(reload=True) reloads the config and warns about deprecation.