Null the value field when the entry is dead (evicted)

In #103 a user broke the map contract by accidentially modifying the
key, causing lookups to not find the entry. This includes eviction,
which finds the entry on a linked list ("Node") and removes from the
hash table with the stashed key. While the node can be marked as dead,
the removal might fail in practice if racing with an explicit
invalidation.

When an weak/soft value entry is dead we already clear the reference to
reduce the GC burden. In the common case null'ing out the field would
have no benefit, because the GC would trace live objects and not need
the hint. In the above case it reduces the impact of a memory leak by
collecting the heavy user objects. The node instances would still
accumulate and the leak may take longer to fail. But since we do it for
reference caching and it could reduce the likelihood of outages, it
does seems friendlier to null the strong value field.

caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache/node/AddHealth.java

@@ -58,11 +58,11 @@ protected void execute() {
         .addModifiers(Modifier.PUBLIC, Modifier.FINAL)
         .returns(boolean.class)
         .build());
-    addState("isRetired", "retire", retiredArg);
-    addState("isDead", "die", deadArg);
+    addState("isRetired", "retire", retiredArg, false);
+    addState("isDead", "die", deadArg, true);
   }
 
-  private void addState(String checkName, String actionName, String arg) {
+  private void addState(String checkName, String actionName, String arg, boolean finalized) {
     context.nodeSubtype.addMethod(MethodSpec.methodBuilder(checkName)
         .addStatement("return (getKeyReference() == $L)", arg)
         .addModifiers(Modifier.PUBLIC, Modifier.FINAL)
@@ -74,7 +74,14 @@ private void addState(String checkName, String actionName, String arg) {
     if (keyStrength() != Strength.STRONG) {
       action.addStatement("(($T<K>) getKeyReference()).clear()", Reference.class);
     }
-    if (valueStrength() != Strength.STRONG) {
+    if (valueStrength() == Strength.STRONG) {
+      // Set the value to null only when dead, as otherwise the explicit removal of an expired async
+      // value will be notified as explicit rather than expired due to the isComputingAsync() check
+      if (finalized) {
+        action.addStatement("$T.UNSAFE.putObject(this, $N, null)",
+            UNSAFE_ACCESS, offsetName("value"));
+      }
+    } else {
       action.addStatement("(($T<V>) getValueReference()).clear()", Reference.class);
     }
     action.addStatement("$T.UNSAFE.putObject(this, $N, $N)", UNSAFE_ACCESS, offsetName("key"), arg);

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

@@ -1210,13 +1210,16 @@ public void run() {
 
       // Ensure that in-flight async computation cannot expire
       if (isComputingAsync(node)) {
-        node.setAccessTime(Long.MAX_VALUE);
-        node.setWriteTime(Long.MAX_VALUE);
-        ((CompletableFuture<?>) node.getValue()).thenRun(() -> {
-          long now = expirationTicker().read();
-          node.setAccessTime(now);
-          node.setWriteTime(now);
-        });
+        CompletableFuture<?> future = (CompletableFuture<?>) node.getValue();
+        if (future != null) {
+          node.setAccessTime(Long.MAX_VALUE);
+          node.setWriteTime(Long.MAX_VALUE);
+          future.thenRun(() -> {
+            long now = expirationTicker().read();
+            node.setAccessTime(now);
+            node.setWriteTime(now);
+          });
+        }
       }
     }
   }

caffeine/src/test/java/com/github/benmanes/caffeine/cache/EvictionTest.java

@@ -25,6 +25,7 @@
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.is;
 import static org.mockito.Mockito.verify;
 
@@ -201,7 +202,8 @@ public void evict_weighted_entryTooBig(Cache<Integer, Integer> cache, CacheConte
   @Test(dataProvider = "caches")
   @CacheSpec(implementation = Implementation.Caffeine, maximumSize = Maximum.TEN,
       weigher = CacheWeigher.VALUE, population = Population.EMPTY,
-      keys = ReferenceType.STRONG, values = ReferenceType.STRONG)
+      keys = ReferenceType.STRONG, values = ReferenceType.STRONG,
+      removalListener = Listener.CONSUMING)
   public void evict_weighted_async(AsyncLoadingCache<Integer, Integer> cache,
       CacheContext context, Eviction<?, ?> eviction) {
     AtomicBoolean ready = new AtomicBoolean();
@@ -220,6 +222,7 @@ public void evict_weighted_async(AsyncLoadingCache<Integer, Integer> cache,
 
     ready.set(true);
     Awaits.await().untilTrue(done);
+    Awaits.await().until(context::consumedNotifications, hasSize(1));
     Awaits.await().until(() -> cache.synchronous().estimatedSize(), is(2L));
     Awaits.await().until(() -> eviction.weightedSize().getAsLong(), is(10L));
 

gradle/dependencies.gradle

@@ -29,14 +29,14 @@ ext {
     commons_compress: '1.12',
     commons_lang3: '3.4',
     config: '1.3.0',
-    error_prone_annotations: '2.0.10',
+    error_prone_annotations: '2.0.11',
     flip_tables: '1.0.2',
     guava: '19.0',
     javapoet: '1.7.0',
     jcache: '1.0.0',
     jsr305: '3.0.1',
-    stream: '2.9.4',
-    univocity_parsers: '2.1.2',
+    stream: '2.9.5',
+    univocity_parsers: '2.2.0',
     ycsb: '0.10.0',
     xz: '1.5',
   ]
@@ -47,33 +47,33 @@ ext {
     jcache_tck: '1.0.1',
     jctools: '1.2.1',
     junit: '4.12',
-    mockito: '2.0.86-beta',
+    mockito: '2.0.96-beta',
     pax_exam: '4.9.1',
     testng: '6.9.12',
     truth: '0.24',
   ]
   benchmark_versions = [
-    cache2k: '0.26-BETA',
+    cache2k: '0.27-BETA',
     concurrentlinkedhashmap: '1.4.2',
     ehcache2: '2.10.2.2.21',
-    ehcache3: '3.1.0',
+    ehcache3: '3.1.1',
     elastic_search: '5.0.0-alpha4',
     infinispan: '9.0.0.Alpha3',
-    jackrabbit: '1.5.5',
+    jackrabbit: '1.5.6',
     jamm: '0.3.1',
     java_object_layout: '0.5',
     koloboke: '0.6.8',
     slf4j: '1.7.21',
     tcache: '0.9.4',
   ]
   plugin_versions = [
-    checkstyle: '7.0',
+    checkstyle: '7.1',
     coveralls: '2.6.3',
     extra_conf: '3.1.0',
     error_prone: '0.0.8',
     jmh: '0.3.0',
     nexus: '2.3.1',
-    pmd: '5.4.1',
+    pmd: '5.5.1',
     semantic_versioning: '1.1.0',
     shadow: '1.2.3',
     stats: '0.2.0',

gradle/wrapper/gradle-wrapper.properties

@@ -3,4 +3,4 @@ distributionBase=GRADLE_USER_HOME
 distributionPath=wrapper/dists
 zipStoreBase=GRADLE_USER_HOME
 zipStorePath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-2.14.1-rc-2-bin.zip
+distributionUrl=https\://services.gradle.org/distributions/gradle-3.0-rc-1-bin.zip