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 index 97d433d2c0..711f53ddbc 100644 --- a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java +++ b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java @@ -1223,8 +1223,9 @@ void demoteFromMainProtected() { * @param node the entry in the page replacement policy * @param now the current time, in nanoseconds * @param recordHit if the hit count should be incremented + * @return the refreshed value if immediately loaded, else null */ - void afterRead(Node node, long now, boolean recordHit) { + @Nullable V afterRead(Node node, long now, boolean recordHit) { if (recordHit) { statsCounter().recordHits(1); } @@ -1233,7 +1234,7 @@ void afterRead(Node node, long now, boolean recordHit) { if (shouldDrainBuffers(delayable)) { scheduleDrainBuffers(); } - refreshIfNeeded(node, now); + return refreshIfNeeded(node, now); } /** Returns if the cache should bypass the read buffer. */ @@ -1246,11 +1247,12 @@ boolean skipReadBuffer() { * * @param node the entry in the cache to refresh * @param now the current time, in nanoseconds + * @return the refreshed value if immediately loaded, else null */ @SuppressWarnings("FutureReturnValueIgnored") - void refreshIfNeeded(Node node, long now) { + @Nullable V refreshIfNeeded(Node node, long now) { if (!refreshAfterWrite()) { - return; + return null; } K key; @@ -1301,6 +1303,8 @@ void refreshIfNeeded(Node node, long now) { } if (refreshFuture[0] != null) { + @SuppressWarnings("unchecked") + V[] refreshedValue = (V[]) new Object[1]; refreshFuture[0].whenComplete((newValue, error) -> { long loadTime = statsTicker().read() - startTime[0]; if (error != null) { @@ -1316,7 +1320,7 @@ void refreshIfNeeded(Node node, long now) { V value = (isAsync && (newValue != null)) ? (V) refreshFuture[0] : newValue; boolean[] discard = new boolean[1]; - compute(key, (k, currentValue) -> { + refreshedValue[0] = compute(key, (k, currentValue) -> { if (currentValue == null) { // If the entry is absent then discard the refresh and maybe notifying the listener discard[0] = (value != null); @@ -1349,8 +1353,10 @@ void refreshIfNeeded(Node node, long now) { refreshes.remove(keyReference, refreshFuture[0]); }); + return refreshedValue[0]; } } + return null; } /** @@ -2074,8 +2080,8 @@ public boolean containsValue(Object value) { setAccessTime(node, now); tryExpireAfterRead(node, castedKey, value, expiry(), now); } - afterRead(node, now, recordStats); - return value; + V refreshed = afterRead(node, now, recordStats); + return (refreshed == null) ? value : refreshed; } @Override @@ -2117,15 +2123,18 @@ public Map getAllPresent(Iterable keys) { if ((node == null) || ((value = node.getValue()) == null) || hasExpired(node, now)) { iter.remove(); } else { - entry.setValue(value); - if (!isComputingAsync(node)) { @SuppressWarnings("unchecked") K castedKey = (K) entry.getKey(); tryExpireAfterRead(node, castedKey, value, expiry(), now); setAccessTime(node, now); } - afterRead(node, now, /* recordHit */ false); + V refreshed = afterRead(node, now, /* recordHit */ false); + if (refreshed == null) { + entry.setValue(value); + } else { + entry.setValue(refreshed); + } } } statsCounter().recordHits(result.size()); @@ -2489,9 +2498,8 @@ public void replaceAll(BiFunction function) { tryExpireAfterRead(node, key, value, expiry(), now); setAccessTime(node, now); } - - afterRead(node, now, /* recordHit */ recordStats); - return value; + var refreshed = afterRead(node, now, /* recordHit */ recordStats); + return (refreshed == null) ? value : refreshed; } } if (recordStats) { 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 index c0f7e96cf0..87081e9e34 100644 --- a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/RefreshAfterWriteTest.java +++ b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/RefreshAfterWriteTest.java @@ -29,6 +29,7 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth8.assertThat; import static java.util.Map.entry; +import static java.util.function.Function.identity; import static org.hamcrest.Matchers.is; import static uk.org.lidalia.slf4jext.ConventionalLevelHierarchy.INFO_LEVELS; import static uk.org.lidalia.slf4jext.Level.WARN; @@ -63,6 +64,7 @@ import com.github.benmanes.caffeine.testing.ConcurrentTestHarness; import com.github.benmanes.caffeine.testing.Int; import com.github.valfirst.slf4jtest.TestLoggerFactory; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; /** @@ -340,18 +342,36 @@ public void refreshIfNeeded_error_log(CacheContext context) { /* --------------- getIfPresent --------------- */ @Test(dataProvider = "caches") - @CacheSpec(refreshAfterWrite = Expire.ONE_MINUTE, loader = Loader.NEGATIVE, + @CacheSpec(refreshAfterWrite = Expire.ONE_MINUTE, loader = Loader.IDENTITY, population = { Population.SINGLETON, Population.PARTIAL, Population.FULL }) - public void getIfPresent(LoadingCache cache, CacheContext context) { + public void getIfPresent_immediate(LoadingCache cache, CacheContext context) { context.ticker().advance(30, TimeUnit.SECONDS); assertThat(cache.getIfPresent(context.middleKey())).isEqualTo(context.middleKey().negate()); context.ticker().advance(45, TimeUnit.SECONDS); - assertThat(cache.getIfPresent(context.middleKey())).isEqualTo(context.middleKey().negate()); + assertThat(cache.getIfPresent(context.middleKey())).isEqualTo(context.middleKey()); assertThat(cache).hasSize(context.initialSize()); assertThat(context).removalNotifications().withCause(REPLACED).hasSize(1).exclusively(); } + @Test(dataProvider = "caches") + @CacheSpec(refreshAfterWrite = Expire.ONE_MINUTE, loader = Loader.ASYNC_INCOMPLETE, + population = { Population.SINGLETON, Population.PARTIAL, Population.FULL }) + public void getIfPresent_delayed(LoadingCache cache, CacheContext context) { + context.ticker().advance(30, TimeUnit.SECONDS); + assertThat(cache.getIfPresent(context.middleKey())).isEqualTo(context.middleKey().negate()); + context.ticker().advance(45, TimeUnit.SECONDS); + assertThat(cache.getIfPresent(context.middleKey())).isEqualTo(context.middleKey().negate()); + + assertThat(cache).hasSize(context.initialSize()); + assertThat(context).removalNotifications().isEmpty(); + + if (context.isCaffeine()) { + cache.policy().refreshes().get(context.middleKey()).complete(context.middleKey()); + assertThat(context).removalNotifications().withCause(REPLACED).hasSize(1).exclusively(); + } + } + @Test(dataProvider = "caches") @CacheSpec(refreshAfterWrite = Expire.ONE_MINUTE, loader = Loader.NEGATIVE, population = { Population.SINGLETON, Population.PARTIAL, Population.FULL }) @@ -368,35 +388,71 @@ public void getIfPresent_async(AsyncLoadingCache cache, CacheContext c /* --------------- getAllPresent --------------- */ @Test(dataProvider = "caches") - @CacheSpec(refreshAfterWrite = Expire.ONE_MINUTE, + @CacheSpec(refreshAfterWrite = Expire.ONE_MINUTE, loader = Loader.IDENTITY, population = { Population.PARTIAL, Population.FULL }) - public void getAllPresent(LoadingCache cache, CacheContext context) { + public void getAllPresent_immediate(LoadingCache cache, CacheContext context) { int count = context.firstMiddleLastKeys().size(); context.ticker().advance(30, TimeUnit.SECONDS); cache.getAllPresent(context.firstMiddleLastKeys()); context.ticker().advance(45, TimeUnit.SECONDS); - assertThat(cache.getAllPresent(context.firstMiddleLastKeys())).hasSize(count); + assertThat(cache.getAllPresent(context.firstMiddleLastKeys())).containsExactly( + context.firstKey(), context.firstKey(), context.middleKey(), context.middleKey(), + context.lastKey(), context.lastKey()); assertThat(cache).hasSize(context.initialSize()); assertThat(context).removalNotifications().withCause(REPLACED).hasSize(count).exclusively(); } + @Test(dataProvider = "caches") + @CacheSpec(refreshAfterWrite = Expire.ONE_MINUTE, loader = Loader.ASYNC_INCOMPLETE, + population = { Population.SINGLETON, Population.PARTIAL, Population.FULL }) + public void getAllPresent_delayed(LoadingCache cache, CacheContext context) { + context.ticker().advance(30, TimeUnit.SECONDS); + var expected = cache.getAllPresent(context.firstMiddleLastKeys()); + context.ticker().advance(45, TimeUnit.SECONDS); + assertThat(cache.getAllPresent(context.firstMiddleLastKeys())) + .containsExactlyEntriesIn(expected); + + if (context.isCaffeine()) { + for (var key : context.firstMiddleLastKeys()) { + cache.policy().refreshes().get(key).complete(key); + } + assertThat(context).removalNotifications().withCause(REPLACED) + .hasSize(expected.size()).exclusively(); + } + } + /* --------------- getFunc --------------- */ @Test(dataProvider = "caches") - @CacheSpec(refreshAfterWrite = Expire.ONE_MINUTE, + @CacheSpec(refreshAfterWrite = Expire.ONE_MINUTE, loader = Loader.IDENTITY, population = { Population.PARTIAL, Population.FULL }) - public void getFunc(LoadingCache cache, CacheContext context) { - Function mappingFunction = context.original()::get; + public void getFunc_immediate(LoadingCache cache, CacheContext context) { context.ticker().advance(30, TimeUnit.SECONDS); - cache.get(context.firstKey(), mappingFunction); + cache.get(context.firstKey(), identity()); context.ticker().advance(45, TimeUnit.SECONDS); - cache.get(context.lastKey(), mappingFunction); // refreshed + assertThat(cache.get(context.lastKey(), identity())).isEqualTo(context.lastKey()); assertThat(cache).hasSize(context.initialSize()); assertThat(context).removalNotifications().withCause(REPLACED).hasSize(1).exclusively(); } + @Test(dataProvider = "caches") + @CacheSpec(refreshAfterWrite = Expire.ONE_MINUTE, loader = Loader.ASYNC_INCOMPLETE, + population = { Population.PARTIAL, Population.FULL }) + public void getFunc_delayed(LoadingCache cache, CacheContext context) { + Function mappingFunction = context.original()::get; + context.ticker().advance(30, TimeUnit.SECONDS); + cache.get(context.firstKey(), mappingFunction); + context.ticker().advance(45, TimeUnit.SECONDS); + assertThat(cache.get(context.lastKey(), mappingFunction)).isEqualTo(context.lastKey().negate()); + + if (context.isCaffeine()) { + cache.policy().refreshes().get(context.lastKey()).complete(context.lastKey()); + assertThat(context).removalNotifications().withCause(REPLACED).hasSize(1).exclusively(); + } + } + @Test(dataProvider = "caches") @CacheSpec(refreshAfterWrite = Expire.ONE_MINUTE, population = { Population.PARTIAL, Population.FULL }) @@ -414,18 +470,33 @@ public void getFunc_async(AsyncLoadingCache cache, CacheContext contex /* --------------- get --------------- */ @Test(dataProvider = "caches") - @CacheSpec(refreshAfterWrite = Expire.ONE_MINUTE, + @CacheSpec(refreshAfterWrite = Expire.ONE_MINUTE, loader = Loader.IDENTITY, population = { Population.PARTIAL, Population.FULL }) - public void get(LoadingCache cache, CacheContext context) { + public void get_immediate(LoadingCache cache, CacheContext context) { context.ticker().advance(30, TimeUnit.SECONDS); cache.get(context.firstKey()); - cache.get(context.absentKey()); context.ticker().advance(45, TimeUnit.SECONDS); + assertThat(cache.get(context.firstKey())).isEqualTo(context.firstKey()); - assertThat(cache).containsEntry(context.firstKey(), context.firstKey().negate()); + assertThat(cache).hasSize(context.initialSize()); assertThat(context).removalNotifications().withCause(REPLACED).hasSize(1).exclusively(); } + @Test(dataProvider = "caches") + @CacheSpec(refreshAfterWrite = Expire.ONE_MINUTE, loader = Loader.ASYNC_INCOMPLETE, + population = { Population.PARTIAL, Population.FULL }) + public void get_delayed(LoadingCache cache, CacheContext context) { + context.ticker().advance(30, TimeUnit.SECONDS); + cache.get(context.firstKey()); + context.ticker().advance(45, TimeUnit.SECONDS); + assertThat(cache.get(context.firstKey())).isEqualTo(context.firstKey().negate()); + + if (context.isCaffeine()) { + cache.policy().refreshes().get(context.firstKey()).complete(context.firstKey()); + assertThat(context).removalNotifications().withCause(REPLACED).hasSize(1).exclusively(); + } + } + @Test(dataProvider = "caches") @CacheSpec(refreshAfterWrite = Expire.ONE_MINUTE, loader = Loader.IDENTITY, population = { Population.PARTIAL, Population.FULL }) @@ -511,23 +582,44 @@ public void get_null(AsyncLoadingCache cache, CacheContext context) { @Test(dataProvider = "caches") @CacheSpec(refreshAfterWrite = Expire.ONE_MINUTE, loader = Loader.IDENTITY, population = { Population.PARTIAL, Population.FULL }) - public void getAll(LoadingCache cache, CacheContext context) { + public void getAll_immediate(LoadingCache cache, CacheContext context) { var keys = List.of(context.firstKey(), context.absentKey()); context.ticker().advance(30, TimeUnit.SECONDS); assertThat(cache.getAll(keys)).containsExactly( context.firstKey(), context.firstKey().negate(), context.absentKey(), context.absentKey()); - // Trigger a refresh, may return old values - context.ticker().advance(45, TimeUnit.SECONDS); - cache.getAll(keys); - - // Ensure new values are present + // Trigger a refresh, ensure new values are present assertThat(cache.getAll(keys)).containsExactly( context.firstKey(), context.firstKey(), context.absentKey(), context.absentKey()); assertThat(context).removalNotifications().withCause(REPLACED).hasSize(1).exclusively(); } + @Test(dataProvider = "caches") + @CacheSpec(refreshAfterWrite = Expire.ONE_MINUTE, loader = Loader.ASYNC_INCOMPLETE, + population = { Population.PARTIAL, Population.FULL }) + public void getAll_delayed(LoadingCache cache, CacheContext context) { + var keys = context.firstMiddleLastKeys(); + var expected = ImmutableMap.of( + context.firstKey(), context.firstKey().negate(), + context.middleKey(), context.middleKey().negate(), + context.lastKey(), context.lastKey().negate()); + context.ticker().advance(30, TimeUnit.SECONDS); + assertThat(cache.getAll(keys)).containsExactlyEntriesIn(expected); + + // Trigger a refresh, returns old values + context.ticker().advance(45, TimeUnit.SECONDS); + assertThat(cache.getAll(keys)).containsExactlyEntriesIn(expected); + + if (context.isCaffeine()) { + for (var key : keys) { + cache.policy().refreshes().get(key).complete(key); + } + assertThat(context).removalNotifications().withCause(REPLACED) + .hasSize(keys.size()).exclusively(); + } + } + @Test(dataProvider = "caches") @CacheSpec(refreshAfterWrite = Expire.ONE_MINUTE, loader = Loader.IDENTITY, population = { Population.PARTIAL, Population.FULL }) diff --git a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/testing/GuavaCacheFromContext.java b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/testing/GuavaCacheFromContext.java index 2cdb833225..596c51d71e 100644 --- a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/testing/GuavaCacheFromContext.java +++ b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/testing/GuavaCacheFromContext.java @@ -63,6 +63,8 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; import com.google.common.util.concurrent.ExecutionError; +import com.google.common.util.concurrent.ListenableFuture; +import com.google.common.util.concurrent.SettableFuture; import com.google.common.util.concurrent.UncheckedExecutionException; /** @@ -627,6 +629,20 @@ public V load(K key) throws Exception { throw e; } } + + @Override + @SuppressWarnings("FutureReturnValueIgnored") + public ListenableFuture reload(K key, V oldValue) throws Exception { + var future = SettableFuture.create(); + delegate.asyncReload(key, oldValue, Runnable::run).whenComplete((r, e) -> { + if (e == null) { + future.set(r); + } else { + future.setException(e); + } + }); + return future; + } } static class BulkLoader extends SingleLoader {