From f09dae06822ccf688829f30e2804627945bd6526 Mon Sep 17 00:00:00 2001 From: Ben Manes Date: Tue, 24 May 2022 17:15:36 -0700 Subject: [PATCH] Fix refreshAfterWrite racing with a removed entry (fixes #715) When a read triggers a refresh, it captures the key and value into local variables and performs validation checks. This should have included verifying that the entry is still alive (vs retired, dead). Otherwise, the key may be an internal sentinel and the refresh will fail. This has no harmful effects due to being logged and swallowed. --- .../caffeine/cache/BoundedLocalCache.java | 2 +- .../benmanes/caffeine/cache/NodeFactory.java | 18 +++++-- .../benmanes/caffeine/cache/References.java | 2 +- .../caffeine/cache/BoundedLocalCacheTest.java | 50 +++++++++++++++++++ 4 files changed, 66 insertions(+), 6 deletions(-) diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java index 77e680817e..5a28099f73 100644 --- a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java +++ b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java @@ -1265,7 +1265,7 @@ boolean skipReadBuffer() { if (((now - writeTime) > refreshAfterWriteNanos()) && (keyReference != null) && ((key = node.getKey()) != null) && ((oldValue = node.getValue()) != null) && ((writeTime & 1L) == 0L) && !(refreshes = refreshes()).containsKey(keyReference) - && node.casWriteTime(writeTime, refreshWriteTime)) { + && node.isAlive() && node.casWriteTime(writeTime, refreshWriteTime)) { long[] startTime = new long[1]; @SuppressWarnings({"unchecked", "rawtypes"}) CompletableFuture[] refreshFuture = new CompletableFuture[1]; diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/NodeFactory.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/NodeFactory.java index b1f68f8f6d..162fc27911 100644 --- a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/NodeFactory.java +++ b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/NodeFactory.java @@ -29,12 +29,13 @@ * @author ben.manes@gmail.com (Ben Manes) */ interface NodeFactory { - WeakKeyReference RETIRED_WEAK_KEY = new WeakKeyReference<>(null, null); - WeakKeyReference DEAD_WEAK_KEY = new WeakKeyReference<>(null, null); MethodType FACTORY = MethodType.methodType(void.class); MethodHandles.Lookup LOOKUP = MethodHandles.lookup(); - Object RETIRED_STRONG_KEY = new Object(); - Object DEAD_STRONG_KEY = new Object(); + + RetiredStrongKey RETIRED_STRONG_KEY = new RetiredStrongKey(); + RetiredWeakKey RETIRED_WEAK_KEY = new RetiredWeakKey(); + DeadStrongKey DEAD_STRONG_KEY = new DeadStrongKey(); + DeadWeakKey DEAD_WEAK_KEY = new DeadWeakKey(); String ACCESS_TIME = "accessTime"; String WRITE_TIME = "writeTime"; String VALUE = "value"; @@ -141,4 +142,13 @@ static NodeFactory loadFactory(String className) { throw new IllegalStateException(className, t); } } + + final class RetiredWeakKey extends WeakKeyReference { + RetiredWeakKey() { super(/* key */ null, /* referenceQueue */ null); } + } + final class DeadWeakKey extends WeakKeyReference { + DeadWeakKey() { super(/* key */ null, /* referenceQueue */ null); } + } + final class RetiredStrongKey {} + final class DeadStrongKey {} } diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/References.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/References.java index 458ad51fdd..d00b430bc8 100644 --- a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/References.java +++ b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/References.java @@ -177,7 +177,7 @@ public String toString() { * the advent that the key is reclaimed so that the entry can be removed from the cache in * constant time. */ - static final class WeakKeyReference extends WeakReference implements InternalReference { + static class WeakKeyReference extends WeakReference implements InternalReference { private final int hashCode; public WeakKeyReference(@Nullable K key, @Nullable ReferenceQueue queue) { diff --git a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/BoundedLocalCacheTest.java b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/BoundedLocalCacheTest.java index 36ec81e981..8278b4d9ae 100644 --- a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/BoundedLocalCacheTest.java +++ b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/BoundedLocalCacheTest.java @@ -43,6 +43,7 @@ import static org.hamcrest.Matchers.is; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -51,6 +52,7 @@ import java.lang.Thread.State; import java.lang.ref.Reference; import java.time.Duration; +import java.util.AbstractMap.SimpleEntry; import java.util.EnumSet; import java.util.List; import java.util.Map; @@ -72,6 +74,7 @@ import com.github.benmanes.caffeine.cache.Policy.FixedExpiration; import com.github.benmanes.caffeine.cache.Policy.VarExpiration; import com.github.benmanes.caffeine.cache.References.WeakKeyReference; +import com.github.benmanes.caffeine.cache.stats.StatsCounter; import com.github.benmanes.caffeine.cache.testing.CacheContext; import com.github.benmanes.caffeine.cache.testing.CacheProvider; import com.github.benmanes.caffeine.cache.testing.CacheSpec; @@ -1319,6 +1322,53 @@ public void expirationDelay_varTime(BoundedLocalCache cache, CacheCont /* --------------- Refresh --------------- */ + @Test(dataProvider = "caches") // Issue #715 + @CacheSpec(implementation = Implementation.Caffeine, population = Population.EMPTY, + refreshAfterWrite = Expire.ONE_MINUTE, executor = CacheExecutor.THREADED, + compute = Compute.SYNC, stats = Stats.DISABLED) + public void refreshIfNeeded_liveliness(CacheContext context) { + var stats = Mockito.mock(StatsCounter.class); + context.caffeine().recordStats(() -> stats); + + // Capture the refresh parameters, should not be retired/dead sentinel entry + var refreshEntry = new AtomicReference>(); + var cache = asBoundedLocalCache(context.build(new CacheLoader() { + @Override public Int load(Object key) { + throw new AssertionError(); + } + @Override public CompletableFuture asyncReload( + Object key, Object oldValue, Executor executor) { + refreshEntry.set(new SimpleEntry<>(key, oldValue)); + return CompletableFuture.completedFuture(key); + } + })); + + // Seed the cache + cache.put(context.absentKey(), context.absentValue()); + var node = cache.data.get(context.absentKey()); + + // Remove the entry after the read, but before the refresh, and leave it as retired + doAnswer(invocation -> { + ConcurrentTestHarness.execute(() -> { + cache.remove(context.absentKey()); + }); + await().until(() -> !cache.containsKey(context.absentKey())); + assertThat(node.isRetired()).isTrue(); + return null; + }).when(stats).recordHits(1); + + // Ensure that the refresh operation was skipped + cache.evictionLock.lock(); + try { + context.ticker().advance(10, TimeUnit.MINUTES); + var value = cache.getIfPresent(context.absentKey(), /* recordStats */ true); + assertThat(value).isEqualTo(context.absentValue()); + assertThat(refreshEntry.get()).isNull(); + } finally { + cache.evictionLock.unlock(); + } + } + @Test(dataProvider = "caches", groups = "isolated") @CacheSpec(implementation = Implementation.Caffeine, population = Population.EMPTY, refreshAfterWrite = Expire.ONE_MINUTE, executor = CacheExecutor.THREADED,