Skip to content

Commit 940dca6

Browse files
ben-manesben-manes
committed
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.
1 parent db0614b commit 940dca6

6 files changed

Lines changed: 184 additions & 5 deletions

File tree

‎caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1274,15 +1274,15 @@ boolean skipReadBuffer() {
12741274
if (Async.isReady(future)) {
12751275
@SuppressWarnings("NullAway")
12761276
var refresh = cacheLoader.asyncReload(key, future.join(), executor);
1277-
refreshFuture[0] = refresh;
1277+
refreshFuture[0] = requireNonNull(refresh, "Null future");
12781278
} else {
12791279
// no-op if load is pending
12801280
return future;
12811281
}
12821282
} else {
12831283
@SuppressWarnings("NullAway")
12841284
var refresh = cacheLoader.asyncReload(key, oldValue, executor);
1285-
refreshFuture[0] = refresh;
1285+
refreshFuture[0] = requireNonNull(refresh, "Null future");
12861286
}
12871287
return refreshFuture[0];
12881288
} catch (InterruptedException e) {

‎caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalAsyncLoadingCache.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -264,7 +264,9 @@ public CompletableFuture<Map<K, V>> refreshAll(Iterable<? extends K> keys) {
264264
refreshed[0] = true;
265265
startTime[0] = asyncCache.cache().statsTicker().read();
266266
try {
267-
return asyncCache.cacheLoader.asyncReload(key, oldValue, asyncCache.cache().executor());
267+
var reloadFuture = asyncCache.cacheLoader.asyncReload(
268+
key, oldValue, asyncCache.cache().executor());
269+
return requireNonNull(reloadFuture, "Null future");
268270
} catch (RuntimeException e) {
269271
throw e;
270272
} catch (InterruptedException e) {
@@ -302,7 +304,7 @@ public CompletableFuture<Map<K, V>> refreshAll(Iterable<? extends K> keys) {
302304
// If the entry is absent then discard the refresh and maybe notifying the listener
303305
discard[0] = (newValue != null);
304306
return null;
305-
} else if (currentValue == newValue) {
307+
} else if ((currentValue == newValue) || (currentValue == castedFuture)) {
306308
// If the reloaded value is the same instance then no-op
307309
return currentValue;
308310
} else if (newValue == Async.getIfReady((CompletableFuture<?>) currentValue)) {

‎caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalLoadingCache.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ default CompletableFuture<V> refresh(K key) {
116116
var refreshFuture = (oldValue[0] == null)
117117
? cacheLoader().asyncLoad(key, cache().executor())
118118
: cacheLoader().asyncReload(key, oldValue[0], cache().executor());
119-
reloading[0] = refreshFuture;
119+
reloading[0] = requireNonNull(refreshFuture, "Null future");
120120
return refreshFuture;
121121
} catch (RuntimeException e) {
122122
throw e;

‎caffeine/src/test/java/com/github/benmanes/caffeine/cache/AsyncLoadingCacheTest.java‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -448,6 +448,29 @@ public void refresh(CacheContext context) {
448448
await().untilAsserted(() -> assertThat(cache).containsEntry(key, key.negate()));
449449
}
450450

451+
@Test(dataProvider = "caches", expectedExceptions = NullPointerException.class)
452+
@CacheSpec(population = Population.EMPTY, compute = Compute.ASYNC)
453+
public void refresh_nullFuture_load(CacheContext context) {
454+
var cache = context.buildAsync((Int key, Executor executor) -> null);
455+
cache.synchronous().refresh(context.absentKey());
456+
}
457+
458+
@Test(dataProvider = "caches", expectedExceptions = NullPointerException.class)
459+
@CacheSpec(population = Population.EMPTY, compute = Compute.ASYNC)
460+
public void refresh_nullFuture_reload(CacheContext context) {
461+
var cache = context.buildAsync(new AsyncCacheLoader<Int, Int>() {
462+
@Override public CompletableFuture<Int> asyncLoad(Int key, Executor executor) {
463+
throw new IllegalStateException();
464+
}
465+
@Override public CompletableFuture<Int> asyncReload(
466+
Int key, Int oldValue, Executor executor) {
467+
return null;
468+
}
469+
});
470+
cache.synchronous().put(context.absentKey(), context.absentValue());
471+
cache.synchronous().refresh(context.absentKey());
472+
}
473+
451474
@Test(dataProvider = "caches", timeOut = 5_000) // Issue #69
452475
@CacheSpec(population = Population.EMPTY,
453476
compute = Compute.ASYNC, executor = CacheExecutor.THREADED)
@@ -507,6 +530,66 @@ public void refresh_interrupted(AsyncLoadingCache<Int, Int> cache, CacheContext
507530
}
508531
}
509532

533+
@CacheSpec
534+
@Test(dataProvider = "caches")
535+
public void refresh_current_inFlight(AsyncLoadingCache<Int, Int> cache, CacheContext context) {
536+
var future = new CompletableFuture<Int>();
537+
cache.put(context.absentKey(), future);
538+
cache.synchronous().refresh(context.absentKey());
539+
assertThat(cache).containsEntry(context.absentKey(), future);
540+
assertThat(cache.synchronous().policy().refreshes()).isEmpty();
541+
future.complete(context.absentValue());
542+
}
543+
544+
@Test(dataProvider = "caches")
545+
@CacheSpec(compute = Compute.ASYNC, removalListener = Listener.CONSUMING)
546+
public void refresh_current_sameInstance(CacheContext context) {
547+
var future = context.absentValue().asFuture();
548+
var cache = context.buildAsync((key, executor) -> future);
549+
550+
cache.put(context.absentKey(), future);
551+
cache.synchronous().refresh(context.absentKey());
552+
assertThat(context).notifications().isEmpty();
553+
}
554+
555+
@CacheSpec
556+
@Test(dataProvider = "caches")
557+
public void refresh_current_failed(AsyncLoadingCache<Int, Int> cache, CacheContext context) {
558+
var future = context.absentValue().asFuture();
559+
cache.put(context.absentKey(), future);
560+
561+
future.obtrudeException(new Exception());
562+
assertThat(cache.asMap()).containsKey(context.absentKey());
563+
564+
cache.synchronous().refresh(context.absentKey());
565+
assertThat(cache).containsEntry(context.absentKey(), context.absentValue());
566+
}
567+
568+
@Test(dataProvider = "caches")
569+
@CacheSpec(compute = Compute.ASYNC,
570+
removalListener = Listener.CONSUMING, executor = CacheExecutor.THREADED)
571+
public void refresh_current_removed(CacheContext context) {
572+
var started = new AtomicBoolean();
573+
var done = new AtomicBoolean();
574+
var cache = context.buildAsync((Int key) -> {
575+
started.set(true);
576+
await().untilTrue(done);
577+
return key;
578+
});
579+
580+
cache.put(context.absentKey(), context.absentValue().asFuture());
581+
cache.synchronous().refresh(context.absentKey());
582+
await().untilTrue(started);
583+
584+
cache.synchronous().invalidate(context.absentKey());
585+
done.set(true);
586+
587+
await().untilAsserted(() -> {
588+
assertThat(context).removalNotifications().containsExactlyValues(
589+
context.absentKey(), context.absentValue());
590+
});
591+
}
592+
510593
/* --------------- AsyncCacheLoader --------------- */
511594

512595
@Test(expectedExceptions = UnsupportedOperationException.class)

‎caffeine/src/test/java/com/github/benmanes/caffeine/cache/LoadingCacheTest.java‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -930,6 +930,36 @@ public void refresh_error_log(CacheContext context) throws Exception {
930930
assertThat(event.getLevel()).isEqualTo(WARN);
931931
}
932932

933+
@Test(dataProvider = "caches", expectedExceptions = NullPointerException.class)
934+
@CacheSpec(implementation = Implementation.Caffeine, population = Population.EMPTY)
935+
public void refresh_nullFuture_load(CacheContext context) {
936+
var cache = context.build(new CacheLoader<Int, Int>() {
937+
@Override public Int load(Int key) {
938+
throw new IllegalStateException();
939+
}
940+
@Override public CompletableFuture<Int> asyncLoad(Int key, Executor executor) {
941+
return null;
942+
}
943+
});
944+
cache.refresh(context.absentKey());
945+
}
946+
947+
@Test(dataProvider = "caches", expectedExceptions = NullPointerException.class)
948+
@CacheSpec(implementation = Implementation.Caffeine, population = Population.EMPTY)
949+
public void refresh_nullFuture_reload(CacheContext context) {
950+
var cache = context.build(new CacheLoader<Int, Int>() {
951+
@Override public Int load(Int key) {
952+
throw new IllegalStateException();
953+
}
954+
@Override public CompletableFuture<Int> asyncReload(
955+
Int key, Int oldValue, Executor executor) {
956+
return null;
957+
}
958+
});
959+
cache.put(context.absentKey(), context.absentValue());
960+
cache.refresh(context.absentKey());
961+
}
962+
933963
/* --------------- refreshAll --------------- */
934964

935965
@CheckNoEvictions @CheckNoStats
@@ -1012,6 +1042,36 @@ public void refreshAll_cancel(LoadingCache<Int, Int> cache, CacheContext context
10121042
assertThat(cache).containsExactlyEntriesIn(context.original());
10131043
}
10141044

1045+
@Test(dataProvider = "caches", expectedExceptions = NullPointerException.class)
1046+
@CacheSpec(implementation = Implementation.Caffeine, population = Population.EMPTY)
1047+
public void refreshAll_nullFuture_load(CacheContext context) {
1048+
var cache = context.build(new CacheLoader<Int, Int>() {
1049+
@Override public Int load(Int key) {
1050+
throw new IllegalStateException();
1051+
}
1052+
@Override public CompletableFuture<Int> asyncLoad(Int key, Executor executor) {
1053+
return null;
1054+
}
1055+
});
1056+
cache.refreshAll(context.absent().keySet());
1057+
}
1058+
1059+
@Test(dataProvider = "caches", expectedExceptions = NullPointerException.class)
1060+
@CacheSpec(implementation = Implementation.Caffeine, population = Population.EMPTY)
1061+
public void refreshAll_nullFuture_reload(CacheContext context) {
1062+
var cache = context.build(new CacheLoader<Int, Int>() {
1063+
@Override public Int load(Int key) {
1064+
throw new IllegalStateException();
1065+
}
1066+
@Override public CompletableFuture<Int> asyncReload(
1067+
Int key, Int oldValue, Executor executor) {
1068+
return null;
1069+
}
1070+
});
1071+
cache.put(context.absentKey(), context.absentValue());
1072+
cache.refreshAll(context.absent().keySet());
1073+
}
1074+
10151075
/* --------------- CacheLoader --------------- */
10161076

10171077
@Test(expectedExceptions = UnsupportedOperationException.class)

‎caffeine/src/test/java/com/github/benmanes/caffeine/cache/RefreshAfterWriteTest.java‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,40 @@ public void refreshIfNeeded_error_log(CacheContext context) {
357357
assertThat(event.getLevel()).isEqualTo(WARN);
358358
}
359359

360+
@Test(dataProvider = "caches")
361+
@CacheSpec(implementation = Implementation.Caffeine,
362+
population = Population.EMPTY, refreshAfterWrite = Expire.ONE_MINUTE)
363+
public void refreshIfNeeded_nullFuture(CacheContext context) {
364+
var refreshed = new AtomicBoolean();
365+
CacheLoader<Int, Int> loader = new CacheLoader<Int, Int>() {
366+
@Override public Int load(Int key) {
367+
throw new IllegalStateException();
368+
}
369+
@Override public CompletableFuture<Int> asyncReload(
370+
Int key, Int oldValue, Executor executor) {
371+
refreshed.set(true);
372+
return null;
373+
}
374+
};
375+
TestLoggerFactory.getAllTestLoggers().values()
376+
.forEach(logger -> logger.setEnabledLevels(INFO_LEVELS));
377+
378+
var cache = context.isAsync()
379+
? context.buildAsync(loader).synchronous()
380+
: context.build(loader);
381+
cache.put(context.absentKey(), context.absentValue());
382+
context.ticker().advance(2, TimeUnit.MINUTES);
383+
cache.get(context.absentKey());
384+
385+
var event = Iterables.getOnlyElement(TestLoggerFactory.getLoggingEvents());
386+
assertThat(event.getThrowable().orElseThrow()).isInstanceOf(NullPointerException.class);
387+
assertThat(event.getLevel()).isEqualTo(WARN);
388+
389+
assertThat(refreshed.get()).isTrue();
390+
assertThat(cache.policy().refreshes()).isEmpty();
391+
assertThat(cache).containsEntry(context.absentKey(), context.absentValue());
392+
}
393+
360394
/* --------------- getIfPresent --------------- */
361395

362396
@CheckNoEvictions

0 commit comments

Comments
 (0)