Serialize Dash._setup_server under a lock and publish its flag last - #3980
Open
mokashang wants to merge 1 commit into
Open
Serialize Dash._setup_server under a lock and publish its flag last#3980mokashang wants to merge 1 commit into
Dash._setup_server under a lock and publish its flag last#3980mokashang wants to merge 1 commit into
Conversation
`_setup_server` runs as a `before_request` hook and sets its `_got_first_request["setup_server"]` guard flag before performing the work that flag protects (populating `registered_paths` via `_generate_scripts_html`, `callback_map` via the `GLOBAL_CALLBACK_MAP` copy, and so on). On a multi-threaded WSGI worker such as `gunicorn -k gthread`, waitress, or `flask run --threaded`, a second request arriving in that gap sees the flag already set, skips setup, then reads `registered_paths` and validates against `callback_map` while both are still empty, so component-bundle requests 500 with `Error loading dependency. "<lib>" is not a registered library`. The setup body now runs under a per-instance `threading.Lock` with a double-checked read of the flag: a raced-in thread waits on the lock, then sees the flag set by whichever thread won and returns without redoing the work. The flag is only published once every side effect has been applied, so no other thread can observe it prematurely. Callers on the hot path after the first request pay no lock cost. Adds a regression test in `tests/unit/` that reproduces the race by slowing `_generate_scripts_html` and running several concurrent `_setup_server` calls; before the fix three of four threads returned with `registered_paths` still empty, after the fix all threads see it populated. Closes plotly#3971.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Contributor Checklist
optionals
CHANGELOG.mdReference issue
Closes #3971.
What does this implement/fix?
Dash._setup_serveris abefore_requesthook that is meant to execute once per process. It sets its_got_first_request["setup_server"]guard flag before performing the work that flag protects: populatingregistered_pathsvia_generate_scripts_html, copying theGLOBAL_CALLBACK_MAPentries intocallback_map, running background-callback validation, and so on. On a multi-threaded WSGI worker such asgunicorn -k gthread, waitress, orflask run --threaded, a second request can enter_setup_serverbetween the "set flag" and "do the work" statements, see the flag already set, skip setup, and then readregistered_pathsand validate againstcallback_mapwhile both are still empty. The visible symptom is the first burst of_dash-component-suites/<lib>/...bundle requests after a restart returning 500 withError loading dependency. "<lib>" is not a registered library. Registered libraries are: [].The fix:
threading.Locknext to_got_first_requestand wrap the setup body with double-checked locking: the fast-path early-return still fires without any lock cost once setup is done; a raced-in second thread waits on the lock, then sees the flag set by whichever thread won and returns without redoing the work._got_first_request["setup_server"] = Truepublication to the very end of the guarded block so no other thread can observe it while any side effect is still in flight.Additional information
Scope is deliberately narrow. The same TOCTOU pattern exists in the
pagesguards insiderouter_async/router_sync(if self._got_first_request["pages"]: returnfollowed byself._got_first_request["pages"] = True); those are left for a follow-up because the async version has anawait get_layouts()inside the section that needs guarding, which needsasyncio.Lockrather than the plainthreading.Lockused here.Regression test in
tests/unit/test_setup_server_race.pyslows_generate_scripts_htmlby 100ms and runs four concurrent_setup_servercalls behind athreading.Barrier. Before the fix, three of the four threads returned from_setup_serverwithapp.registered_pathsstill empty; with the fix all four see it populated.pylint tests/unit -d all -e C0410,C0413,W0109 --rcfile=.pylintrcandpylint dash --rcfile=.pylintrcboth stay clean, andblack dash tests --exclude 'metadata_test.py|node_modules' --checkpasses.