Fix #188: avoid per-key stalls on concurrent fetch and invalidate (Fix #180 flaky too) - #205
Merged
Merged
Conversation
Under a storm of concurrent fetch and invalidate operations on a single hot key, the coordinator held a per-key EXCLUSIVE write lock for the whole network round-trip, so every operation on that key serialized (one RTT each) and the per-key queue could not drain, making that key unresponsive while the rest of the cache was fine. Two server-side changes, protocol- and backward-compatible: - Read/write lock split: fetchEntry now takes a SHARED read lock instead of the exclusive write lock. Concurrent fetches on the same key no longer serialize, while staying mutually exclusive with invalidate/put/load, so the ordering invariant from #170 (a fetch's client-registration must not race an invalidate) is preserved. CacheStatus is already internally thread-safe, so fetch-vs-fetch exclusion was unnecessary. - Invalidation coalescing: concurrent invalidations of the same key attach to an in-flight one instead of queueing behind the write lock for another full broadcast round-trip. This is safe because the in-flight invalidation holds the write lock during the broadcast, which is mutually exclusive with fetches, so no client can register for the key between the interested- clients snapshot and broadcast completion. Tests: - FetchAndInvalidateStormTest reproduces #188: before the fix 50 operations timed out (peak 8.8s); after the fix 0 timeouts (peak ~0.4s). - WriterStarvationTest guards against writer starvation caused by the read/write split (invalidate stays bounded under heavy concurrent fetch). - FetchAndInvalidateHammerTest (#170 correctness) stays green.
Server operations acquire a per-key lock and release it only from the (possibly asynchronous) completion callback. If the action body threw before wiring that callback, the lock was leaked and the key became permanently stuck (all later operations on it would block). Guard finishAndReleaseLock with an AtomicBoolean so the lock is released exactly once (a double StampedLock unlock would throw) and wrap each body in a try/catch that releases the lock and reports the error to the caller. Covers putEntry, loadEntry, invalidateKey (including draining the coalesced group), invalidateKeyStandalone and fetchEntry; unregisterEntries already used try/finally.
Complete the release-on-error hardening for the two operations that were skipped earlier: lockKey now releases the acquired write lock (and clears its cacheStatus tracking) if registering the lock or sending the reply throws, so a failure cannot leave the key write-locked forever; unlockKey now reports an error instead of leaving the caller hanging when the lock id is malformed or the stamp is stale/wrong. KeyedLockManager parses a client-provided lock id defensively: a non-numeric value yields an invalid lock (null), which callers already report as "invalid clientProvidedLockId", instead of throwing NumberFormatException out of the handler action (which would never invoke onFinish).
The test simulated a server-side connection error on client2 and then asserted client2 still held the cached value. That assertion raced client2's own reconnect/emptyCache (a disconnected client empties its cache on channelClosed), so the test was flaky in dev and CI. Assert the deterministic outcome instead: client1's invalidate completes (the server declares the errored client dead) and, because the server-side error drops client2's connection, client2 empties its cache and stops serving the stale entry (polled with a timeout). Add a @test timeout as an anti-hang guard and lower the slow-client timeout to keep the test fast.
diegosalvi
commented
Jul 3, 2026
| public class ApparentlyStuckClientDueToServerSideErrorTest { | ||
|
|
||
| @Test | ||
| @Test(timeout = 60000) |
Contributor
Author
There was a problem hiding this comment.
This was a flaky test
diegosalvi
changed the title
dmercuriali
approved these changes
Jul 6, 2026
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
2 participants
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 (#188)
Under a storm of concurrent fetch and invalidate operations on a single hot key, the coordinator became unresponsive on that key (the rest of the cache stayed fine). Clients timed out waiting; the server showed pending broadcast invalidations piling up.
Root cause
CacheServer.fetchEntry / invalidateKey / putEntry / loadEntry all acquired a per-key exclusive write lock (KeyedLockManager, StampedLock.writeLock()) and held it for the entire network round-trip (released only in the async reply callback). So every operation on a key serialized, one RTT each, and under high traffic the per-key queue could not drain. CacheStatus is already internally thread-safe, so fetch-vs-fetch exclusion was unnecessary — the lock only needsto order a fetch's client-registration against an invalidate (the invariant from #170).
Fix (server-side, protocol- and backward-compatible)
Verification
Flaky (Fixes #180 too)
now asserts the deterministic outcome (invalidate completes and the disconnected client empties its cache), with a @test timeout as an anti-hang guard.
Following this checklist to help us incorporate your
contribution quickly and easily:
mvn clean verifyto make sure basic checks pass. A more thorough check willbe performed on your pull request automatically.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.