sync with guava's cache test changes

Note that since Caffeine's refresh is linearizable, we invert the test that asserts the stale reload populates the cache and instead we discard it as no longer reliable. The tests were generalized from using internal methods and moved into those test classes.
ben-manes · Sep 2, 2024 · c2015da · c2015da
1 parent 967f17a
commit c2015da
Show file tree

Hide file tree

Showing 7 changed files with 129 additions and 2 deletions.
diff --git a/.deepsource.toml b/.deepsource.toml
@@ -14,3 +14,7 @@ enabled = true
 [[analyzers]]
 name = "shell"
 enabled = true
+
+[[analyzers]]
+name = "test-coverage"
+enabled = true
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
@@ -221,8 +221,10 @@ jobs:
             ${{ env.ALLOWED_ENDPOINTS }}
             artifacts.codacy.com:443
             api.codacy.com:443
+            app.deepsource.com:443
             codecov.io:443
             coveralls.io:443
+            deepsource.io:443
             fastly.com:443
             nodejs.org:443
             raw.githubusercontent.com
@@ -260,6 +262,14 @@ jobs:
         with:
           project-token: ${{ secrets.CODACY_PROJECT_TOKEN }}
         continue-on-error: true
+      - name: Publish to DeepSource
+        env:
+          DEEPSOURCE_DSN: ${{ secrets.DEEPSOURCE_DSN }}
+        continue-on-error: true
+        run: |
+          curl https://deepsource.io/cli | sh
+          ./bin/deepsource report --analyzer test-coverage --key java \
+            --value-file ./build/reports/jacoco/jacocoFullReport/jacocoFullReport.xml
       - name: Publish to SonarQube
         uses: ./.github/actions/run-gradle
         env:

diff --git a/.github/workflows/scorecards-analysis.yml b/.github/workflows/scorecards-analysis.yml
@@ -25,6 +25,7 @@ jobs:
           disable-sudo: true
           egress-policy: block
           allowed-endpoints: >
+            api.deps.dev:443
             api.github.com:443
             api.osv.dev:443
             api.securityscorecards.dev:443

