Fix contains key for the async cache's synchronous view when in-flight (

fixes #1626)

The keySet and its iterator will now suppress in-flight entries for
the contains test and by its iterator. The retain and removal methods
will discard in-flight mappings, like the entrySet view does. This is
intentional as it is better to overly discard data then keep stale
contents due to incorrect linearization assumptions (of which async can
offer few). The synchronous views are best-effort, lenient, and try to
match a user's likely expected or intended behavior while being
conservative by being cautiously safe. Thus, small leaks of in-flight
behavior is preferrable, especially given the concurrent nature of the
use-case, and we encourage using the async asMap view when
orchastrating more nuanced behavior.

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

@@ -2208,7 +2208,7 @@ public boolean containsValue(Object value) {
   }
 
   @Override
-  public @Nullable V getIfPresentQuietly(K key) {
+  public @Nullable V getIfPresentQuietly(Object key) {
     V value;
     Node<K, V> node = data.get(nodeFactory.newLookupKey(key));
     if ((node == null) || ((value = node.getValue()) == null)

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

@@ -591,6 +591,7 @@ public ConcurrentMap<K, V> asMap() {
   final class AsMapView<K, V> implements ConcurrentMap<K, V> {
     final LocalCache<K, CompletableFuture<V>> delegate;
 
+    @Nullable Set<K> keys;
     @Nullable Collection<V> values;
     @Nullable Set<Entry<K, V>> entries;
 
@@ -615,7 +616,7 @@ public void clear() {
 
     @Override
     public boolean containsKey(Object key) {
-      return delegate.containsKey(key);
+      return Async.isReady(delegate.getIfPresentQuietly(key));
     }
 
     @Override
@@ -950,7 +951,7 @@ public boolean replace(K key, V oldValue, V newValue) {
 
     @Override
     public Set<K> keySet() {
-      return delegate.keySet();
+      return (keys == null) ? (keys = new KeySet()) : keys;
     }
 
     @Override
@@ -1016,6 +1017,71 @@ public String toString() {
       return result.append('}').toString();
     }
 
+    private final class KeySet extends AbstractSet<K> {
+
+      @Override
+      public boolean isEmpty() {
+        return AsMapView.this.isEmpty();
+      }
+
+      @Override
+      public int size() {
+        return AsMapView.this.size();
+      }
+
+      @Override
+      public void clear() {
+        AsMapView.this.clear();
+      }
+
+      @Override
+      public boolean contains(Object o) {
+        return AsMapView.this.containsKey(o);
+      }
+
+      @Override
+      public boolean removeAll(Collection<?> collection) {
+        return delegate.keySet().removeAll(collection);
+      }
+
+      @Override
+      public boolean remove(Object o) {
+        return delegate.keySet().remove(o);
+      }
+
+      @Override
+      public boolean removeIf(Predicate<? super K> filter) {
+        return delegate.keySet().removeIf(filter);
+      }
+
+      @Override
+      public boolean retainAll(Collection<?> collection) {
+        return delegate.keySet().retainAll(collection);
+      }
+
+      @Override
+      public Iterator<K> iterator() {
+        return new Iterator<>() {
+          final Iterator<Entry<K, V>> iterator = entrySet().iterator();
+
+          @Override
+          public boolean hasNext() {
+            return iterator.hasNext();
+          }
+
+          @Override
+          public K next() {
+            return iterator.next().getKey();
+          }
+
+          @Override
+          public void remove() {
+            iterator.remove();
+          }
+        };
+      }
+    }
+
     private final class Values extends AbstractCollection<V> {
 
       @Override

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

@@ -82,7 +82,7 @@ interface LocalCache<K, V> extends ConcurrentMap<K, V> {
    * the statistics nor the eviction policy.
    */
   @Nullable
-  V getIfPresentQuietly(K key);
+  V getIfPresentQuietly(Object key);
 
   /** See {@link Cache#getAllPresent}. */
   Map<K, V> getAllPresent(Iterable<? extends K> keys);

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

@@ -138,7 +138,7 @@ public boolean isPendingEviction(K key) {
   }
 
   @Override
-  public @Nullable V getIfPresentQuietly(K key) {
+  public @Nullable V getIfPresentQuietly(Object key) {
     return data.get(key);
   }
 

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

@@ -148,6 +148,16 @@ public void containsKey_absent(Map<Int, Int> map, CacheContext context) {
     assertThat(map.containsKey(context.absentKey())).isFalse();
   }
 
+  @CheckNoStats
+  @Test(dataProvider = "caches")
+  @CacheSpec(removalListener = { Listener.DISABLED, Listener.REJECTING })
+  public void containsKey_inFlight(AsyncCache<Int, Int> cache, CacheContext context) {
+    var future = new CompletableFuture<Int>();
+    cache.put(context.absentKey(), future);
+    assertThat(cache.synchronous().asMap().containsKey(context.absentKey())).isFalse();
+    cache.synchronous().invalidate(context.absentKey());
+  }
+
   @CheckNoStats
   @Test(dataProvider = "caches")
   @CacheSpec(removalListener = { Listener.DISABLED, Listener.REJECTING })
@@ -1766,6 +1776,16 @@ public void keySet_contains_present(Map<Int, Int> map, CacheContext context) {
     assertThat(map.keySet().contains(context.firstKey())).isTrue();
   }
 
+  @CheckNoStats
+  @Test(dataProvider = "caches")
+  @CacheSpec(removalListener = { Listener.DISABLED, Listener.REJECTING })
+  public void keySet_contains_inFlight(AsyncCache<Int, Int> cache, CacheContext context) {
+    var future = new CompletableFuture<Int>();
+    cache.put(context.absentKey(), future);
+    assertThat(cache.synchronous().asMap().keySet().contains(context.absentKey())).isFalse();
+    cache.synchronous().invalidate(context.absentKey());
+  }
+
   @CheckNoStats
   @Test(dataProvider = "caches")
   @CacheSpec(population = Population.EMPTY,

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

@@ -680,10 +680,10 @@ public void containsKey_inFlight(AsyncCache<Int, Int> cache, CacheContext contex
     var future = new CompletableFuture<Int>();
     cache.put(context.absentKey(), future);
     assertThat(cache.asMap().containsKey(context.absentKey())).isTrue();
-    assertThat(cache.synchronous().asMap().containsKey(context.absentKey())).isTrue();
+    assertThat(cache.synchronous().asMap().containsKey(context.absentKey())).isFalse();
     context.ticker().advance(Duration.ofMinutes(5));
     assertThat(cache.asMap().containsKey(context.absentKey())).isTrue();
-    assertThat(cache.synchronous().asMap().containsKey(context.absentKey())).isTrue();
+    assertThat(cache.synchronous().asMap().containsKey(context.absentKey())).isFalse();
     future.complete(null);
   }
 

gradle/plugins/src/main/kotlin/lifecycle/dependency-versions-caffeine-conventions.gradle.kts

@@ -18,6 +18,7 @@ tasks.named<DependencyUpdatesTask>("dependencyUpdates").configure {
       }
     }
     force(libs.guice)
+    force(libs.commons.collections4)
     force(libs.bundles.coherence.get())
   }
 }