diff --git a/packages/reflex-base/news/6933.bugfix.md b/packages/reflex-base/news/6933.bugfix.md new file mode 100644 index 00000000000..62eb9315dac --- /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, and a stale `rxconfig` module from a previously loaded project directory no longer fakes the existence check. diff --git a/packages/reflex-base/src/reflex_base/config.py b/packages/reflex-base/src/reflex_base/config.py index b15533da731..ef2ffbc1ac8 100644 --- a/packages/reflex-base/src/reflex_base/config.py +++ b/packages/reflex-base/src/reflex_base/config.py @@ -808,12 +808,57 @@ def _set_persistent(self, **kwargs): _config_module_deps: set[str] = set() +class _ImportRecorder: + """Meta-path finder that records import attempts on the recording thread. + + Never resolves anything; per-thread recording keeps concurrent imports by + other threads out of the rxconfig dep set (unlike a sys.modules diff). + Left in sys.meta_path permanently: removal shifts the list under other + threads' unlocked _find_spec iteration, which can skip a real finder, and + raises ValueError if rxconfig.py rebuilt sys.meta_path. + """ + + 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 threading.get_ident() == self._thread: + self.names.add(fullname) + + +_import_recorder = _ImportRecorder() + + 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 a leftover module from another + # project directory would fake the existence check below. + sys.modules.pop(constants.Config.MODULE, None) # only import the module if it exists. If a module spec exists then # the module exists. spec = find_spec(constants.Config.MODULE) @@ -821,21 +866,21 @@ 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) + # Reinstall if rxconfig.py rebuilt sys.meta_path on a previous load. + if _import_recorder not in sys.meta_path: + sys.meta_path.insert(0, _import_recorder) + _import_recorder.start() try: rxconfig = importlib.import_module(constants.Config.MODULE) finally: + _import_recorder.stop() # 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) + for name in _import_recorder.names: + origin = getattr(sys.modules.get(name), "__file__", None) if ( origin and (path := Path(origin)).is_relative_to(project_root) @@ -872,30 +917,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() -> Config: diff --git a/tests/units/test_config.py b/tests/units/test_config.py index 361a52dd537..6fc16ed3b8c 100644 --- a/tests/units/test_config.py +++ b/tests/units/test_config.py @@ -1,6 +1,7 @@ import logging import multiprocessing import os +import sys import threading import time from pathlib import Path @@ -890,3 +891,165 @@ def worker(i: int) -> None: assert load_count == 1 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. + sys.modules.pop("colorsys", None) + 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 + + +def test_concurrent_import_not_recorded_as_rxconfig_dep( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +): + """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. + """ + import textwrap + import types + + (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) + sys.modules.pop("side_module", None) + + 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 + sys.modules.pop("side_module", None) + sys.modules.pop("rxconfig", None) + reflex_base.config._config_module_deps.discard("rxconfig") + + +def test_load_config_survives_rxconfig_rebuilding_meta_path( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +): + """A load succeeds even if rxconfig.py rebuilds sys.meta_path. + + Cleanup used to call sys.meta_path.remove(recorder) unconditionally in a + finally block, so an rxconfig that rebound sys.meta_path turned a + successful load into a ValueError (or masked the real import error). The + recorder is now permanent and reinstalled on the next load. + + Args: + tmp_path: The pytest tmp_path fixture. + monkeypatch: The pytest monkeypatch fixture. + """ + import textwrap + + (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) + 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: + sys.modules.pop("rxconfig", None) + reflex_base.config._config_module_deps.discard("rxconfig") + if reflex_base.config._import_recorder not in sys.meta_path: + sys.meta_path.insert(0, reflex_base.config._import_recorder)