From 4ba734a6cf2f7243c77d2ad8ea9d941f6e36175c Mon Sep 17 00:00:00 2001 From: Ben Manes Date: Sun, 14 Apr 2024 18:23:50 -0700 Subject: [PATCH] Fix containsKey for 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/cache/BoundedLocalCache.java | 2 +- .../caffeine/cache/LocalAsyncCache.java | 70 ++++++++++++++++++- .../benmanes/caffeine/cache/LocalCache.java | 2 +- .../caffeine/cache/UnboundedLocalCache.java | 3 +- .../benmanes/caffeine/cache/AsMapTest.java | 20 ++++++ .../caffeine/cache/ExpirationTest.java | 4 +- ...y-versions-caffeine-conventions.gradle.kts | 1 + 7 files changed, 95 insertions(+), 7 deletions(-) diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java index f0748f8244..d0b44963c8 100644 --- a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java +++ b/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 node = data.get(nodeFactory.newLookupKey(key)); if ((node == null) || ((value = node.getValue()) == null) diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalAsyncCache.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalAsyncCache.java index 998a794c50..a13715a593 100644 --- a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalAsyncCache.java +++ b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalAsyncCache.java @@ -591,6 +591,7 @@ public ConcurrentMap asMap() { final class AsMapView implements ConcurrentMap { final LocalCache> delegate; + @Nullable Set keys; @Nullable Collection values; @Nullable Set> 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 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 { + + @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 filter) { + return delegate.keySet().removeIf(filter); + } + + @Override + public boolean retainAll(Collection collection) { + return delegate.keySet().retainAll(collection); + } + + @Override + public Iterator iterator() { + return new Iterator<>() { + final Iterator> 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 { @Override diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalCache.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalCache.java index d16d1359bd..04347d4614 100644 --- a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalCache.java +++ b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalCache.java @@ -82,7 +82,7 @@ interface LocalCache extends ConcurrentMap { * the statistics nor the eviction policy. */ @Nullable - V getIfPresentQuietly(K key); + V getIfPresentQuietly(Object key); /** See {@link Cache#getAllPresent}. */ Map getAllPresent(Iterable keys); diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/UnboundedLocalCache.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/UnboundedLocalCache.java index 5d6b649922..ed8187bfac 100644 --- a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/UnboundedLocalCache.java +++ b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/UnboundedLocalCache.java @@ -138,7 +138,8 @@ public boolean isPendingEviction(K key) { } @Override - public @Nullable V getIfPresentQuietly(K key) { + @SuppressWarnings("SuspiciousMethodCalls") + public @Nullable V getIfPresentQuietly(Object key) { return data.get(key); } diff --git a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/AsMapTest.java b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/AsMapTest.java index c4384c0961..c993cd1987 100644 --- a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/AsMapTest.java +++ b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/AsMapTest.java @@ -148,6 +148,16 @@ public void containsKey_absent(Map map, CacheContext context) { assertThat(map.containsKey(context.absentKey())).isFalse(); } + @CheckNoStats + @Test(dataProvider = "caches") + @CacheSpec(removalListener = { Listener.DISABLED, Listener.REJECTING }) + public void containsKey_inFlight(AsyncCache cache, CacheContext context) { + var future = new CompletableFuture(); + 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 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 cache, CacheContext context) { + var future = new CompletableFuture(); + 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, diff --git a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/ExpirationTest.java b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/ExpirationTest.java index 6066461897..16b2977189 100644 --- a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/ExpirationTest.java +++ b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/ExpirationTest.java @@ -680,10 +680,10 @@ public void containsKey_inFlight(AsyncCache cache, CacheContext contex var future = new CompletableFuture(); 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); } diff --git a/gradle/plugins/src/main/kotlin/lifecycle/dependency-versions-caffeine-conventions.gradle.kts b/gradle/plugins/src/main/kotlin/lifecycle/dependency-versions-caffeine-conventions.gradle.kts index 2c56543516..ada747dfa5 100644 --- a/gradle/plugins/src/main/kotlin/lifecycle/dependency-versions-caffeine-conventions.gradle.kts +++ b/gradle/plugins/src/main/kotlin/lifecycle/dependency-versions-caffeine-conventions.gradle.kts @@ -18,6 +18,7 @@ tasks.named("dependencyUpdates").configure { } } force(libs.guice) + force(libs.commons.collections4) force(libs.bundles.coherence.get()) } }