fix: acquire GIL before AppState write lock to prevent deadlock - #227
Merged
Merged
Conversation
…lock ensure_compiled_snapshot, setup_database, and close_database took the AppState write lock and then called rebuild_snapshot(), which acquires the GIL to clone Py<PyAny> handles. When those ran on a tokio worker thread without the GIL already held, this created an AB-BA lock-order inversion against handle_rsgi (GIL held, then state.read()/write()): one thread could hold the Rust lock waiting for the GIL while another held the GIL waiting for the Rust lock, deadlocking the worker. Acquire the GIL first in all three call sites, then take the write lock inside that scope, matching the GIL-then-lock order used everywhere else in the hot path. Closes #205
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
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.
Problem
ensure_compiled_snapshot(dispatch.rs),setup_database, andclose_database(lib.rs) took theAppStatewrite lock and then calledrebuild_snapshot(), which acquires the GIL internally to clonePy<PyAny>handles. These run on tokio worker threads without the GIL already held (asyncrun_rsgipath, or a spawned future), so the lock order there was Rust lock → GIL.Elsewhere (e.g.
handle_rsgi), the order is GIL → Rust lock (GIL held for the whole pymethod call, thenstate.read()/state.write()).Two different lock orders on the same pair of locks is a textbook AB-BA deadlock: one thread can hold the Rust write lock while blocked acquiring the GIL, while another thread holds the GIL and is blocked acquiring the Rust lock. Under real concurrency (e.g. no explicit
freeze()+ any middleware, which skips the sync short-circuit) this hangs the worker.Fix
Acquire the GIL first in all three call sites, then take the write lock inside that scope — matching the GIL-then-lock order used everywhere else in the hot path.
Python::with_gilis reentrant, so this is a no-op cost when GIL is already held (e.g. pymethods).Testing
cargo build --libpasses. No behavior change to the lock-free read path.Closes #205