fix(state/in-memory): avoid deadlock between Close and cleanup goroutine - #4496
fix(state/in-memory): avoid deadlock between Close and cleanup goroutine#4496karprabha wants to merge 5 commits into
Conversation
Close called wg.Wait while holding the write lock, but the cleanup goroutine started by Init needs that same lock in doCleanExpiredItems. If the cleanup ticker fired between Close acquiring the lock and the goroutine reaching Lock, the two waited on each other and Close never returned. Wait for the cleanup goroutine to exit before acquiring the lock, and add a regression test that deadlocks reliably on the previous code. Introduced in d098e38 (dapr#2474). Signed-off-by: Prabhakar Yadav <developer.prabhakaryadav@gmail.com>
mikeee
left a comment
There was a problem hiding this comment.
Could you please consider including the proposed changes to startCleanThread in this PR to reduce the CI run load?
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4496 +/- ##
==========================================
+ Coverage 31.94% 32.14% +0.20%
==========================================
Files 353 353
Lines 47723 37916 -9807
==========================================
- Hits 15243 12188 -3055
+ Misses 31277 24524 -6753
- Partials 1203 1204 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… clock startCleanThread called time.After directly, so the cleanup tick ignored the clock injected into the store. Use store.clock.After and hoist the interval into a package constant shared with the regression test. Signed-off-by: Prabhakar Yadav <developer.prabhakaryadav@gmail.com>
|
Done, pushed. Separately, |
Now that the cleanup ticker runs on the injected clock, the failing interleaving can be produced directly instead of with scheduler pressure: park the cleanup goroutine on its tick, step the fake clock so it commits to the cleanup branch of its select, and call Close before it is scheduled again. One store and no sleeps instead of 2000 stores and a 1s wait. Fails on the first attempt in 3/3 runs with the fix reverted, passes with it. Signed-off-by: Prabhakar Yadav <developer.prabhakaryadav@gmail.com>
|
Hi @mikeee I've made the said changes Kindly review it again |
Description
Close()callswg.Wait()while holding the write lock. The cleanup goroutinestarted by
Init()needs that same lock indoCleanExpiredItems(). If thecleanup ticker fires in the window between
Close()acquiring the lock and thegoroutine reaching
Lock(), the two wait on each other andClose()neverreturns.
This is a permanent hang — no timeout, no error, no retry. Under Kubernetes it
surfaces as a sidecar stuck terminating until SIGKILL, with nothing in the logs.
Fix: wait for the cleanup goroutine to exit before acquiring the lock.
Once
wg.Wait()returns, nothing can contend for it.Introduced in d098e38 (#2474, Feb 2023).
Reproduction
The window is microseconds wide, so it reproduces under scheduling pressure
rather than precise timing: many stores closed concurrently, timed to coincide
with their cleanup tick. It is not single-core specific — measured on a 10-core
machine, 6 rounds per configuration:
Goroutine dump at the deadlock:
Cleanup ticker now uses the injected clock
startCleanThreadusedtime.Afterdirectly, so the cleanup tick ignored theclock injected into the store. It now uses
store.clock.After, with the intervalhoisted into a package constant shared with the test.
This means a fake clock now governs the sweeper as well -
fakeClock.StepinTestReadAndWritedrives a sweep, so tests get full control of the component'stime rather than a background goroutine ticking on real time underneath them.
Existing tests pass unchanged.
Issue reference
Closes #4495
Checklist
No documentation changes are required: this is an internal concurrency fix with
no change to component behavior, configuration metadata, or public API.