skip refreshing if the async cache entry is still loading (fixes #1478)

ben-manes · Jan 13, 2025 · 25405d6 · 25405d6
1 parent 2d93f2b
commit 25405d6
Show file tree

Hide file tree

Showing 16 changed files with 108 additions and 30 deletions.
diff --git a/.github/workflows/actionlint.yml b/.github/workflows/actionlint.yml
@@ -16,7 +16,7 @@ jobs:
             github.com:443
       - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
       - name: actionlint
-        uses: reviewdog/action-actionlint@af17f9e3640ac863dbcc515d45f5f35d708d0faf # v1.62.0
+        uses: reviewdog/action-actionlint@f3dcc52bc6039e5d736486952379dce3e869e8a2 # v1.63.0
         env:
           SHELLCHECK_OPTS: -e SC2001 -e SC2035 -e SC2046 -e SC2061 -e SC2086 -e SC2156
         with:

diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
@@ -191,7 +191,7 @@ jobs:
           find . -path */jacoco/*.exec -o -path */results/*.xml
           | tar czf ${{ env.ARTIFACT_NAME }}.tar.gz --files-from -
       - name: Upload test results
-        uses: actions/upload-artifact@6f51ac03b9356f520e9adb1b1b7802705f340c2b # v4.5.0
+        uses: actions/upload-artifact@65c4c4a1ddee5b72f698fdd19549f0f0fb45cf08 # v4.6.0
         if: always() && (env.JAVA_VERSION == env.PUBLISH_JDK)
         with:
           retention-days: 1

diff --git a/.github/workflows/codacy.yml b/.github/workflows/codacy.yml
@@ -47,7 +47,7 @@ jobs:
         if: steps.check_files.outputs.files_exists == 'true'
         run: jq -c '.runs |= unique_by({tool, invocations, results})' < results.sarif > codacy.sarif
       - name: Upload result to GitHub Code Scanning
-        uses: github/codeql-action/upload-sarif@48ab28a6f5dbc2a99bf1e0131198dd8f1df78169 # v3.28.0
+        uses: github/codeql-action/upload-sarif@b6a472f63d85b9c78a3ac5e89422239fc15e9b3c # v3.28.1
         if: steps.check_files.outputs.files_exists == 'true'
         continue-on-error: true
         with:

diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml
@@ -56,12 +56,12 @@ jobs:
           java: ${{ env.JAVA_VERSION }}
           cache-encryption-key: ${{ secrets.GRADLE_ENCRYPTION_KEY }}
       - name: Initialize CodeQL (Actions)
-        uses: github/codeql-action/init@48ab28a6f5dbc2a99bf1e0131198dd8f1df78169 # v3.28.0
+        uses: github/codeql-action/init@b6a472f63d85b9c78a3ac5e89422239fc15e9b3c # v3.28.1
         if: ${{ matrix.language == 'actions' }}
         with:
           languages: actions
       - name: Initialize CodeQL (Java)
-        uses: github/codeql-action/init@48ab28a6f5dbc2a99bf1e0131198dd8f1df78169 # v3.28.0
+        uses: github/codeql-action/init@b6a472f63d85b9c78a3ac5e89422239fc15e9b3c # v3.28.1
         if: ${{ matrix.language == 'java' }}
         with:
           queries: >
@@ -78,6 +78,6 @@ jobs:
           config: |
             threat-models: local
       - name: Autobuild
-        uses: github/codeql-action/autobuild@48ab28a6f5dbc2a99bf1e0131198dd8f1df78169 # v3.28.0
+        uses: github/codeql-action/autobuild@b6a472f63d85b9c78a3ac5e89422239fc15e9b3c # v3.28.1
       - name: Perform CodeQL Analysis
-        uses: github/codeql-action/analyze@48ab28a6f5dbc2a99bf1e0131198dd8f1df78169 # v3.28.0
+        uses: github/codeql-action/analyze@b6a472f63d85b9c78a3ac5e89422239fc15e9b3c # v3.28.1
diff --git a/.github/workflows/dependency-check.yml b/.github/workflows/dependency-check.yml
@@ -61,7 +61,7 @@ jobs:
         with:
           files: build/reports/dependency-check-report.sarif
       - name: Upload result to GitHub Code Scanning
-        uses: github/codeql-action/upload-sarif@48ab28a6f5dbc2a99bf1e0131198dd8f1df78169 # v3.28.0
+        uses: github/codeql-action/upload-sarif@b6a472f63d85b9c78a3ac5e89422239fc15e9b3c # v3.28.1
         if: steps.check_files.outputs.files_exists == 'true'
         with:
           sarif_file: build/reports/dependency-check-report.sarif
diff --git a/.github/workflows/devskim.yml b/.github/workflows/devskim.yml
@@ -31,6 +31,6 @@ jobs:
       - name: Run DevSkim scanner
         uses: microsoft/DevSkim-Action@914fa647b406c387000300b2f09bb28691be2b6d # v1.0.14
       - name: Upload DevSkim scan results to GitHub Security tab
-        uses: github/codeql-action/upload-sarif@48ab28a6f5dbc2a99bf1e0131198dd8f1df78169 # v3.28.0
+        uses: github/codeql-action/upload-sarif@b6a472f63d85b9c78a3ac5e89422239fc15e9b3c # v3.28.1
         with:
           sarif_file: devskim-results.sarif
diff --git a/.github/workflows/qodana.yml b/.github/workflows/qodana.yml
@@ -70,6 +70,6 @@ jobs:
           upload-result: true
           github-token: ${{ secrets.GITHUB_TOKEN }}
       - name: Upload SARIF file for GitHub Advanced Security Dashboard
-        uses: github/codeql-action/upload-sarif@48ab28a6f5dbc2a99bf1e0131198dd8f1df78169 # v3.28.0
+        uses: github/codeql-action/upload-sarif@b6a472f63d85b9c78a3ac5e89422239fc15e9b3c # v3.28.1
         with:
           sarif_file: ${{ runner.temp }}/qodana/results/qodana.sarif.json
diff --git a/.github/workflows/scorecards-analysis.yml b/.github/workflows/scorecards-analysis.yml
@@ -52,12 +52,12 @@ jobs:
           results_file: results.sarif
           repo_token: ${{ secrets.SCORECARD_READ_TOKEN }}
       - name: Upload artifact
-        uses: actions/upload-artifact@6f51ac03b9356f520e9adb1b1b7802705f340c2b # v4.5.0
+        uses: actions/upload-artifact@65c4c4a1ddee5b72f698fdd19549f0f0fb45cf08 # v4.6.0
         with:
           name: SARIF file
           path: results.sarif
           retention-days: 5
       - name: Upload to code-scanning
-        uses: github/codeql-action/upload-sarif@48ab28a6f5dbc2a99bf1e0131198dd8f1df78169 # v3.28.0
+        uses: github/codeql-action/upload-sarif@b6a472f63d85b9c78a3ac5e89422239fc15e9b3c # v3.28.1
         with:
           sarif_file: results.sarif
diff --git a/.github/workflows/semgrep.yml b/.github/workflows/semgrep.yml
@@ -31,7 +31,7 @@ jobs:
         with:
           files: results.sarif
       - name: Upload SARIF file for GitHub Advanced Security Dashboard
-        uses: github/codeql-action/upload-sarif@48ab28a6f5dbc2a99bf1e0131198dd8f1df78169 # v3.28.0
+        uses: github/codeql-action/upload-sarif@b6a472f63d85b9c78a3ac5e89422239fc15e9b3c # v3.28.1
         if: steps.check_files.outputs.files_exists == 'true'
         continue-on-error: true
         with:

diff --git a/.github/workflows/snyk.yml b/.github/workflows/snyk.yml
@@ -42,7 +42,7 @@ jobs:
         with:
           files: snyk.sarif
       - name: Upload result to GitHub Code Scanning
-        uses: github/codeql-action/upload-sarif@48ab28a6f5dbc2a99bf1e0131198dd8f1df78169 # v3.28.0
+        uses: github/codeql-action/upload-sarif@b6a472f63d85b9c78a3ac5e89422239fc15e9b3c # v3.28.1
         if: steps.check_files.outputs.files_exists == 'true'
         with:
           sarif_file: snyk.sarif

diff --git a/.github/workflows/trivy.yml b/.github/workflows/trivy.yml
@@ -37,7 +37,7 @@ jobs:
         with:
           files: results.sarif
       - name: Upload result to GitHub Code Scanning
-        uses: github/codeql-action/upload-sarif@48ab28a6f5dbc2a99bf1e0131198dd8f1df78169 # v3.28.0
+        uses: github/codeql-action/upload-sarif@b6a472f63d85b9c78a3ac5e89422239fc15e9b3c # v3.28.1
         if: steps.check_files.outputs.files_exists == 'true'
         with:
             sarif_file: results.sarif
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
@@ -323,8 +323,8 @@ public boolean isAsync() {
   }
 
   /** Returns if the node's value is currently being computed asynchronously. */
-  final boolean isComputingAsync(Node<?, ?> node) {
-    return isAsync && !Async.isReady((CompletableFuture<?>) node.getValue());
+  final boolean isComputingAsync(@Nullable V value) {
+    return isAsync && !Async.isReady((CompletableFuture<?>) value);
   }
 
   @GuardedBy("evictionLock")
@@ -1003,7 +1003,7 @@ long getExpirationDelay(long now) {
   /** Returns if the entry has expired. */
   @SuppressWarnings("ShortCircuitBoolean")
   boolean hasExpired(Node<K, V> node, long now) {
-    if (isComputingAsync(node)) {
+    if (isComputingAsync(node.getValue())) {
       return false;
     }
     return (expiresAfterAccess() && (now - node.getAccessTime() >= expiresAfterAccessNanos()))
@@ -1318,7 +1318,8 @@ boolean skipReadBuffer() {
     ConcurrentMap<Object, CompletableFuture<?>> refreshes;
     if (((now - writeTime) > refreshAfterWriteNanos()) && (keyReference != null)
         && ((key = node.getKey()) != null) && ((oldValue = node.getValue()) != null)
-        && ((writeTime & 1L) == 0L) && !(refreshes = refreshes()).containsKey(keyReference)
+        && !isComputingAsync(oldValue) && ((writeTime & 1L) == 0L)
+        && !(refreshes = refreshes()).containsKey(keyReference)
         && node.isAlive() && node.casWriteTime(writeTime, refreshWriteTime)) {
       long[] startTime = new long[1];
       @SuppressWarnings({"rawtypes", "unchecked"})
@@ -1335,8 +1336,8 @@ boolean skipReadBuffer() {
                 var refresh = cacheLoader.asyncReload(key, future.join(), executor);
                 refreshFuture[0] = requireNonNull(refresh, "Null future");
               } else {
-                // no-op if load is pending
-                return future;
+                // no-op if the future's completion state was modified (e.g. obtrude methods)
+                return null;
               }
             } else {
               @SuppressWarnings("NullAway")
@@ -1930,7 +1931,7 @@ public void run() {
       }
 
       // Ensure that in-flight async computation cannot expire (reset on a completion callback)
-      if (isComputingAsync(node)) {
+      if (isComputingAsync(node.getValue())) {
         synchronized (node) {
           if (!Async.isReady((CompletableFuture<?>) node.getValue())) {
             long expirationTime = expirationTicker().read() + ASYNC_EXPIRY;
@@ -2199,7 +2200,7 @@ public boolean containsValue(Object value) {
       return null;
     }
 
-    if (!isComputingAsync(node)) {
+    if (!isComputingAsync(value)) {
       @SuppressWarnings("unchecked")
       var castedKey = (K) key;
       setAccessTime(node, now);
@@ -2256,7 +2257,7 @@ public Map<K, V> getAllPresent(Iterable<? extends K> keys) {
       if ((node == null) || ((value = node.getValue()) == null) || hasExpired(node, now)) {
         iter.remove();
       } else {
-        if (!isComputingAsync(node)) {
+        if (!isComputingAsync(value)) {
           tryExpireAfterRead(node, entry.getKey(), value, expiry(), now);
           setAccessTime(node, now);
         }
@@ -2319,7 +2320,7 @@ public void putAll(Map<? extends K, ? extends V> map) {
           // An optimistic fast path to avoid unnecessary locking
           V currentValue = prior.getValue();
           if ((currentValue != null) && !hasExpired(prior, now)) {
-            if (!isComputingAsync(prior)) {
+            if (!isComputingAsync(currentValue)) {
               tryExpireAfterRead(prior, key, currentValue, expiry(), now);
               setAccessTime(prior, now);
             }
@@ -2331,7 +2332,7 @@ public void putAll(Map<? extends K, ? extends V> map) {
         // An optimistic fast path to avoid unnecessary locking
         V currentValue = prior.getValue();
         if ((currentValue != null) && !hasExpired(prior, now)) {
-          if (!isComputingAsync(prior)) {
+          if (!isComputingAsync(currentValue)) {
             tryExpireAfterRead(prior, key, currentValue, expiry(), now);
             setAccessTime(prior, now);
           }
@@ -2655,7 +2656,7 @@ public void replaceAll(BiFunction<? super K, ? super V, ? extends V> function) {
     if (node != null) {
       V value = node.getValue();
       if ((value != null) && !hasExpired(node, now)) {
-        if (!isComputingAsync(node)) {
+        if (!isComputingAsync(value)) {
           tryExpireAfterRead(node, key, value, expiry(), now);
           setAccessTime(node, now);
         }
@@ -2749,7 +2750,7 @@ public void replaceAll(BiFunction<? super K, ? super V, ? extends V> function) {
       return null;
     }
     if (newValue[0] == null) {
-      if (!isComputingAsync(node)) {
+      if (!isComputingAsync(oldValue[0])) {
         tryExpireAfterRead(node, key, oldValue[0], expiry(), now[0]);
         setAccessTime(node, now[0]);
       }

diff --git a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/BoundedLocalCacheTest.java b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/BoundedLocalCacheTest.java
@@ -30,6 +30,7 @@
 import static com.github.benmanes.caffeine.cache.RemovalCause.EXPLICIT;
 import static com.github.benmanes.caffeine.cache.RemovalCause.REPLACED;
 import static com.github.benmanes.caffeine.cache.RemovalCause.SIZE;
+import static com.github.benmanes.caffeine.cache.testing.AsyncCacheSubject.assertThat;
 import static com.github.benmanes.caffeine.cache.testing.CacheContext.intern;
 import static com.github.benmanes.caffeine.cache.testing.CacheContextSubject.assertThat;
 import static com.github.benmanes.caffeine.cache.testing.CacheSpec.Expiration.AFTER_ACCESS;
@@ -2276,6 +2277,7 @@ public void expirationDelay_varTime(BoundedLocalCache<Int, Int> cache, CacheCont
 
   /* --------------- Refresh --------------- */
 
+  @CheckNoEvictions
   @Test(dataProvider = "caches") // Issue #715
   @CacheSpec(implementation = Implementation.Caffeine, population = Population.EMPTY,
       refreshAfterWrite = Expire.ONE_MINUTE, executor = CacheExecutor.THREADED,
@@ -2337,6 +2339,7 @@ public void refreshIfNeeded_liveliness(CacheContext context) {
     }
   }
 
+  @CheckNoEvictions
   @Test(dataProvider = "caches", groups = "isolated")
   @CacheSpec(implementation = Implementation.Caffeine, population = Population.EMPTY,
       refreshAfterWrite = Expire.ONE_MINUTE, executor = CacheExecutor.THREADED,
@@ -2389,6 +2392,48 @@ public CompletableFuture<Int> asyncReload(Int key, Int oldValue, Executor execut
     await().untilAsserted(() -> assertThat(cache).containsEntry(context.absentKey(), newValue));
   }
 
+  @CheckNoEvictions
+  @Test(dataProvider = "caches")
+  @CacheSpec(loader = Loader.ASYNC_INCOMPLETE,
+      refreshAfterWrite = Expire.ONE_MINUTE, expireAfterWrite = Expire.FOREVER)
+  public void refreshIfNeeded_slowLoad_obtrudeToNull(
+      AsyncLoadingCache<Int, Int> cache, CacheContext context) throws Exception {
+    var future = CompletableFuture.completedFuture(context.absentKey());
+    var localCache = asBoundedLocalCache(cache);
+    cache.put(context.absentKey(), future);
+
+    var lookupKey = localCache.nodeFactory.newLookupKey(context.absentKey());
+    var node = requireNonNull(localCache.data.get(lookupKey));
+    var task = new Future<?>[1];
+
+    localCache.refreshes().compute(node.getKeyReference(), (k, v) -> {
+      assertThat(v).isNull();
+
+      context.ticker().advance(Duration.ofHours(1));
+      var reader = new AtomicReference<Thread>();
+      task[0] = ConcurrentTestHarness.submit(() -> {
+        reader.set(Thread.currentThread());
+        var future2 = cache.getIfPresent(context.absentKey());
+        assertThat(future2).isSameInstanceAs(future);
+        assertThat(future2).succeedsWith(null);
+      });
+
+      var threadState = EnumSet.of(BLOCKED, WAITING);
+      await().until(() -> {
+        var thread = reader.get();
+        return (thread != null) && threadState.contains(thread.getState());
+      });
+      future.obtrudeValue(null);
+      return null;
+    });
+
+    task[0].get(1, TimeUnit.MINUTES);
+    cache.asMap().remove(context.absentKey(), future); // obtruded values are not discarded
+    await().untilAsserted(() -> assertThat(cache).containsExactlyEntriesIn(context.original()));
+    assertThat(localCache.refreshes()).isEmpty();
+  }
+
+  @CheckNoEvictions
   @Test(dataProvider = "caches", groups = "isolated")
   @CacheSpec(population = Population.EMPTY, executor = CacheExecutor.THREADED,
       compute = Compute.ASYNC, stats = Stats.DISABLED)
@@ -2913,6 +2958,12 @@ static <K, V> BoundedLocalCache<K, V> asBoundedLocalCache(Cache<K, V> cache) {
     return (BoundedLocalCache<K, V>) cache.asMap();
   }
 
+  static <K, V> BoundedLocalCache<K, CompletableFuture<V>> asBoundedLocalCache(
+      AsyncCache<K, V> cache) {
+    var localCache = (LocalAsyncCache<K, V>) cache;
+    return (BoundedLocalCache<K, CompletableFuture<V>>) localCache.cache();
+  }
+
   static final class CustomBoundedLocalCache<K, V> extends BoundedLocalCache<K, V> {
     @SuppressWarnings("unchecked")
     CustomBoundedLocalCache(Caffeine<K, V> builder,

diff --git a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/RefreshAfterWriteTest.java b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/RefreshAfterWriteTest.java
@@ -396,6 +396,7 @@ public void refreshIfNeeded_error_log(CacheContext context) {
         .hasSize(1);
   }
 
+  @CheckNoEvictions
   @Test(dataProvider = "caches")
   @CacheSpec(implementation = Implementation.Caffeine,
       population = Population.EMPTY, refreshAfterWrite = Expire.ONE_MINUTE)
@@ -433,6 +434,30 @@ public void refreshIfNeeded_nullFuture(CacheContext context) {
     assertThat(cache).containsEntry(context.absentKey(), context.absentValue());
   }
 
+  @CheckNoEvictions
+  @Test(dataProvider = "caches")
+  @CacheSpec(loader = Loader.ASYNC_INCOMPLETE,
+      refreshAfterWrite = Expire.ONE_MINUTE, expireAfterWrite = Expire.FOREVER)
+  public void refreshIfNeeded_slowLoad(AsyncLoadingCache<Int, Int> cache, CacheContext context) {
+    cache.synchronous().asMap().put(context.absentKey(), context.absentKey());
+
+    context.ticker().advance(Duration.ofHours(1));
+    cache.put(context.absentKey(), new CompletableFuture<>());
+
+    context.ticker().advance(Duration.ofHours(1));
+    var future = requireNonNull(cache.getIfPresent(context.absentKey()));
+    assertThat(future).isNotDone();
+
+    assertThat(cache.synchronous().policy().refreshes()).isEmpty();
+
+    future.complete(context.absentKey().negate());
+    assertThat(cache.synchronous().policy().refreshes()).isEmpty();
+
+    var expectedMap = new HashMap<>(context.original());
+    expectedMap.put(context.absentKey(), future.join());
+    assertThat(cache).containsExactlyEntriesIn(expectedMap);
+  }
+
   /* --------------- getIfPresent --------------- */
 
   @CheckNoEvictions

diff --git a/gradle/config/spotbugs/exclude.xml b/gradle/config/spotbugs/exclude.xml
@@ -512,6 +512,7 @@
       EC_NULL_ARG,
       PREDICTABLE_RANDOM,
       RV_RETURN_VALUE_IGNORED,
+      PDP_POORLY_DEFINED_PARAMETER,
       HE_HASHCODE_USE_OBJECT_EQUALS,
       ITC_INHERITANCE_TYPE_CHECKING,
       SIC_INNER_SHOULD_BE_STATIC_ANON,

diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml
@@ -18,7 +18,7 @@ commons-text = "1.13.0"
 concurrentlinkedhashmap = "1.4.2"
 config = "1.4.3"
 coveralls = "2.12.2"
-dependency-check = "11.1.1"
+dependency-check = "12.0.0"
 eclipse-collections = "12.0.0.M3"
 ehcache3 = "3.10.8"
 errorprone = "2.36.0"
@@ -69,7 +69,7 @@ lincheck = "2.34"
 mockito = "5.15.2"
 nexus-publish = "2.0.0"
 nullaway = "0.12.3"
-nullaway-plugin = "2.1.0"
+nullaway-plugin = "2.2.0"
 okhttp-bom = "4.12.0"
 okio-bom = "3.10.2"
 osgi-annotations = "1.5.1"