Fixed race causing an incorrect removal cause (fixes #412)

This bug manifests due to an incorrectly optimized `Map.remove(key)`
implementation when there is no CacheWriter specified. If there is,
an alternative implementation is used which does not suffer from this
mistake. This change now uses that version in all cases.

The explicit removal and an eviction of the same entry can occur
concurrently. If the removal wins the race, it is responsible for
retiring the entry. However even after that entry is discarded, an
further evictions may be necessary to meet the bounding criteria
(e.g. weighted entries). Therefore the eviction thread eagerly
discards the entry from the policy's data structures so that it can
continue its evaluation. While the entry is synchronized in both
cases, the eviction thread could lose the removal but retire the
entry first. This lead to the removal seeing a dead entry and
miscommunicating the cause to the listener.

There is a modest performance difference in a microbenchmark because
of a slightly coarser locking. However most of the differences between
libraries is whether they decide to be linearizable or perform an
optimistic check to no-op if the entry is absent. The optimistic check
avoids locking but also can lead to a removal to not block waiting for
an in-flight computation to complete. While a valid choice, some users
have expressed a desire for removal to be pessimistic and is rare
enough that we follow their wishes.

caffeine/src/jmh/java/com/github/benmanes/caffeine/cache/BasicCache.java

@@ -26,12 +26,14 @@
 public interface BasicCache<K, V> {
 
   /** Returns the value stored in the cache, or null if not present. */
-  @Nullable
-  V get(@NonNull K key);
+  @Nullable V get(@NonNull K key);
 
   /** Stores the value into the cache, replacing an existing mapping if present. */
   void put(@NonNull K key, @NonNull V value);
 
+  /** Removes the entry from the cache, if present. */
+  void remove(@NonNull K key);
+
   /** Invalidates all entries from the cache. */
   void clear();
 

caffeine/src/jmh/java/com/github/benmanes/caffeine/cache/PutRemoveBenchmark.java

@@ -0,0 +1,108 @@
+/*
+ * Copyright 2014 Ben Manes. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.github.benmanes.caffeine.cache;
+
+import java.util.Random;
+
+import org.openjdk.jmh.annotations.Benchmark;
+import org.openjdk.jmh.annotations.Group;
+import org.openjdk.jmh.annotations.GroupThreads;
+import org.openjdk.jmh.annotations.Level;
+import org.openjdk.jmh.annotations.Param;
+import org.openjdk.jmh.annotations.Scope;
+import org.openjdk.jmh.annotations.Setup;
+import org.openjdk.jmh.annotations.State;
+import org.openjdk.jmh.annotations.TearDown;
+
+import site.ycsb.generator.NumberGenerator;
+import site.ycsb.generator.ScrambledZipfianGenerator;
+
+/**
+ * A benchmark that loosely evaluates the put and removal performance of the cache. The cache is
+ * written using a Zipf distribution of keys is used to incur lock contention on popular entries.
+ * <p>
+ * The performance of this benchmark is expected to be unrealistic due to removing entries. A cache
+ * that optimistically skips locking when the entry is absent receives a benefit, but sacrifices
+ * linearizability with a computation. In real-world usages the rate of explicit writes is
+ * relatively rare compared to reads. Thus, this benchmark is only for diagnosing performance
+ * concerns and should not be used to compare implementations.
+ * <p>
+ * <pre>{@code
+ *   ./gradlew jmh -PincludePattern=PutRemoveBenchmark
+ * }</pre>
+ *
+ * @author ben.manes@gmail.com (Ben Manes)
+ */
+@State(Scope.Group)
+public class PutRemoveBenchmark {
+  private static final int SIZE = (2 << 14);
+  private static final int MASK = SIZE - 1;
+  private static final int ITEMS = SIZE / 3;
+
+  @Param({
+    "Caffeine",
+    "LinkedHashMap_Lru",
+    "ConcurrentHashMap",
+    "ConcurrentLinkedHashMap",
+    "Guava",
+    "Cache2k",
+    "Ehcache3",
+  })
+  CacheType cacheType;
+
+  BasicCache<Integer, Boolean> cache;
+  Integer[] ints;
+
+  @State(Scope.Thread)
+  public static class ThreadState {
+    static final Random random = new Random();
+    int index = random.nextInt();
+  }
+
+  @Setup
+  public void setup() {
+    ints = new Integer[SIZE];
+    cache = cacheType.create(2 * SIZE);
+
+    // Enforce full initialization of internal structures
+    for (int i = 0; i < 2 * SIZE; i++) {
+      cache.put(i, Boolean.TRUE);
+    }
+    cache.clear();
+
+    // Populate with a realistic access distribution
+    NumberGenerator generator = new ScrambledZipfianGenerator(ITEMS);
+    for (int i = 0; i < SIZE; i++) {
+      ints[i] = generator.nextValue().intValue();
+      cache.put(ints[i], Boolean.TRUE);
+    }
+  }
+
+  @TearDown(Level.Iteration)
+  public void tearDown() {
+    cache.cleanUp();
+  }
+
+  @Benchmark @Group @GroupThreads(4)
+  public void put(ThreadState threadState) {
+    cache.put(ints[threadState.index++ & MASK], Boolean.TRUE);
+  }
+
+  @Benchmark @Group @GroupThreads(4)
+  public void remove(ThreadState threadState) {
+    cache.remove(ints[threadState.index++ & MASK]);
+  }
+}

caffeine/src/jmh/java/com/github/benmanes/caffeine/cache/impl/Cache2k.java

@@ -28,7 +28,7 @@ public final class Cache2k<K, V> implements BasicCache<K, V> {
 
   @SuppressWarnings("unchecked")
   public Cache2k(int maximumSize) {
-    cache = (Cache<K, V>) Cache2kBuilder.forUnknownTypes()
+    cache = Cache2kBuilder.forUnknownTypes()
         .entryCapacity(maximumSize)
         .eternal(true)
         .build();
@@ -44,6 +44,11 @@ public void put(K key, V value) {
     cache.put(key, value);
   }
 
+  @Override
+  public void remove(K key) {
+    cache.remove(key);
+  }
+
   @Override
   public void clear() {
     cache.clear();

caffeine/src/jmh/java/com/github/benmanes/caffeine/cache/impl/CaffeineCache.java

@@ -46,6 +46,11 @@ public void put(K key, V value) {
     map.put(key, value);
   }
 
+  @Override
+  public void remove(K key) {
+    map.remove(key);
+  }
+
   @Override
   public void clear() {
     cache.invalidateAll();

caffeine/src/jmh/java/com/github/benmanes/caffeine/cache/impl/Collision.java

@@ -43,6 +43,11 @@ public void put(K key, V value) {
     cache.putReplace(key, value);
   }
 
+  @Override
+  public void remove(K key) {
+    cache.remove(key);
+  }
+
   @Override
   public void clear() {
     cache.clear();

caffeine/src/jmh/java/com/github/benmanes/caffeine/cache/impl/ConcurrentMapCache.java

@@ -41,6 +41,11 @@ public void put(K key, V value) {
     map.put(key, value);
   }
 
+  @Override
+  public void remove(K key) {
+    map.remove(key);
+  }
+
   @Override
   public void clear() {
     map.clear();

caffeine/src/jmh/java/com/github/benmanes/caffeine/cache/impl/Ehcache3.java

@@ -50,6 +50,11 @@ public void put(K key, V value) {
     cache.put(key, value);
   }
 
+  @Override
+  public void remove(K key) {
+    cache.remove(key);
+  }
+
   @Override
   public void clear() {
     cache.clear();

caffeine/src/jmh/java/com/github/benmanes/caffeine/cache/impl/ElasticSearchCache.java

@@ -42,6 +42,11 @@ public void put(K key, V value) {
     cache.put(key, value);
   }
 
+  @Override
+  public void remove(K key) {
+    cache.invalidate(key);
+  }
+
   @Override
   public void clear() {
     cache.invalidateAll();

caffeine/src/jmh/java/com/github/benmanes/caffeine/cache/impl/ExpiringMapCache.java

@@ -43,6 +43,11 @@ public void put(K key, V value) {
     cache.put(key, value);
   }
 
+  @Override
+  public void remove(K key) {
+    cache.remove(key);
+  }
+
   @Override
   public void clear() {
     cache.clear();

caffeine/src/jmh/java/com/github/benmanes/caffeine/cache/impl/GuavaCache.java

@@ -50,6 +50,11 @@ public void put(K key, V value) {
     cache.put(key, value);
   }
 
+  @Override
+  public void remove(K key) {
+    cache.invalidate(key);
+  }
+
   @Override
   public void clear() {
     cache.invalidateAll();

caffeine/src/jmh/java/com/github/benmanes/caffeine/cache/impl/LinkedHashMapCache.java

@@ -44,6 +44,13 @@ public void put(K key, V value) {
     }
   }
 
+  @Override
+  public void remove(K key) {
+    synchronized (map) {
+      map.remove(key);
+    }
+  }
+
   @Override
   public void clear() {
     synchronized (map) {

caffeine/src/jmh/java/com/github/benmanes/caffeine/cache/impl/TCache.java

@@ -47,6 +47,11 @@ public void put(K key, V value) {
     cache.put(key, value);
   }
 
+  @Override
+  public void remove(K key) {
+    cache.remove(key);
+  }
+
   @Override
   public void clear() {
     cache.clear();

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

@@ -2088,56 +2088,6 @@ public Map<K, V> getAllPresent(Iterable<?> keys) {
 
   @Override
   public @Nullable V remove(Object key) {
-    return hasWriter()
-        ? removeWithWriter(key)
-        : removeNoWriter(key);
-  }
-
-  /**
-   * Removes the mapping for a key without notifying the writer.
-   *
-   * @param key key whose mapping is to be removed
-   * @return the removed value or null if no mapping was found
-   */
-  @Nullable V removeNoWriter(Object key) {
-    Node<K, V> node = data.remove(nodeFactory.newLookupKey(key));
-    if (node == null) {
-      return null;
-    }
-
-    V oldValue;
-    synchronized (node) {
-      oldValue = node.getValue();
-      if (node.isAlive()) {
-        node.retire();
-      }
-    }
-
-    RemovalCause cause;
-    if (oldValue == null) {
-      cause = RemovalCause.COLLECTED;
-    } else if (hasExpired(node, expirationTicker().read())) {
-      cause = RemovalCause.EXPIRED;
-    } else {
-      cause = RemovalCause.EXPLICIT;
-    }
-
-    if (hasRemovalListener()) {
-      @SuppressWarnings("unchecked")
-      K castKey = (K) key;
-      notifyRemoval(castKey, oldValue, cause);
-    }
-    afterWrite(new RemovalTask(node));
-    return (cause == RemovalCause.EXPLICIT) ? oldValue : null;
-  }
-
-  /**
-   * Removes the mapping for a key after notifying the writer.
-   *
-   * @param key key whose mapping is to be removed
-   * @return the removed value or null if no mapping was found
-   */
-  @Nullable V removeWithWriter(Object key) {
     @SuppressWarnings("unchecked")
     K castKey = (K) key;
     @SuppressWarnings({"unchecked", "rawtypes"})

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

@@ -167,7 +167,7 @@ default Map<K, V> getAll(@NonNull Iterable<? extends @NonNull K> keys,
    * @param keys the keys whose associated values are to be removed
    * @throws NullPointerException if the specified collection is null or contains a null element
    */
-  void invalidateAll(@NonNull Iterable<?> keys);
+  void invalidateAll(@NonNull Iterable<@NonNull ?> keys);
 
   /**
    * Discards all entries in the cache. The behavior of this operation is undefined for an entry