fix: don't hold the storage mapping cache guard across the database query - #740
Open
dispather wants to merge 1 commit into
Open
fix: don't hold the storage mapping cache guard across the database query#740dispather wants to merge 1 commit into
dispather wants to merge 1 commit into
Conversation
…uery `DashMap::get` hands out a read guard on the map shard, and taking the write lock for `insert` parks the calling thread rather than just the task. Holding that guard across the storage mapping query therefore blocks any worker that inserts into the same shard while the query is in flight; once that starves the runtime, nothing polls the reactor, the query never completes and the guard is never released, so the process stops serving until it is restarted. The stale-refresh and cache-miss branches had identical tails, so they collapse into a single path once the guard is dropped before the query. Signed-off-by: dispather <62810211+dispather@users.noreply.github.com>
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.
What
get_storage_mappingholds aDashMapshard read guard across the.awaitof the storage-mapping database query. This drops the guard before the query instead.Fixes #739
Why
dashmap::mapref::one::Refis a read guard on the map shard. Taking the write lock forinsertblocks the calling thread, not just the task —dashmap6.x builds itsRawRwLockonlock_api+parking_lot_core, so a waiting writer parks the OS thread.So while
load_storage_mappingis in flight:self.cache.insert(...)for the same shard parks a Tokio worker thread.On my instance this reproduces every 15 hours to 10 days: the process stays alive at 0% CPU with flat memory, both the main HTTP port and
--metrics-portstop responding at the same instant, and no further log line is written. Across five freeze captures, zero threads were inepoll_waitwhile every thread sat infutex_wait; a healthy process of the same build has exactly one thread inepoll_wait. Full write-up in the linked issue.This was introduced in 1.3.3 by the
updatingflag refactor (avoid too many concurrent db queries when cache becomes invalid). 1.3.2 did not hold the guard across the await —Option::filterconsumed and dropped theRefbefore the query. 1.3.4, 1.3.5 and currentmasterare unchanged.How
The stale-refresh branch and the cache-miss branch had identical tails, so they collapse into one path once the guard is released early:
updatingflag;No behaviour change other than the lock hold time: a valid entry is still returned under the read guard without querying, a stale entry still serves through
updating, and a failed query still clears the flag.Testing
cargo check --all-targets,cargo clippy --all-targets,cargo test --lib(2 passed) andcargo buildall pass on rustc 1.94.0.Since both shapes compile, I checked separately that the change actually shortens the lock hold. Reducing each shape to a
DashMapplus a sleep, and timing aninserton the same key issued while the future is suspended:and that the original shape deadlocks rather than just stalling, once the runtime's workers are occupied by same-shard writers — the task that would wake the guard holder never gets scheduled:
I did not add this as a regression test in the PR: the reduction is faithful to the locking shape but not to
get_storage_mappingitself, and testing the real function needs a controlled stall insidefetch_allplus a shard collision, which I couldn't make non-flaky. Happy to add either version if you'd like one.I have not been able to prove which lock the threads park on:
wchanonly says "in a futex", and unwinding the release binary from outside its container fails (static-PIE, no frame pointers —eu-stackandgdb --sysrootboth stop at the syscall stub). I've instrumented my instance to dump/proc/<pid>/task/*/syscallat the next freeze and cluster the futex addresses; shared addresses would confirm lock contention, all-distinct addresses would mean the cause is elsewhere. I'll report back either way.