diff --git a/guava/src/test/java/com/github/benmanes/caffeine/guava/compatibility/CacheBuilderTest.java b/guava/src/test/java/com/github/benmanes/caffeine/guava/compatibility/CacheBuilderTest.java
@@ -547,6 +547,8 @@ public void testRemovalNotification_clear_basher() throws InterruptedException {
     // notification.
     assertEquals(expectedKeys, Sets.union(cache.asMap().keySet(), removalNotifications.keySet()));
     assertTrue(Collections.disjoint(cache.asMap().keySet(), removalNotifications.keySet()));
+    threadPool.shutdown();
+    threadPool.awaitTermination(300, SECONDS);
   }
 
   /**

diff --git a/guava/src/test/java/com/github/benmanes/caffeine/guava/compatibility/CacheLoadingTest.java b/guava/src/test/java/com/github/benmanes/caffeine/guava/compatibility/CacheLoadingTest.java
@@ -24,12 +24,14 @@
 import static java.util.Arrays.asList;
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
 
+import java.time.Duration;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.Callable;
 import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Executors;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
@@ -54,6 +56,7 @@
 import com.google.common.util.concurrent.ExecutionError;
 import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.ListeningExecutorService;
 import com.google.common.util.concurrent.MoreExecutors;
 import com.google.common.util.concurrent.UncheckedExecutionException;
 import com.google.common.util.concurrent.Uninterruptibles;
@@ -2235,6 +2238,55 @@ public String load(String key) {
     assertEquals(refreshKey + suffix, map.get(refreshKey));
   }
 
+  @SuppressWarnings({"CheckReturnValue", "FutureReturnValueIgnored"})
+  public void testLongAsyncRefresh() throws Exception {
+    FakeTicker ticker = new FakeTicker();
+    AtomicInteger counter = new AtomicInteger();
+    CountDownLatch reloadStarted = new CountDownLatch(1);
+    CountDownLatch reloadExpired = new CountDownLatch(1);
+    CountDownLatch reloadCompleted = new CountDownLatch(1);
+
+    ListeningExecutorService refreshExecutor = MoreExecutors.listeningDecorator(
+        Executors.newSingleThreadExecutor());
+    try {
+      Caffeine<Object, Object> builder = Caffeine.newBuilder()
+          .expireAfterWrite(100, MILLISECONDS)
+          .refreshAfterWrite(5, MILLISECONDS)
+          .executor(refreshExecutor)
+          .ticker(ticker::read);
+
+      CacheLoader<String, String> loader =
+          new CacheLoader<String, String>() {
+            @Override public String load(String key) {
+              return key + "Load-" + counter.incrementAndGet();
+            }
+            @Override public ListenableFuture<String> reload(String key, String oldValue) {
+              ListenableFuture<String> future = refreshExecutor.submit(() -> {
+                reloadStarted.countDown();
+                reloadExpired.await();
+                return key + "Reload";
+              });
+              future.addListener(reloadCompleted::countDown, refreshExecutor);
+              return future;
+            }
+          };
+      LoadingCache<String, String> cache = CaffeinatedGuava.build(builder, loader);
+
+      assertThat(cache.get("test")).isEqualTo("testLoad-1");
+
+      ticker.advance(10, MILLISECONDS); // so that the next call will trigger refresh
+      assertThat(cache.get("test")).isEqualTo("testLoad-1");
+      reloadStarted.await();
+      ticker.advance(500, MILLISECONDS); // so that the entry expires during the reload
+      reloadExpired.countDown();
+
+      assertThat(cache.get("test")).isEqualTo("testLoad-2");
+    } finally {
+      refreshExecutor.shutdown();
+      refreshExecutor.awaitTermination(Duration.ofMinutes(1));
+    }
+  }
+
   static <T> Callable<T> throwing(final Exception exception) {
     return new Callable<T>() {
       @Override public T call() throws Exception {

diff --git a/guava/src/test/java/com/github/benmanes/caffeine/guava/compatibility/CacheManualTest.java b/guava/src/test/java/com/github/benmanes/caffeine/guava/compatibility/CacheManualTest.java
@@ -15,14 +15,21 @@
 package com.github.benmanes.caffeine.guava.compatibility;
 
 import static java.util.Arrays.asList;
-import junit.framework.TestCase;
+
+import java.util.concurrent.Callable;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
 
 import com.github.benmanes.caffeine.cache.Caffeine;
 import com.github.benmanes.caffeine.guava.CaffeinatedGuava;
 import com.google.common.cache.Cache;
 import com.google.common.cache.CacheStats;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.util.concurrent.UncheckedExecutionException;
+
+import junit.framework.TestCase;
 
 /**
  * @author Charles Fry
@@ -155,4 +162,55 @@ public void testGetAllPresent() {
     assertEquals(6, stats.hitCount());
   }
 
+  public void testLoadDifferentKeyInLoader() throws ExecutionException {
+    Cache<String, String> cache = CaffeinatedGuava.build(Caffeine.newBuilder());
+    String key1 = "key1";
+    String key2 = "key2";
+
+    // loads a different key, should work
+    assertEquals(key2, cache.get(key1, () -> cache.get(key2, () -> key2)));
+  }
+
+  public void testRecursiveLoad() throws InterruptedException {
+    Cache<String, String> cache = CaffeinatedGuava.build(Caffeine.newBuilder());
+    String key = "key";
+
+    // recursive load, this should fail
+    Callable<String> loader = () -> cache.get(key, () -> key);
+    testLoadThrows(key, cache, loader);
+  }
+
+  public void testRecursiveLoadWithProxy() throws InterruptedException {
+    String key = "key";
+    String otherKey = "otherKey";
+    Cache<String, String> cache = CaffeinatedGuava.build(Caffeine.newBuilder());
+
+    // recursive load (same as the initial one), this should fail
+    Callable<String> loader = () -> cache.get(key, () -> key);
+    // loads another key, is ok
+    Callable<String> proxyLoader = () -> cache.get(otherKey, loader);
+    testLoadThrows(key, cache, proxyLoader);
+  }
+
+  private void testLoadThrows(String key, Cache<String, String> cache, Callable<String> loader)
+      throws InterruptedException {
+    CountDownLatch doneSignal = new CountDownLatch(1);
+    Thread thread = new Thread(() -> {
+      try {
+        cache.get(key, loader);
+      } catch (UncheckedExecutionException | ExecutionException e) {
+        doneSignal.countDown();
+      }
+    });
+    thread.start();
+
+    boolean done = doneSignal.await(1, TimeUnit.SECONDS);
+    if (!done) {
+      StringBuilder builder = new StringBuilder();
+      for (StackTraceElement trace : thread.getStackTrace()) {
+        builder.append("\tat ").append(trace).append('\n');
+      }
+      fail(builder.toString());
+    }
+  }
 }
diff --git a/...main/java/com/github/benmanes/caffeine/cache/simulator/policy/greedy_dual/GdsfPolicy.java b/...main/java/com/github/benmanes/caffeine/cache/simulator/policy/greedy_dual/GdsfPolicy.java
@@ -210,7 +210,7 @@ public boolean equals(Object o) {
         return false;
       }
       var node = (Node) o;
-      return (key == node.key) && (priority == node.priority);
+      return compareTo(node) == 0;
     }
 
     @Override