AsyncCache#getAll could skip storing additional mappings (fixes #655)

ben-manes · Mar 20, 2022 · 0cef551 · 0cef551
1 parent 7458245
commit 0cef551
Show file tree

Hide file tree

Showing 4 changed files with 66 additions and 3 deletions.
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
@@ -268,9 +268,6 @@ private void fillProxies(Map<K, V> result) {
 
     /** Adds to the cache any extra entries computed that were not requested. */
     private void addNewEntries(Map<K, V> result) {
-      if (proxies.size() == result.size()) {
-        return;
-      }
       result.forEach((key, value) -> {
         if (!proxies.containsKey(key)) {
           cache.put(key, CompletableFuture.completedFuture(value));

diff --git a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/AsyncCacheTest.java b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/AsyncCacheTest.java
@@ -21,11 +21,14 @@
 import static com.github.benmanes.caffeine.testing.Awaits.await;
 import static com.github.benmanes.caffeine.testing.IsFutureValue.futureOf;
 import static com.google.common.collect.Streams.stream;
+import static java.util.function.Function.identity;
 import static java.util.stream.Collectors.toMap;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.aMapWithSize;
 import static org.hamcrest.Matchers.anEmptyMap;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.greaterThan;
+import static org.hamcrest.Matchers.hasItems;
 import static org.hamcrest.Matchers.hasKey;
 import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.is;
@@ -42,6 +45,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Map.Entry;
 import java.util.Set;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.CompletionException;
@@ -453,6 +457,21 @@ public void getAllFunction_exceeds(AsyncCache<Integer, Integer> cache, CacheCont
     verifyStats(context, verifier -> verifier.hits(0).misses(result.size()).success(1).failures(0));
   }
 
+  @Test(dataProvider = "caches")
+  @CacheSpec(removalListener = { Listener.DEFAULT, Listener.REJECTING })
+  public void getAllFunction_different(AsyncCache<Integer, Integer> cache, CacheContext context) {
+    Map<Integer, Integer> actual = context.absentKeys().stream()
+        .collect(toMap(k -> -k, identity()));
+    Map<Integer, Integer> result = cache.getAll(context.absentKeys(), keys -> actual).join();
+    @SuppressWarnings({"unchecked", "rawtypes"})
+    Entry<Integer, Integer>[] array = actual.entrySet().toArray(new Map.Entry[0]);
+
+    assertThat(result, is(anEmptyMap()));
+    assertThat(cache.synchronous().asMap().entrySet(), hasItems(array));
+    assertThat(cache.asMap(), aMapWithSize(context.original().size() + actual.size()));
+    verifyStats(context, verifier -> verifier.hits(0).misses(actual.size()).success(1).failures(0));
+  }
+
   @CheckNoWriter
   @Test(dataProvider = "caches")
   @CacheSpec(population = { Population.SINGLETON, Population.PARTIAL, Population.FULL },
@@ -719,6 +738,23 @@ public void getAllBifunction_exceeds(AsyncCache<Integer, Integer> cache, CacheCo
     verifyStats(context, verifier -> verifier.hits(0).misses(result.size()).success(1).failures(0));
   }
 
+  @Test(dataProvider = "caches")
+  @CacheSpec(removalListener = { Listener.DEFAULT, Listener.REJECTING })
+  public void getAllBifunction_different(AsyncCache<Integer, Integer> cache, CacheContext context) {
+    Map<Integer, Integer> actual = context.absentKeys().stream()
+        .collect(toMap(k -> -k, identity()));
+    Map<Integer, Integer> result = cache.getAll(context.absentKeys(), (keys, executor) -> {
+      return CompletableFuture.completedFuture(actual);
+    }).join();
+    @SuppressWarnings({"unchecked", "rawtypes"})
+    Entry<Integer, Integer>[] array = actual.entrySet().toArray(new Map.Entry[0]);
+
+    assertThat(result, is(anEmptyMap()));
+    assertThat(cache.synchronous().asMap().entrySet(), hasItems(array));
+    assertThat(cache.asMap(), is(aMapWithSize(context.original().size() + actual.size())));
+    verifyStats(context, verifier -> verifier.hits(0).misses(actual.size()).success(1).failures(0));
+  }
+
   @CheckNoWriter
   @Test(dataProvider = "caches")
   @CacheSpec(population = { Population.SINGLETON, Population.PARTIAL, Population.FULL },

diff --git a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/AsyncLoadingCacheTest.java b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/AsyncLoadingCacheTest.java
@@ -20,8 +20,10 @@
 import static com.github.benmanes.caffeine.testing.Awaits.await;
 import static com.github.benmanes.caffeine.testing.IsFutureValue.futureOf;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.anEmptyMap;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.greaterThan;
+import static org.hamcrest.Matchers.hasItems;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.nullValue;
 import static org.hamcrest.Matchers.sameInstance;
@@ -31,6 +33,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Map.Entry;
 import java.util.Set;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.CompletionException;
@@ -237,6 +240,21 @@ public void getAll_exceeds(AsyncLoadingCache<Integer, Integer> cache, CacheConte
     verifyStats(context, verifier -> verifier.hits(0).misses(result.size()).success(1).failures(0));
   }
 
+  @Test(dataProvider = "caches")
+  @CacheSpec(loader = Loader.BULK_DIFFERENT,
+      removalListener = { Listener.DEFAULT, Listener.REJECTING })
+  public void getAll_different(AsyncLoadingCache<Integer, Integer> cache, CacheContext context) {
+    Map<Integer, Integer> result = cache.getAll(context.absentKeys()).join();
+    @SuppressWarnings({"unchecked", "rawtypes"})
+    Entry<Integer, CompletableFuture<Integer>>[] expected =
+        result.entrySet().toArray(new Map.Entry[0]);
+
+    assertThat(result, is(anEmptyMap()));
+    assertThat(cache.asMap().entrySet(), hasItems(expected));
+    verifyStats(context, verifier ->
+        verifier.hits(0).misses(context.absent().size()).success(1).failures(0));
+  }
+
   @CheckNoWriter
   @Test(dataProvider = "caches")
   @CacheSpec(loader = { Loader.NEGATIVE, Loader.BULK_NEGATIVE },

diff --git a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/testing/CacheSpec.java b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/testing/CacheSpec.java
@@ -19,6 +19,8 @@
 import static java.lang.annotation.ElementType.METHOD;
 import static java.lang.annotation.RetentionPolicy.RUNTIME;
 import static java.util.Objects.requireNonNull;
+import static java.util.function.Function.identity;
+import static java.util.stream.Collectors.toMap;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.Mockito.when;
@@ -53,6 +55,7 @@
 import com.github.benmanes.caffeine.cache.testing.RemovalListeners.ConsumingRemovalListener;
 import com.github.benmanes.caffeine.testing.ConcurrentTestHarness;
 import com.google.common.collect.Iterables;
+import com.google.common.collect.Streams;
 import com.google.common.util.concurrent.MoreExecutors;
 import com.google.common.util.concurrent.testing.TestingExecutors;
 
@@ -508,6 +511,15 @@ enum Loader implements CacheLoader<Integer, Integer> {
         return result;
       }
     },
+    /** A bulk-only loader that loads only keys that were not requested. */
+    BULK_DIFFERENT {
+      @Override public Integer load(Integer key) {
+        throw new UnsupportedOperationException();
+      }
+      @Override public Map<Integer, Integer> loadAll(Iterable<? extends Integer> keys) {
+        return Streams.stream(keys).collect(toMap(k -> -k, identity()));
+      }
+    },
     /** A bulk-only loader that loads more than requested. */
     BULK_NEGATIVE_EXCEEDS {
       @Override public Integer load(Integer key) {