Fix compatibility between the cache compute methods and a load

The asMap().compute implementation did not take into account that the
present value may be loading. A load does not block other writes to
that entry and takes into account that it may be clobbered, causing
it to automatically discard itself. This is a known design choice that
breaks linearizability assumptions (#1881). The compute should check
if a load is in progress and call the appropriate internal removal
method.

Because a zombie entry remained in the cache and still is marked as
loading, the loader could discover entry and try to wait for it to
materialize. When the computation is a removal, indicated by a null
value, the loader would see this as the zombie's result. Since a cache
loader may not return null it would throw an exception to indicate
a user bug.

A new ComputingValueReference resolves both issues by indicating
that the load has completed. The compute's removeEntry will then
actually remove this entry and the loader will not wait on the
zombie. Instead if it observes the entry, it will neither receive
a non-null value or wait for it to load, but rather try to load
anew under the lock. This piggybacks on the reference collection
support where an entry is present but its value was garbage
collected, causing the load to proceed. By the time the lock is
obtained the compute method's entry was removed and the load
proceeds as normal (so no unnecessary notification is produced).

fixes #5342
fixes #2827
resolves underlying cause of #2108

Author: ben-manes

guava-tests/test/com/google/common/cache/LocalCacheMapComputeTest.java

@@ -18,9 +18,17 @@
 
 import static com.google.common.truth.Truth.assertThat;
 
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Queue;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
 import java.util.function.IntConsumer;
 import java.util.stream.IntStream;
+
+import com.google.common.util.concurrent.UncheckedExecutionException;
+
 import junit.framework.TestCase;
 
 /** Test Java8 map.compute in concurrent cache context. */
@@ -91,6 +99,27 @@ public void testComputeIfPresent() {
     assertThat(cache.getIfPresent(key).split(delimiter)).hasLength(count + 1);
   }
 
+  public void testComputeIfPresentRemove() {
+    List<RemovalNotification<Integer, Integer>> notifications = new ArrayList<>();
+    Cache<Integer, Integer> cache = CacheBuilder.newBuilder()
+        .removalListener(new RemovalListener<Integer, Integer>() {
+          @Override public void onRemoval(RemovalNotification<Integer, Integer> notification) {
+            notifications.add(notification);
+          }
+        }).build();
+    cache.put(1, 2);
+
+    // explicitly remove the existing value
+    cache.asMap().computeIfPresent(1, (key, value) -> null);
+    assertThat(notifications).hasSize(1);
+    CacheTesting.checkEmpty(cache);
+
+    // ensure no zombie entry remains
+    cache.asMap().computeIfPresent(1, (key, value) -> null);
+    assertThat(notifications).hasSize(1);
+    CacheTesting.checkEmpty(cache);
+  }
+
   public void testUpdates() {
     cache.put(key, "1");
     // simultaneous update for same key, some null, some non-null
@@ -113,6 +142,39 @@ public void testCompute() {
     assertEquals(0, cache.size());
   }
 
+  //
+  public void testComputeWithLoad() {
+    Queue<RemovalNotification<String, String>> notifications = new ConcurrentLinkedQueue<>();
+    cache = CacheBuilder.newBuilder()
+        .removalListener(new RemovalListener<String, String>() {
+          @Override public void onRemoval(RemovalNotification<String, String> notification) {
+            notifications.add(notification);
+          }
+        })
+        .expireAfterAccess(500000, TimeUnit.MILLISECONDS)
+        .maximumSize(count)
+        .build();
+
+    cache.put(key, "1");
+    // simultaneous load and deletion
+    doParallelCacheOp(
+        count,
+        n -> {
+          try {
+            cache.get(key, () -> key);
+            cache.asMap().compute(key, (k, v) -> null);
+          } catch (ExecutionException e) {
+            throw new UncheckedExecutionException(e);
+          }
+        });
+
+    CacheTesting.checkEmpty(cache);
+    for (RemovalNotification<String, String> entry : notifications) {
+      assertThat(entry.getKey()).isNotNull();
+      assertThat(entry.getValue()).isNotNull();
+    }
+  }
+
   public void testComputeExceptionally() {
     try {
       doParallelCacheOp(

guava/src/com/google/common/cache/LocalCache.java

@@ -2188,7 +2188,7 @@ V waitForLoadingValue(ReferenceEntry<K, V> e, K key, ValueReference<K, V> valueR
     V compute(K key, int hash, BiFunction<? super K, ? super V, ? extends V> function) {
       ReferenceEntry<K, V> e;
       ValueReference<K, V> valueReference = null;
-      LoadingValueReference<K, V> loadingValueReference = null;
+      ComputingValueReference<K, V> loadingValueReference = null;
       boolean createNewEntry = true;
       V newValue;
 
@@ -2229,7 +2229,7 @@ V compute(K key, int hash, BiFunction<? super K, ? super V, ? extends V> functio
 
         // note valueReference can be an existing value or even itself another loading value if
         // the value for the key is already being computed.
-        loadingValueReference = new LoadingValueReference<>(valueReference);
+        loadingValueReference = new ComputingValueReference<>(valueReference);
 
         if (e == null) {
           createNewEntry = true;
@@ -2257,6 +2257,9 @@ V compute(K key, int hash, BiFunction<? super K, ? super V, ? extends V> functio
         } else if (createNewEntry) {
           removeLoadingValue(key, hash, loadingValueReference);
           return null;
+        } else if (valueReference.isLoading()) {
+          removeLoadingValue(key, hash, loadingValueReference);
+          return null;
         } else {
           removeEntry(e, hash, RemovalCause.EXPLICIT);
           return null;
@@ -3465,6 +3468,18 @@ void runUnlockedCleanup() {
     }
   }
 
+  static class ComputingValueReference<K, V> extends LoadingValueReference<K, V> {
+    public ComputingValueReference() {
+      super();
+    }
+    public ComputingValueReference(ValueReference<K, V> oldValue) {
+      super(oldValue);
+    }
+    @Override public boolean isLoading() {
+      return false;
+    }
+  }
+
   static class LoadingValueReference<K, V> implements ValueReference<K, V> {
     volatile ValueReference<K, V> oldValue;
 
@@ -3927,7 +3942,7 @@ long longSize() {
     Segment<K, V>[] segments = this.segments;
     long sum = 0;
     for (int i = 0; i < segments.length; ++i) {
-      sum += Math.max(0, segments[i].count); // see https://github.com/google/guava/issues/2108
+      sum += segments[i].count;
     }
     return sum;
   }