diff --git a/CHANGELOG.md b/CHANGELOG.md index 31efd91573..7ca0a44f81 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ This project adheres to [Semantic Versioning](https://semver.org/). - [#3986](https://github.com/plotly/dash/pull/3986) Adjust `_run_before_hooks` in the `fastapi` backend to honor a response returned by a `before_request` function, matching the `flask` backend's behavior. ### Fixed +- [#3980](https://github.com/plotly/dash/pull/3980) Fix the three `before_request` hooks (`Dash._setup_server` and the pages `router_sync` / `router_async`) publishing their "already done" guard flag before the setup work behind it had run. Under a multi-threaded WSGI worker such as `gunicorn -k gthread` (or under an ASGI worker for the async router), a second request arriving mid-setup could observe the flag already set, skip setup, then read `registered_paths` / `callback_map` / the pages router callback while they were still being registered - causing the first burst of component bundle requests after a restart to 500 with `Error loading dependency. "" is not a registered library`, or the pages router to hit `DuplicateCallback` when two workers raced past the guard. Each hook body now runs under a lock (`threading.Lock` for the two sync hooks, an `asyncio.Lock` bound to the running loop for the async router) 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`, so it can drive current stable Chrome via Selenium Manager and stop the widespread CI flakiness. - Speed up the renderer layout crawl (`crawlLayout` and its callers) by replacing curried-ramda `path`/`pathOr` lookups with direct property access on the hot path: ~16% faster `Patch().append()` into a large container, with no behavior change. diff --git a/dash/dash.py b/dash/dash.py index ed1ed5ebc8..2fb1b58a27 100644 --- a/dash/dash.py +++ b/dash/dash.py @@ -1,3 +1,4 @@ +import asyncio import functools import os import sys @@ -732,6 +733,15 @@ 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} + # Serialize the before_request hooks so a raced-in worker cannot see + # the guard flag while the work behind it is still in flight (gh-3971). + # The async lock is created inside router_async so it binds to the + # running event loop; on 3.9 asyncio.Lock captures the loop at + # construction, which for a Dash instance built in module scope is + # the wrong one. + self._setup_server_lock = threading.Lock() + self._pages_lock = threading.Lock() + self._pages_async_lock: Optional[asyncio.Lock] = None # Secret used to sign background-callback handles (see _callback_signing). # Prefer the Flask/Quart secret_key (shared across workers when the @@ -1801,88 +1811,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} @@ -2735,153 +2761,181 @@ def enable_pages(self) -> None: async def router_async(): if self._got_first_request["pages"]: return - self._got_first_request["pages"] = True - inputs = { - "pathname_": Input(_ID_LOCATION, "pathname"), - "search_": Input(_ID_LOCATION, "search"), - } - inputs.update(self.routing_callback_inputs) - - @self.callback( - Output(_ID_CONTENT, "children"), - Output(_ID_STORE, "data"), - inputs=inputs, - prevent_initial_call=True, - hidden=True, - ) - async def update(pathname_, search_, **states): - query_parameters = _parse_query_string(search_) - page, path_variables = _path_to_page( - self.strip_relative_path(pathname_) + # Lazily create the lock so it binds to the running event loop + # (see __init__). Check-and-assign is safe here: asyncio only + # yields at await points, so no other task can race between the + # two lines. + if self._pages_async_lock is None: + self._pages_async_lock = asyncio.Lock() + + # Double-checked locking, same rationale as _setup_server + # (gh-3971). Without it a raced-in task can either serve requests + # against an unregistered _ID_CONTENT callback or hit + # DuplicateCallback registering the router callback twice. + async with self._pages_async_lock: + if self._got_first_request["pages"]: + return + + inputs = { + "pathname_": Input(_ID_LOCATION, "pathname"), + "search_": Input(_ID_LOCATION, "search"), + } + inputs.update(self.routing_callback_inputs) + + @self.callback( + Output(_ID_CONTENT, "children"), + Output(_ID_STORE, "data"), + inputs=inputs, + prevent_initial_call=True, + hidden=True, ) - if page == {}: - for module, page in _pages.PAGE_REGISTRY.items(): - if module.split(".")[-1] == "not_found_404": - layout = page["layout"] - title = page["title"] - break + async def update(pathname_, search_, **states): + query_parameters = _parse_query_string(search_) + page, path_variables = _path_to_page( + self.strip_relative_path(pathname_) + ) + if page == {}: + for module, page in _pages.PAGE_REGISTRY.items(): + if module.split(".")[-1] == "not_found_404": + layout = page["layout"] + title = page["title"] + break + else: + layout = html.H1("404 - Page not found") + title = self.title else: - layout = html.H1("404 - Page not found") - title = self.title - else: - layout = page.get("layout", "") - title = page["title"] + layout = page.get("layout", "") + title = page["title"] - if callable(layout): - layout = await execute_async_function( - layout, - **{**(path_variables or {}), **query_parameters, **states}, - ) - if callable(title): - title = await execute_async_function( - title, **{**(path_variables or {})} - ) - return layout, {"title": title} + if callable(layout): + layout = await execute_async_function( + layout, + **{**(path_variables or {}), **query_parameters, **states}, + ) + if callable(title): + title = await execute_async_function( + title, **{**(path_variables or {})} + ) + return layout, {"title": title} - _validate.check_for_duplicate_pathnames(_pages.PAGE_REGISTRY) - _validate.validate_registry(_pages.PAGE_REGISTRY) + _validate.check_for_duplicate_pathnames(_pages.PAGE_REGISTRY) + _validate.validate_registry(_pages.PAGE_REGISTRY) - if not self.config.suppress_callback_exceptions: + if not self.config.suppress_callback_exceptions: - async def get_layouts(): - return [ - await execute_async_function(page["layout"]) - if callable(page["layout"]) - else page["layout"] - for page in _pages.PAGE_REGISTRY.values() - ] + async def get_layouts(): + return [ + await execute_async_function(page["layout"]) + if callable(page["layout"]) + else page["layout"] + for page in _pages.PAGE_REGISTRY.values() + ] - layouts = await get_layouts() - # pylint: disable=not-callable - layouts += [self.layout() if callable(self.layout) else self.layout] - self.validation_layout = html.Div(layouts) - if _ID_CONTENT not in self.validation_layout: - raise Exception("`dash.page_container` not found in the layout") - - self.clientside_callback( - """ - function(data) { - document.title = data.title - } - """, - Output(_ID_DUMMY, "children"), - Input(_ID_STORE, "data"), - hidden=True, - ) + layouts = await get_layouts() + # pylint: disable=not-callable + layouts += [self.layout() if callable(self.layout) else self.layout] + self.validation_layout = html.Div(layouts) + if _ID_CONTENT not in self.validation_layout: + raise Exception("`dash.page_container` not found in the layout") + + self.clientside_callback( + """ + function(data) { + document.title = data.title + } + """, + Output(_ID_DUMMY, "children"), + Input(_ID_STORE, "data"), + hidden=True, + ) + + # Publish the flag last so a raced-in task cannot observe it + # set while the callback registration above is still pending. + self._got_first_request["pages"] = True # Sync version def router_sync(): if self._got_first_request["pages"]: return - self._got_first_request["pages"] = True - inputs = { - "pathname_": Input(_ID_LOCATION, "pathname"), - "search_": Input(_ID_LOCATION, "search"), - } - inputs.update(self.routing_callback_inputs) - - @self.callback( - Output(_ID_CONTENT, "children"), - Output(_ID_STORE, "data"), - inputs=inputs, - prevent_initial_call=True, - hidden=True, - ) - def update(pathname_, search_, **states): - query_parameters = _parse_query_string(search_) - page, path_variables = _path_to_page( - self.strip_relative_path(pathname_) + # Double-checked locking, same rationale as router_async and + # _setup_server (gh-3971). + with self._pages_lock: + if self._got_first_request["pages"]: + return + + inputs = { + "pathname_": Input(_ID_LOCATION, "pathname"), + "search_": Input(_ID_LOCATION, "search"), + } + inputs.update(self.routing_callback_inputs) + + @self.callback( + Output(_ID_CONTENT, "children"), + Output(_ID_STORE, "data"), + inputs=inputs, + prevent_initial_call=True, + hidden=True, ) - if page == {}: - for module, page in _pages.PAGE_REGISTRY.items(): - if module.split(".")[-1] == "not_found_404": - layout = page["layout"] - title = page["title"] - break + def update(pathname_, search_, **states): + query_parameters = _parse_query_string(search_) + page, path_variables = _path_to_page( + self.strip_relative_path(pathname_) + ) + if page == {}: + for module, page in _pages.PAGE_REGISTRY.items(): + if module.split(".")[-1] == "not_found_404": + layout = page["layout"] + title = page["title"] + break + else: + layout = html.H1("404 - Page not found") + title = self.title else: - layout = html.H1("404 - Page not found") - title = self.title - else: - layout = page.get("layout", "") - title = page["title"] + layout = page.get("layout", "") + title = page["title"] - if callable(layout): - layout = layout( - **{**(path_variables or {}), **query_parameters, **states} + if callable(layout): + layout = layout( + **{**(path_variables or {}), **query_parameters, **states} + ) + if callable(title): + title = title(**(path_variables or {})) + return layout, {"title": title} + + _validate.check_for_duplicate_pathnames(_pages.PAGE_REGISTRY) + _validate.validate_registry(_pages.PAGE_REGISTRY) + + if not self.config.suppress_callback_exceptions: + layout = self.layout + if not isinstance(layout, list): + # pylint: disable=not-callable + layout = [self.layout() if callable(self.layout) else self.layout] + self.validation_layout = html.Div( + [ + page["layout"]() if callable(page["layout"]) else page["layout"] + for page in _pages.PAGE_REGISTRY.values() + ] + + layout ) - if callable(title): - title = title(**(path_variables or {})) - return layout, {"title": title} - - _validate.check_for_duplicate_pathnames(_pages.PAGE_REGISTRY) - _validate.validate_registry(_pages.PAGE_REGISTRY) + if _ID_CONTENT not in self.validation_layout: + raise Exception("`dash.page_container` not found in the layout") - if not self.config.suppress_callback_exceptions: - layout = self.layout - if not isinstance(layout, list): - # pylint: disable=not-callable - layout = [self.layout() if callable(self.layout) else self.layout] - self.validation_layout = html.Div( - [ - page["layout"]() if callable(page["layout"]) else page["layout"] - for page in _pages.PAGE_REGISTRY.values() - ] - + layout + self.clientside_callback( + """ + function(data) { + document.title = data.title + } + """, + Output(_ID_DUMMY, "children"), + Input(_ID_STORE, "data"), ) - if _ID_CONTENT not in self.validation_layout: - raise Exception("`dash.page_container` not found in the layout") - self.clientside_callback( - """ - function(data) { - document.title = data.title - } - """, - Output(_ID_DUMMY, "children"), - Input(_ID_STORE, "data"), - ) + # Publish the flag last so a raced-in thread cannot observe + # it set while the callback registration above is still + # pending. + self._got_first_request["pages"] = True if self._use_async: self.backend.before_request(router_async) diff --git a/tests/unit/test_pages_router_race.py b/tests/unit/test_pages_router_race.py new file mode 100644 index 0000000000..6c0cfaaeca --- /dev/null +++ b/tests/unit/test_pages_router_race.py @@ -0,0 +1,159 @@ +"""Regression test for gh-3971: pages ``router_sync`` / ``router_async`` TOCTOU race. + +Both hooks used to set ``_got_first_request["pages"]`` at their top and only +then register the ``_ID_CONTENT`` router callback and build +``validation_layout``. Under a multi-threaded WSGI worker (for the sync +hook) or an ASGI worker with multiple concurrent tasks (for the async +one), a second request arriving mid-setup could observe the flag already +set, skip setup, then serve a request against a callback that was not +yet registered. + +The tests here reproduce the window by pausing setup inside the guarded +region and driving the hook concurrently. The state observed at the +moment each caller returns must reflect a fully completed setup. +""" + +import asyncio +import threading +import time + +import dash +from dash import Dash, dependencies, html + +ROUTER_OUTPUT_ID = "_pages_content" + + +def _grab_before_request(app): + """Capture the ``before_request`` closure that ``enable_pages`` installs.""" + captured = {} + original = app.backend.before_request + + def capture(fn): + captured["fn"] = fn + + app.backend.before_request = capture # type: ignore[method-assign] + try: + app.enable_pages() + finally: + app.backend.before_request = original # type: ignore[method-assign] + return captured["fn"] + + +def test_router_sync_is_atomic_under_concurrent_requests(clear_pages_state): + app = Dash(use_pages=True, pages_folder="") + dash.register_page("home_page", path="/", layout=html.Div("home")) + app.layout = html.Div([dash.page_container]) + app._use_async = False + + original_input_init = dependencies.Input.__init__ + slowed = {"first": True} + slow_lock = threading.Lock() + + def slow_input_init(self, component_id, component_property): + # Widen the window between "flag is set" and "router callback is + # registered" so a stock CPython scheduler reliably lets other + # threads through the pages guard while the winning thread is + # still building its Input arguments. + with slow_lock: + need_sleep = slowed["first"] + slowed["first"] = False + if need_sleep: + time.sleep(0.1) + original_input_init(self, component_id, component_property) + + dependencies.Input.__init__ = slow_input_init # type: ignore[assignment] + try: + router_sync = _grab_before_request(app) + + thread_count = 4 + barrier = threading.Barrier(thread_count) + callback_maps_seen = [] + errors = [] + lock = threading.Lock() + + def worker(): + barrier.wait() + try: + router_sync() + except Exception as e: # noqa: BLE001 + with lock: + errors.append(e) + return + # Snapshot ``callback_map`` at the moment this thread's call + # returns. A raced-in thread that skipped setup on the old + # (buggy) code path would return here with the map empty of + # the router callback. + with lock: + callback_maps_seen.append(dict(app.callback_map)) + + threads = [threading.Thread(target=worker) for _ in range(thread_count)] + for t in threads: + t.start() + for t in threads: + t.join() + finally: + dependencies.Input.__init__ = original_input_init # type: ignore[assignment] + + assert not errors, f"router_sync raised under concurrent callers: {errors}" + assert len(callback_maps_seen) == thread_count + for cm in callback_maps_seen: + router_entries = [k for k in cm if ROUTER_OUTPUT_ID in k] + assert router_entries, ( + "A thread returned from router_sync before the router callback " + "was registered; callback_map was still missing the entry. " + "(Before the fix, threads that raced past the guard would see " + "the flag set but no callback yet.)" + ) + winning_map = callback_maps_seen[-1] + router_entries = [k for k in winning_map if ROUTER_OUTPUT_ID in k] + assert len(router_entries) == 1 + + +def test_router_async_is_atomic_under_concurrent_tasks(clear_pages_state): + async def slow_home_layout(): + # Force an await inside the guarded region (via ``get_layouts()``) + # so a concurrent task can slip past the pages guard while this + # one is still building ``validation_layout``. + await asyncio.sleep(0.05) + return html.Div("home") + + app = Dash(use_pages=True, pages_folder="") + dash.register_page("home_page", path="/", layout=slow_home_layout) + app.layout = html.Div([dash.page_container]) + app._use_async = True + + router_async = _grab_before_request(app) + + async def wrapped(): + await router_async() + # Snapshot the state at the moment this task's call returns. + # Before the fix, a task that raced past the guard would return + # here with ``validation_layout`` still unset. + return ( + [k for k in app.callback_map if ROUTER_OUTPUT_ID in k], + getattr(app, "validation_layout", None), + ) + + async def run_two_racing_tasks(): + return await asyncio.gather(wrapped(), wrapped(), return_exceptions=True) + + results = asyncio.run(run_two_racing_tasks()) + + for r in results: + assert not isinstance( + r, Exception + ), f"router_async raised under concurrent callers: {r!r}" + + for router_entries, vlayout in results: + assert router_entries, ( + "A task returned from router_async before the router callback " + "was registered." + ) + assert vlayout is not None, ( + "A task returned from router_async before validation_layout " + "was built. Before the fix, a raced-in task would return here " + "while the winning task was still awaiting ``get_layouts()``." + ) + + router_entries = [k for k in app.callback_map if ROUTER_OUTPUT_ID in k] + assert len(router_entries) == 1 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." + )