Fail if the refresh future is null

While the computed value may be null, the future itself may not be. This is treated like any other exception to compute the future by bubbling up the exception to the caller. For an explicit refresh the user receives it directly, whereas for refreshAfterWrite it is logged and swallowed. Previously the null future might be ignored (as a no-op) or trigger an infinite retry loop.
deepstupid · Jul 23, 2022 · 2f271e9 · 2f271e9
1 parent 7841372
commit 2f271e9
Show file tree

Hide file tree

Showing 6 changed files with 184 additions and 5 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
@@ -1274,15 +1274,15 @@ boolean skipReadBuffer() {
               if (Async.isReady(future)) {
                 @SuppressWarnings("NullAway")
                 var refresh = cacheLoader.asyncReload(key, future.join(), executor);
-                refreshFuture[0] = refresh;
+                refreshFuture[0] = requireNonNull(refresh, "Null future");
               } else {
                 // no-op if load is pending
                 return future;
               }
             } else {
               @SuppressWarnings("NullAway")
               var refresh = cacheLoader.asyncReload(key, oldValue, executor);
-              refreshFuture[0] = refresh;
+              refreshFuture[0] = requireNonNull(refresh, "Null future");
             }
             return refreshFuture[0];
           } catch (InterruptedException e) {

diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalAsyncLoadingCache.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalAsyncLoadingCache.java
@@ -264,7 +264,9 @@ public CompletableFuture<Map<K, V>> refreshAll(Iterable<? extends K> keys) {
         refreshed[0] = true;
         startTime[0] = asyncCache.cache().statsTicker().read();
         try {
-          return asyncCache.cacheLoader.asyncReload(key, oldValue, asyncCache.cache().executor());
+          var reloadFuture = asyncCache.cacheLoader.asyncReload(
+              key, oldValue, asyncCache.cache().executor());
+          return requireNonNull(reloadFuture, "Null future");
         } catch (RuntimeException e) {
           throw e;
         } catch (InterruptedException e) {
@@ -302,7 +304,7 @@ public CompletableFuture<Map<K, V>> refreshAll(Iterable<? extends K> keys) {
                 // If the entry is absent then discard the refresh and maybe notifying the listener
                 discard[0] = (newValue != null);
                 return null;
-              } else if (currentValue == newValue) {
+              } else if ((currentValue == newValue) || (currentValue == castedFuture)) {
                 // If the reloaded value is the same instance then no-op
                 return currentValue;
               } else if (newValue == Async.getIfReady((CompletableFuture<?>) currentValue)) {

diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalLoadingCache.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalLoadingCache.java
@@ -116,7 +116,7 @@ default CompletableFuture<V> refresh(K key) {
         var refreshFuture = (oldValue[0] == null)
             ? cacheLoader().asyncLoad(key, cache().executor())
             : cacheLoader().asyncReload(key, oldValue[0], cache().executor());
-        reloading[0] = refreshFuture;
+        reloading[0] = requireNonNull(refreshFuture, "Null future");
         return refreshFuture;
       } catch (RuntimeException e) {
         throw e;

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
@@ -448,6 +448,29 @@ public void refresh(CacheContext context) {
     await().untilAsserted(() -> assertThat(cache).containsEntry(key, key.negate()));
   }
 
+  @Test(dataProvider = "caches", expectedExceptions = NullPointerException.class)
+  @CacheSpec(population = Population.EMPTY, compute = Compute.ASYNC)
+  public void refresh_nullFuture_load(CacheContext context) {
+    var cache = context.buildAsync((Int key, Executor executor) -> null);
+    cache.synchronous().refresh(context.absentKey());
+  }
+
+  @Test(dataProvider = "caches", expectedExceptions = NullPointerException.class)
+  @CacheSpec(population = Population.EMPTY, compute = Compute.ASYNC)
+  public void refresh_nullFuture_reload(CacheContext context) {
+    var cache = context.buildAsync(new AsyncCacheLoader<Int, Int>() {
+      @Override public CompletableFuture<Int> asyncLoad(Int key, Executor executor) {
+        throw new IllegalStateException();
+      }
+      @Override public CompletableFuture<Int> asyncReload(
+          Int key, Int oldValue, Executor executor) {
+        return null;
+      }
+    });
+    cache.synchronous().put(context.absentKey(), context.absentValue());
+    cache.synchronous().refresh(context.absentKey());
+  }
+
   @Test(dataProvider = "caches", timeOut = 5_000) // Issue #69
   @CacheSpec(population = Population.EMPTY,
       compute = Compute.ASYNC, executor = CacheExecutor.THREADED)
@@ -507,6 +530,66 @@ public void refresh_interrupted(AsyncLoadingCache<Int, Int> cache, CacheContext
     }
   }
 
+  @CacheSpec
+  @Test(dataProvider = "caches")
+  public void refresh_current_inFlight(AsyncLoadingCache<Int, Int> cache, CacheContext context) {
+    var future = new CompletableFuture<Int>();
+    cache.put(context.absentKey(), future);
+    cache.synchronous().refresh(context.absentKey());
+    assertThat(cache).containsEntry(context.absentKey(), future);
+    assertThat(cache.synchronous().policy().refreshes()).isEmpty();
+    future.complete(context.absentValue());
+  }
+
+  @Test(dataProvider = "caches")
+  @CacheSpec(compute = Compute.ASYNC, removalListener = Listener.CONSUMING)
+  public void refresh_current_sameInstance(CacheContext context) {
+    var future = context.absentValue().asFuture();
+    var cache = context.buildAsync((key, executor) -> future);
+
+    cache.put(context.absentKey(), future);
+    cache.synchronous().refresh(context.absentKey());
+    assertThat(context).notifications().isEmpty();
+  }
+
+  @CacheSpec
+  @Test(dataProvider = "caches")
+  public void refresh_current_failed(AsyncLoadingCache<Int, Int> cache, CacheContext context) {
+    var future = context.absentValue().asFuture();
+    cache.put(context.absentKey(), future);
+
+    future.obtrudeException(new Exception());
+    assertThat(cache.asMap()).containsKey(context.absentKey());
+
+    cache.synchronous().refresh(context.absentKey());
+    assertThat(cache).containsEntry(context.absentKey(), context.absentValue());
+  }
+
+  @Test(dataProvider = "caches")
+  @CacheSpec(compute = Compute.ASYNC,
+      removalListener = Listener.CONSUMING, executor = CacheExecutor.THREADED)
+  public void refresh_current_removed(CacheContext context) {
+    var started = new AtomicBoolean();
+    var done = new AtomicBoolean();
+    var cache = context.buildAsync((Int key) -> {
+      started.set(true);
+      await().untilTrue(done);
+      return key;
+    });
+
+    cache.put(context.absentKey(), context.absentValue().asFuture());
+    cache.synchronous().refresh(context.absentKey());
+    await().untilTrue(started);
+
+    cache.synchronous().invalidate(context.absentKey());
+    done.set(true);
+
+    await().untilAsserted(() -> {
+      assertThat(context).removalNotifications().containsExactlyValues(
+          context.absentKey(), context.absentValue());
+    });
+  }
+
   /* --------------- AsyncCacheLoader --------------- */
 
   @Test(expectedExceptions = UnsupportedOperationException.class)

diff --git a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/LoadingCacheTest.java b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/LoadingCacheTest.java
@@ -930,6 +930,36 @@ public void refresh_error_log(CacheContext context) throws Exception {
     assertThat(event.getLevel()).isEqualTo(WARN);
   }
 
+  @Test(dataProvider = "caches", expectedExceptions = NullPointerException.class)
+  @CacheSpec(implementation = Implementation.Caffeine, population = Population.EMPTY)
+  public void refresh_nullFuture_load(CacheContext context) {
+    var cache = context.build(new CacheLoader<Int, Int>() {
+      @Override public Int load(Int key) {
+        throw new IllegalStateException();
+      }
+      @Override public CompletableFuture<Int> asyncLoad(Int key, Executor executor) {
+        return null;
+      }
+    });
+    cache.refresh(context.absentKey());
+  }
+
+  @Test(dataProvider = "caches", expectedExceptions = NullPointerException.class)
+  @CacheSpec(implementation = Implementation.Caffeine, population = Population.EMPTY)
+  public void refresh_nullFuture_reload(CacheContext context) {
+    var cache = context.build(new CacheLoader<Int, Int>() {
+      @Override public Int load(Int key) {
+        throw new IllegalStateException();
+      }
+      @Override public CompletableFuture<Int> asyncReload(
+          Int key, Int oldValue, Executor executor) {
+        return null;
+      }
+    });
+    cache.put(context.absentKey(), context.absentValue());
+    cache.refresh(context.absentKey());
+  }
+
   /* --------------- refreshAll --------------- */
 
   @CheckNoEvictions @CheckNoStats
@@ -1012,6 +1042,36 @@ public void refreshAll_cancel(LoadingCache<Int, Int> cache, CacheContext context
     assertThat(cache).containsExactlyEntriesIn(context.original());
   }
 
+  @Test(dataProvider = "caches", expectedExceptions = NullPointerException.class)
+  @CacheSpec(implementation = Implementation.Caffeine, population = Population.EMPTY)
+  public void refreshAll_nullFuture_load(CacheContext context) {
+    var cache = context.build(new CacheLoader<Int, Int>() {
+      @Override public Int load(Int key) {
+        throw new IllegalStateException();
+      }
+      @Override public CompletableFuture<Int> asyncLoad(Int key, Executor executor) {
+        return null;
+      }
+    });
+    cache.refreshAll(context.absent().keySet());
+  }
+
+  @Test(dataProvider = "caches", expectedExceptions = NullPointerException.class)
+  @CacheSpec(implementation = Implementation.Caffeine, population = Population.EMPTY)
+  public void refreshAll_nullFuture_reload(CacheContext context) {
+    var cache = context.build(new CacheLoader<Int, Int>() {
+      @Override public Int load(Int key) {
+        throw new IllegalStateException();
+      }
+      @Override public CompletableFuture<Int> asyncReload(
+          Int key, Int oldValue, Executor executor) {
+        return null;
+      }
+    });
+    cache.put(context.absentKey(), context.absentValue());
+    cache.refreshAll(context.absent().keySet());
+  }
+
   /* --------------- CacheLoader --------------- */
 
   @Test(expectedExceptions = UnsupportedOperationException.class)

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
@@ -357,6 +357,40 @@ public void refreshIfNeeded_error_log(CacheContext context) {
     assertThat(event.getLevel()).isEqualTo(WARN);
   }
 
+  @Test(dataProvider = "caches")
+  @CacheSpec(implementation = Implementation.Caffeine,
+      population = Population.EMPTY, refreshAfterWrite = Expire.ONE_MINUTE)
+  public void refreshIfNeeded_nullFuture(CacheContext context) {
+    var refreshed = new AtomicBoolean();
+    CacheLoader<Int, Int> loader = new CacheLoader<Int, Int>() {
+      @Override public Int load(Int key) {
+        throw new IllegalStateException();
+      }
+      @Override public CompletableFuture<Int> asyncReload(
+          Int key, Int oldValue, Executor executor) {
+        refreshed.set(true);
+        return null;
+      }
+    };
+    TestLoggerFactory.getAllTestLoggers().values()
+        .forEach(logger -> logger.setEnabledLevels(INFO_LEVELS));
+
+    var cache = context.isAsync()
+        ? context.buildAsync(loader).synchronous()
+        : context.build(loader);
+    cache.put(context.absentKey(), context.absentValue());
+    context.ticker().advance(2, TimeUnit.MINUTES);
+    cache.get(context.absentKey());
+
+    var event = Iterables.getOnlyElement(TestLoggerFactory.getLoggingEvents());
+    assertThat(event.getThrowable().orElseThrow()).isInstanceOf(NullPointerException.class);
+    assertThat(event.getLevel()).isEqualTo(WARN);
+
+    assertThat(refreshed.get()).isTrue();
+    assertThat(cache.policy().refreshes()).isEmpty();
+    assertThat(cache).containsEntry(context.absentKey(), context.absentValue());
+  }
+
   /* --------------- getIfPresent --------------- */
 
   @CheckNoEvictions