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/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<? extends V>[] refreshFuture = new CompletableFuture[1];

caffeine/src/main/java/com/github/benmanes/caffeine/cache/NodeFactory.java

@@ -29,12 +29,13 @@
  * @author ben.manes@gmail.com (Ben Manes)
  */
 interface NodeFactory<K, V> {
-  WeakKeyReference<Object> RETIRED_WEAK_KEY = new WeakKeyReference<>(null, null);
-  WeakKeyReference<Object> 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 <K, V> NodeFactory<K, V> loadFactory(String className) {
       throw new IllegalStateException(className, t);
     }
   }
+
+  final class RetiredWeakKey extends WeakKeyReference<Object> {
+    RetiredWeakKey() { super(/* key */ null, /* referenceQueue */ null); }
+  }
+  final class DeadWeakKey extends WeakKeyReference<Object> {
+    DeadWeakKey() { super(/* key */ null, /* referenceQueue */ null); }
+  }
+  final class RetiredStrongKey {}
+  final class DeadStrongKey {}
 }

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<K> extends WeakReference<K> implements InternalReference<K> {
+  static class WeakKeyReference<K> extends WeakReference<K> implements InternalReference<K> {
     private final int hashCode;
 
     public WeakKeyReference(@Nullable K key, @Nullable ReferenceQueue<K> queue) {

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<Int, Int> 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<Map.Entry<Object, Object>>();
+    var cache = asBoundedLocalCache(context.build(new CacheLoader<Object, Object>() {
+      @Override public Int load(Object key) {
+        throw new AssertionError();
+      }
+      @Override public CompletableFuture<Object> 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,