diff --git a/CHANGELOG.md b/CHANGELOG.md index 51e3212852..3ec1b0662c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ This project adheres to [Semantic Versioning](https://semver.org/). - [#3646](https://github.com/plotly/dash/pull/3646) Remove React 16 support (`16.14.0` is no longer an accepted value for `REACT_VERSION` / `_set_react_version`). ### Fixed +- Fix `Dash._setup_server` publishing its "already done" guard flag before the setup work behind it had run. Under a multi-threaded WSGI worker such as `gunicorn -k gthread` a second request arriving mid-setup could observe the flag already set, skip setup, then read `registered_paths` / `callback_map` while they were still empty, causing the first burst of component bundle requests after a restart to 500 with `Error loading dependency. "" is not a registered library`. The setup body now runs under a lock and only publishes the flag after all work completes. Fixes [#3971](https://github.com/plotly/dash/issues/3971). - [#3944](https://github.com/plotly/dash/pull/3944) Fix `dash.testing` runner backend detection for wrapped FastAPI/Quart servers so threaded Flask-only options are not passed to ASGI runners. - [#3955](https://github.com/plotly/dash/pull/3955) Unpin `selenium` in the testing requirements (was capped at `<=4.2.0`, from 2022) and require `>=4.11.0`. The old cap predated Selenium Manager, so the pinned selenium could not drive the current stable Chrome (151+) that CI installs, producing widespread `StaleElementReferenceException`/`TimeoutException` flakiness across the browser-based integration tests. Modern selenium auto-provisions a matching chromedriver, restoring stable CI runs. - Speed up the layout crawl the renderer runs on every path recompute and callback gather (`crawlLayout` and its callers) by replacing curried-ramda `path`/`pathOr` lookups with direct property access on the per-node hot path. Cut a `Patch().append()` into a large container by ~16% and initial render / wildcard resolution by a few percent, with no behavior change. diff --git a/dash/dash.py b/dash/dash.py index 134e92107f..ae5ee0395b 100644 --- a/dash/dash.py +++ b/dash/dash.py @@ -684,6 +684,10 @@ def __init__( # pylint: disable=too-many-statements, too-many-branches # tracks internally if a function already handled at least one request. self._got_first_request = {"pages": False, "setup_server": False} + # Serializes _setup_server so a concurrent worker thread cannot + # observe the guard flag while the setup work behind it is still + # in flight (see gh-3971). + self._setup_server_lock = threading.Lock() # Secret used to sign background-callback handles (see _callback_signing). # Prefer the Flask/Quart secret_key (shared across workers when the @@ -1703,88 +1707,104 @@ def _setup_server(self): if self._got_first_request["setup_server"]: return - self._got_first_request["setup_server"] = True - - # Apply _force_eager_loading overrides from modules - eager_loading = self.config.eager_loading - for module_name in ComponentRegistry.registry: - module = sys.modules[module_name] - eager = getattr(module, "_force_eager_loading", False) - eager_loading = eager_loading or eager - - # Update eager_loading settings - self.scripts.config.eager_loading = eager_loading - - if self.config.include_assets_files: - self._walk_assets_directory() - - if not self.layout and self.use_pages: - self.layout = page_container + # Double-checked locking: previously the guard flag was set before the + # work it protects, which let a second thread (e.g. under gunicorn + # ``-k gthread``) skip setup while ``registered_paths`` and + # ``callback_map`` were still being populated by the first thread and + # then fail validation on component bundle requests. See gh-3971. + with self._setup_server_lock: + if self._got_first_request["setup_server"]: + return - _validate.validate_layout(self.layout, self._layout_value()) + # Apply _force_eager_loading overrides from modules + eager_loading = self.config.eager_loading + for module_name in ComponentRegistry.registry: + module = sys.modules[module_name] + eager = getattr(module, "_force_eager_loading", False) + eager_loading = eager_loading or eager - self._generate_scripts_html() - self._generate_css_dist_html() + # Update eager_loading settings + self.scripts.config.eager_loading = eager_loading - # Copy over global callback data structures assigned with `dash.callback` - for k in list(_callback.GLOBAL_CALLBACK_MAP): - if k in self.callback_map: - raise DuplicateCallback( - f"The callback `{k}` provided with `dash.callback` was already " - "assigned with `app.callback`." - ) + if self.config.include_assets_files: + self._walk_assets_directory() - self.callback_map[k] = _callback.GLOBAL_CALLBACK_MAP.pop(k) + if not self.layout and self.use_pages: + self.layout = page_container - self._callback_list.extend(_callback.GLOBAL_CALLBACK_LIST) + _validate.validate_layout(self.layout, self._layout_value()) - # For each callback function, if the hidden parameter uses the default value None, - # replace it with the actual value of the self.config.hide_all_callbacks. - self._callback_list = [ - ( - {**_callback, "hidden": self.config.get("hide_all_callbacks", False)} - if _callback.get("hidden") is None - else _callback - ) - for _callback in self._callback_list - ] + self._generate_scripts_html() + self._generate_css_dist_html() - _callback.GLOBAL_CALLBACK_LIST.clear() + # Copy over global callback data structures assigned with `dash.callback` + for k in list(_callback.GLOBAL_CALLBACK_MAP): + if k in self.callback_map: + raise DuplicateCallback( + f"The callback `{k}` provided with `dash.callback` was already " + "assigned with `app.callback`." + ) - _validate.validate_background_callbacks(self.callback_map) + self.callback_map[k] = _callback.GLOBAL_CALLBACK_MAP.pop(k) - cancels = {} + self._callback_list.extend(_callback.GLOBAL_CALLBACK_LIST) - for callback in self.callback_map.values(): - background = callback.get("background") - if not background: - continue - if "cancel_inputs" in background: - cancel = background.pop("cancel_inputs") - for c in cancel: - cancels[c] = background.get("manager") - - if cancels: - for cancel_input, manager in cancels.items(): - # pylint: disable=cell-var-from-loop - @self.callback( - Output(cancel_input.component_id, "id"), - cancel_input, - prevent_initial_call=True, - manager=manager, + # For each callback function, if the hidden parameter uses the default value None, + # replace it with the actual value of the self.config.hide_all_callbacks. + self._callback_list = [ + ( + { + **_callback, + "hidden": self.config.get("hide_all_callbacks", False), + } + if _callback.get("hidden") is None + else _callback ) - def cancel_call(*_): - job_ids = callback_context.args.getlist("cancelJob") - executor = _callback.context_value.get().background_callback_manager - if job_ids: - secret = self._get_signing_secret() - end_id = _callback.get_request_end_id(secret) - scope = _callback_signing.job_scope(end_id) - for job_id in job_ids: - job = _callback_signing.unsign(secret, scope, job_id) - if job is not None: - executor.terminate_job(job) - return no_update + for _callback in self._callback_list + ] + + _callback.GLOBAL_CALLBACK_LIST.clear() + + _validate.validate_background_callbacks(self.callback_map) + + cancels = {} + + for callback in self.callback_map.values(): + background = callback.get("background") + if not background: + continue + if "cancel_inputs" in background: + cancel = background.pop("cancel_inputs") + for c in cancel: + cancels[c] = background.get("manager") + + if cancels: + for cancel_input, manager in cancels.items(): + # pylint: disable=cell-var-from-loop + @self.callback( + Output(cancel_input.component_id, "id"), + cancel_input, + prevent_initial_call=True, + manager=manager, + ) + def cancel_call(*_): + job_ids = callback_context.args.getlist("cancelJob") + executor = ( + _callback.context_value.get().background_callback_manager + ) + if job_ids: + secret = self._get_signing_secret() + end_id = _callback.get_request_end_id(secret) + scope = _callback_signing.job_scope(end_id) + for job_id in job_ids: + job = _callback_signing.unsign(secret, scope, job_id) + if job is not None: + executor.terminate_job(job) + return no_update + + # Publish the flag last, so a raced-in thread cannot see it set + # while the setup work above is still in flight. + self._got_first_request["setup_server"] = True def _add_assets_resource(self, url_path, file_path): res = {"asset_path": url_path, "filepath": file_path} diff --git a/tests/unit/test_setup_server_race.py b/tests/unit/test_setup_server_race.py new file mode 100644 index 0000000000..997b0b7253 --- /dev/null +++ b/tests/unit/test_setup_server_race.py @@ -0,0 +1,61 @@ +"""Regression test for gh-3971: ``_setup_server`` TOCTOU race. + +Before the fix, ``Dash._setup_server`` set its guard flag before doing the +work that flag protects (populating ``registered_paths``, ``callback_map``, +etc.). Under a multi-threaded WSGI worker such as ``gunicorn -k gthread``, +a second thread arriving mid-setup could observe the flag already set, skip +setup, and then read ``registered_paths`` while it was still empty, which +caused component-bundle requests to 500 with "Error loading dependency." + +The test simulates a concurrent second request by slowing down one of the +inner setup steps and having several threads call ``_setup_server`` at the +same time. After every thread returns, ``registered_paths`` must be +populated, because a return from ``_setup_server`` is meant to guarantee +the setup work is done. +""" +import threading +import time + +from dash import Dash, html + + +def test_setup_server_is_atomic_under_concurrent_requests(): + app = Dash() + app.layout = html.Div(id="root") + + original_generate_scripts_html = app._generate_scripts_html + + def slow_generate_scripts_html(): + # Widen the TOCTOU window so a stock CPython scheduler reliably lets + # other threads reach the guard while this one is still running. + time.sleep(0.1) + return original_generate_scripts_html() + + app._generate_scripts_html = slow_generate_scripts_html + + thread_count = 4 + barrier = threading.Barrier(thread_count) + paths_seen_after_setup = [] + lock = threading.Lock() + + def worker(): + barrier.wait() + app._setup_server() + with lock: + paths_seen_after_setup.append(set(app.registered_paths)) + + threads = [threading.Thread(target=worker) for _ in range(thread_count)] + for t in threads: + t.start() + for t in threads: + t.join() + + assert len(paths_seen_after_setup) == thread_count + for paths in paths_seen_after_setup: + # Every thread that received control back from _setup_server must + # observe registered_paths already populated by the winning thread. + # Before the fix, threads that raced past the guard saw an empty set. + assert paths, ( + "A thread returned from _setup_server before the setup work " + "was done; registered_paths was still empty." + )