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,