From 49fc588c99d69a5e62bbc9d127f9529b74b7e3cf Mon Sep 17 00:00:00 2001 From: Ben Manes Date: Sun, 28 Jun 2020 13:55:59 -0700 Subject: [PATCH] Fix expiration delay for scheduled cleanup (fixes #431) --- build.gradle | 2 +- .../caffeine/cache/BoundedLocalCache.java | 8 +-- .../caffeine/cache/ExpirationTest.java | 52 ++++++++++++++++++- .../caffeine/cache/testing/CacheSpec.java | 10 +++- checksum.xml | 14 +++++ gradle/dependencies.gradle | 14 ++++- simulator/build.gradle | 1 + 7 files changed, 92 insertions(+), 9 deletions(-) diff --git a/build.gradle b/build.gradle index cd3efc40bc..f6283ef1d8 100644 --- a/build.gradle +++ b/build.gradle @@ -156,6 +156,6 @@ tasks.coveralls { } dependencyUpdates.resolutionStrategy { - force 'javax.json.bind:javax.json.bind-api:1.0' + force libraries.coherenceProvidedScope force testLibraries.truth } 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 ede7040cc7..7f85497d3f 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 @@ -850,23 +850,23 @@ private long getExpirationDelay(long now) { if (expiresAfterAccess()) { Node node = accessOrderWindowDeque().peekFirst(); if (node != null) { - delay = Math.min(delay, now - node.getAccessTime() + expiresAfterAccessNanos()); + delay = Math.min(delay, expiresAfterAccessNanos() - (now - node.getAccessTime())); } if (evicts()) { node = accessOrderProbationDeque().peekFirst(); if (node != null) { - delay = Math.min(delay, now - node.getAccessTime() + expiresAfterAccessNanos()); + delay = Math.min(delay, expiresAfterAccessNanos() - (now - node.getAccessTime())); } node = accessOrderProtectedDeque().peekFirst(); if (node != null) { - delay = Math.min(delay, now - node.getAccessTime() + expiresAfterAccessNanos()); + delay = Math.min(delay, expiresAfterAccessNanos() - (now - node.getAccessTime())); } } } if (expiresAfterWrite()) { Node node = writeOrderDeque().peekFirst(); if (node != null) { - delay = Math.min(delay, now - node.getWriteTime() + expiresAfterWriteNanos()); + delay = Math.min(delay, expiresAfterWriteNanos() - (now - node.getWriteTime())); } } if (expiresVariable()) { diff --git a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/ExpirationTest.java b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/ExpirationTest.java index 34d9a3dd92..9fccc5d1ec 100644 --- a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/ExpirationTest.java +++ b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/ExpirationTest.java @@ -39,7 +39,10 @@ import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import java.time.Duration; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; @@ -48,6 +51,7 @@ import java.util.function.Function; import org.mockito.ArgumentCaptor; +import org.mockito.stubbing.Answer; import org.testng.annotations.Listeners; import org.testng.annotations.Test; @@ -75,6 +79,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterators; import com.google.common.collect.Maps; +import com.google.common.util.concurrent.Futures; /** * The test cases for caches that support an expiration policy. @@ -149,7 +154,7 @@ public void schedule(Cache cache, CacheContext context) { scheduler = CacheScheduler.MOCK) public void schedule_immediate(Cache cache, CacheContext context) { doAnswer(invocation -> { - ((Runnable) invocation.getArgument(1)).run(); + invocation.getArgument(1, Runnable.class).run(); return DisabledFuture.INSTANCE; }).when(context.scheduler()).schedule(any(), any(), anyLong(), any()); @@ -157,6 +162,51 @@ public void schedule_immediate(Cache cache, CacheContext conte verify(context.scheduler(), atMostOnce()).schedule(any(), any(), anyLong(), any()); } + @Test(dataProvider = "caches") + @CacheSpec(implementation = Implementation.Caffeine, population = Population.EMPTY, + mustExpireWithAnyOf = { AFTER_ACCESS, AFTER_WRITE, VARIABLE }, + expiry = { CacheExpiry.DISABLED, CacheExpiry.CREATE, CacheExpiry.WRITE, CacheExpiry.ACCESS }, + expireAfterAccess = {Expire.DISABLED, Expire.ONE_MINUTE}, + expireAfterWrite = {Expire.DISABLED, Expire.ONE_MINUTE}, expiryTime = Expire.ONE_MINUTE, + scheduler = CacheScheduler.MOCK, removalListener = Listener.MOCK) + public void schedule_delay(Cache cache, CacheContext context) + throws InterruptedException { + Map actualExpirationPeriods = new HashMap<>(); + ArgumentCaptor delay = ArgumentCaptor.forClass(long.class); + ArgumentCaptor task = ArgumentCaptor.forClass(Runnable.class); + Answer onRemoval = invocation -> { + Integer key = invocation.getArgument(0, Integer.class); + Duration value = invocation.getArgument(1, Duration.class); + actualExpirationPeriods.put(key, Duration.ofNanos(context.ticker().read()).minus(value)); + return null; + }; + doAnswer(onRemoval).when(context.removalListener()).onRemoval(any(), any(), any()); + when(context.scheduler().schedule(any(), task.capture(), delay.capture(), any())) + .thenReturn(Futures.immediateFuture(null)); + + Integer key1 = 1; + cache.put(key1, Duration.ofNanos(context.ticker().read())); + + Duration insertDelay = Duration.ofMillis(10); + context.ticker().advance(insertDelay); + + Integer key2 = 2; + cache.put(key2, Duration.ofNanos(context.ticker().read())); + + Duration expireKey1 = Duration.ofNanos(delay.getValue()).minus(insertDelay); + context.ticker().advance(expireKey1); + task.getValue().run(); + + Duration expireKey2 = Duration.ofNanos(delay.getValue()); + context.ticker().advance(expireKey2); + task.getValue().run(); + + Duration maxExpirationPeriod = Duration.ofNanos(Pacer.TOLERANCE) + .plusNanos(context.expiryTime().timeNanos()); + assertThat(actualExpirationPeriods.get(key1), lessThan(maxExpirationPeriod)); + assertThat(actualExpirationPeriods.get(key2), lessThan(maxExpirationPeriod)); + } + /* --------------- Cache --------------- */ @Test(dataProvider = "caches", expectedExceptions = DeleteException.class) diff --git a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/testing/CacheSpec.java b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/testing/CacheSpec.java index 4af12ff2a7..94d6311c7b 100644 --- a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/testing/CacheSpec.java +++ b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/testing/CacheSpec.java @@ -329,8 +329,9 @@ enum Expire { IMMEDIATELY(0L), /** A configuration where entries are evicted almost immediately. */ ONE_MILLISECOND(TimeUnit.MILLISECONDS.toNanos(1L)), - /** A configuration that holds a single entry. */ + /** A configuration where entries are after a time duration. */ ONE_MINUTE(TimeUnit.MINUTES.toNanos(1L)), + /** A configuration where entries should never expire. */ /** A configuration that holds the {@link Population#FULL} count. */ FOREVER(Long.MAX_VALUE); @@ -420,6 +421,13 @@ enum Listener { @Override public RemovalListener create() { return RemovalListeners.consuming(); } + }, + /** A removal listener that records interactions. */ + MOCK { + @SuppressWarnings("unchecked") + @Override public RemovalListener create() { + return Mockito.mock(RemovalListener.class); + } }; public abstract RemovalListener create(); diff --git a/checksum.xml b/checksum.xml index 6638f6a90c..cd38f02960 100644 --- a/checksum.xml +++ b/checksum.xml @@ -53,6 +53,7 @@ + @@ -82,11 +83,20 @@ + + + + + + + + + @@ -132,7 +142,11 @@ + + + + diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 0bca78a72c..1baa1790eb 100644 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -77,7 +77,7 @@ ext { pluginVersions = [ apt: '0.21', bnd: '5.1.1', - checkstyle: '8.33', + checkstyle: '8.34', coveralls: '2.8.4', coverity: '1.0.10', errorprone: '1.2.1', @@ -85,7 +85,7 @@ ext { jmh: '0.5.0', jmhReport: '0.9.0', nullaway: '1.0.1', - pmd: '6.24.0', + pmd: '6.25.0', semanticVersioning: '1.1.0', shadow: '6.0.0', sonarqube: '3.0', @@ -103,6 +103,16 @@ ext { "com.oracle.coherence.ce:coherence:${versions.coherence}", 'javax.json.bind:javax.json.bind-api:1.0', ], + coherenceProvidedScope: [ + 'org.glassfish.jersey.core:jersey-server:2.31', + 'javax.json.bind:javax.json.bind-api:1.0', + 'javax.ws.rs:javax.ws.rs-api:2.0', + 'javax.jms:javax.jms-api:2.0.1', + 'javax.inject:javax.inject:1', + 'org.ow2.asm:asm-commons:8.0.1', + 'com.sleepycat:je:18.3.12', + 'org.ow2.asm:asm:8.0.1', + ], collision: "systems.comodal:collision:${versions.collision}", commonsCompress: "org.apache.commons:commons-compress:${versions.commonsCompress}", commonsLang3: "org.apache.commons:commons-lang3:${versions.commonsLang3}", diff --git a/simulator/build.gradle b/simulator/build.gradle index 4b527a4f4e..4cf5671106 100644 --- a/simulator/build.gradle +++ b/simulator/build.gradle @@ -30,6 +30,7 @@ dependencies { implementation libraries.elasticSearch implementation libraries.commonsCompress implementation libraries.univocityParsers + implementation libraries.coherenceProvidedScope testImplementation testLibraries.testng }