fix: don't hold the storage mapping cache guard across the database query - #740
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>
|
Thanks! |
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
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.