From c29a257aad55a15ed5ad6f75c36c1cdc99387b35 Mon Sep 17 00:00:00 2001 From: Ben Manes Date: Mon, 16 Jan 2023 18:18:44 -0800 Subject: [PATCH] touchups to #859 --- .../caffeine/cache/BoundedLocalCache.java | 12 ++-- .../github/benmanes/caffeine/cache/Pacer.java | 5 ++ .../caffeine/cache/BoundedLocalCacheTest.java | 24 +++++++- .../caffeine/cache/ExpirationTest.java | 5 +- .../benmanes/caffeine/cache/PacerTest.java | 59 ++++++++++++++----- 5 files changed, 79 insertions(+), 26 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 index bda0a169a2..aac7815d5b 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 @@ -1661,15 +1661,13 @@ void rescheduleCleanUpIfIncomplete() { } // If a scheduler was configured then the maintenance can be deferred onto the custom executor - // to be run some time into the future. This is only leveraged if there was not otherwise - // scheduled by a pending expiration event. Otherwise, rely on other cache activity trigger the - // next run. + // to be run in the near future. This is only used if there is no scheduled set, else the next + // run depends on other activity to trigger it. var pacer = pacer(); - if ((pacer != null) && (pacer.future == null) && evictionLock.tryLock()) { + if ((pacer != null) && !pacer.isScheduled() && evictionLock.tryLock()) { try { - if ((pacer.future == null) && (drainStatusOpaque() == REQUIRED)) { - pacer.schedule(executor, drainBuffersTask, - expirationTicker().read(), Pacer.TOLERANCE); + if ((drainStatusOpaque() == REQUIRED) && !pacer.isScheduled()) { + pacer.schedule(executor, drainBuffersTask, expirationTicker().read(), Pacer.TOLERANCE); } } finally { evictionLock.unlock(); diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Pacer.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Pacer.java index 6d5b331ced..da9b4071d6 100644 --- a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Pacer.java +++ b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Pacer.java @@ -72,6 +72,11 @@ public void cancel() { } } + /** Returns if a task is scheduled to run. */ + public boolean isScheduled() { + return (future != null) && !future.isDone(); + } + /** * Returns if the current fire time is sooner, or if it is later and within the tolerance limit. */ 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 index 06a8948555..eb9cfbaae1 100644 --- a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/BoundedLocalCacheTest.java +++ b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/BoundedLocalCacheTest.java @@ -351,7 +351,7 @@ public void rescheduleCleanUpIfIncomplete_notScheduled_future( BoundedLocalCache cache, CacheContext context) { reset(context.scheduler()); cache.drainStatus = REQUIRED; - cache.pacer().future = DisabledFuture.INSTANCE; + cache.pacer().future = new CompletableFuture<>(); cache.rescheduleCleanUpIfIncomplete(); verifyNoInteractions(context.scheduler()); @@ -383,13 +383,33 @@ public void rescheduleCleanUpIfIncomplete_notScheduled_locked( @Test(dataProvider = "caches") @CacheSpec(population = Population.EMPTY, scheduler = CacheScheduler.MOCKITO) - public void rescheduleCleanUpIfIncomplete_scheduled( + public void rescheduleCleanUpIfIncomplete_scheduled_noFuture( BoundedLocalCache cache, CacheContext context) { reset(context.scheduler()); + + when(context.scheduler().schedule(any(), any(), anyLong(), any())) + .thenReturn(new CompletableFuture<>()); cache.drainStatus = REQUIRED; cache.pacer().cancel(); cache.rescheduleCleanUpIfIncomplete(); + assertThat(cache.pacer().isScheduled()).isTrue(); + verify(context.scheduler()).schedule(any(), any(), anyLong(), any()); + } + + @Test(dataProvider = "caches") + @CacheSpec(population = Population.EMPTY, scheduler = CacheScheduler.MOCKITO) + public void rescheduleCleanUpIfIncomplete_scheduled_doneFuture( + BoundedLocalCache cache, CacheContext context) { + reset(context.scheduler()); + + when(context.scheduler().schedule(any(), any(), anyLong(), any())) + .thenReturn(new CompletableFuture<>()); + cache.pacer().future = DisabledFuture.INSTANCE; + cache.drainStatus = REQUIRED; + + cache.rescheduleCleanUpIfIncomplete(); + assertThat(cache.pacer().isScheduled()).isTrue(); verify(context.scheduler()).schedule(any(), any(), anyLong(), any()); } 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 cc50e9c335..cb295b0dec 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 @@ -32,7 +32,6 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.atMostOnce; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.verify; @@ -150,11 +149,11 @@ public void schedule(Cache cache, CacheContext context) { public void schedule_immediate(Cache cache, CacheContext context) { doAnswer(invocation -> { invocation.getArgument(1, Runnable.class).run(); - return DisabledFuture.INSTANCE; + return new CompletableFuture<>(); }).when(context.scheduler()).schedule(any(), any(), anyLong(), any()); cache.put(context.absentKey(), context.absentValue()); - verify(context.scheduler(), atMostOnce()).schedule(any(), any(), anyLong(), any()); + verify(context.scheduler()).schedule(any(), any(), anyLong(), any()); } @Test(dataProvider = "caches") diff --git a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/PacerTest.java b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/PacerTest.java index a3183c097c..e8bfef4166 100644 --- a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/PacerTest.java +++ b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/PacerTest.java @@ -23,6 +23,7 @@ import static org.mockito.Mockito.verifyNoMoreInteractions; import java.util.Random; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.Executor; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; @@ -59,11 +60,12 @@ public void beforeMethod() throws Exception { @Test public void schedule_initialize() { long delay = random.nextInt(Ints.saturatedCast(Pacer.TOLERANCE)); - doReturn(DisabledFuture.INSTANCE) + doReturn(future) .when(scheduler).schedule(executor, command, Pacer.TOLERANCE, TimeUnit.NANOSECONDS); pacer.schedule(executor, command, NOW, delay); - assertThat(pacer.future).isSameInstanceAs(DisabledFuture.INSTANCE); + assertThat(pacer.isScheduled()).isTrue(); + assertThat(pacer.future).isSameInstanceAs(future); assertThat(pacer.nextFireTime).isEqualTo(NOW + Pacer.TOLERANCE); } @@ -74,11 +76,13 @@ public void schedule_initialize_recurse() { assertThat(pacer.future).isNull(); assertThat(pacer.nextFireTime).isNotEqualTo(0); pacer.schedule(executor, command, NOW, delay); - return DisabledFuture.INSTANCE; + return future; }).when(scheduler).schedule(executor, command, Pacer.TOLERANCE, TimeUnit.NANOSECONDS); pacer.schedule(executor, command, NOW, delay); - assertThat(pacer.future).isSameInstanceAs(DisabledFuture.INSTANCE); + + assertThat(pacer.isScheduled()).isTrue(); + assertThat(pacer.future).isSameInstanceAs(future); assertThat(pacer.nextFireTime).isEqualTo(NOW + Pacer.TOLERANCE); } @@ -92,13 +96,16 @@ public void schedule_cancel_schedule() { pacer.schedule(executor, command, NOW, delay); assertThat(pacer.nextFireTime).isEqualTo(fireTime); assertThat(pacer.future).isSameInstanceAs(future); + assertThat(pacer.isScheduled()).isTrue(); pacer.cancel(); verify(future).cancel(false); assertThat(pacer.nextFireTime).isEqualTo(0); + assertThat(pacer.isScheduled()).isFalse(); assertThat(pacer.future).isNull(); pacer.schedule(executor, command, NOW, delay); + assertThat(pacer.isScheduled()).isTrue(); assertThat(pacer.nextFireTime).isEqualTo(fireTime); assertThat(pacer.future).isSameInstanceAs(future); } @@ -110,10 +117,11 @@ public void scheduled_afterNextFireTime_skip() { long expectedNextFireTime = pacer.nextFireTime; pacer.schedule(executor, command, NOW, ONE_MINUTE_IN_NANOS); + verifyNoInteractions(scheduler, executor, command, future); + assertThat(pacer.isScheduled()).isTrue(); assertThat(pacer.future).isSameInstanceAs(future); assertThat(pacer.nextFireTime).isEqualTo(expectedNextFireTime); - verifyNoInteractions(scheduler, executor, command, future); } @Test @@ -125,10 +133,11 @@ public void schedule_beforeNextFireTime_skip() { long delay = ONE_MINUTE_IN_NANOS - Math.max(1, random.nextInt(Ints.saturatedCast(Pacer.TOLERANCE))); pacer.schedule(executor, command, NOW, delay); + verifyNoInteractions(scheduler, executor, command, future); + assertThat(pacer.isScheduled()).isTrue(); assertThat(pacer.future).isSameInstanceAs(future); assertThat(pacer.nextFireTime).isEqualTo(expectedNextFireTime); - verifyNoInteractions(scheduler, executor, command, future); } @Test @@ -137,18 +146,19 @@ public void schedule_beforeNextFireTime_minimumDelay() { pacer.future = future; long delay = random.nextInt(Ints.saturatedCast(Pacer.TOLERANCE)); - doReturn(DisabledFuture.INSTANCE) + doReturn(future) .when(scheduler).schedule(executor, command, Pacer.TOLERANCE, TimeUnit.NANOSECONDS); pacer.schedule(executor, command, NOW, delay); - assertThat(pacer.future).isSameInstanceAs(DisabledFuture.INSTANCE); - assertThat(pacer.nextFireTime).isEqualTo(NOW + Pacer.TOLERANCE); - verify(future).cancel(false); verify(scheduler).schedule(executor, command, Pacer.TOLERANCE, TimeUnit.NANOSECONDS); verifyNoInteractions(executor, command); verifyNoMoreInteractions(scheduler, future); + + assertThat(pacer.future).isSameInstanceAs(future); + assertThat(pacer.nextFireTime).isEqualTo(NOW + Pacer.TOLERANCE); + assertThat(pacer.isScheduled()).isTrue(); } @Test @@ -157,24 +167,26 @@ public void schedule_beforeNextFireTime_customDelay() { pacer.future = future; long delay = (Pacer.TOLERANCE + Math.max(1, random.nextInt())); - doReturn(DisabledFuture.INSTANCE) + doReturn(future) .when(scheduler).schedule(executor, command, delay, TimeUnit.NANOSECONDS); pacer.schedule(executor, command, NOW, delay); - assertThat(pacer.future).isSameInstanceAs(DisabledFuture.INSTANCE); - assertThat(pacer.nextFireTime).isEqualTo(NOW + delay); - verify(future).cancel(false); verify(scheduler).schedule(executor, command, delay, TimeUnit.NANOSECONDS); verifyNoInteractions(executor, command); verifyNoMoreInteractions(scheduler, future); + + assertThat(pacer.future).isSameInstanceAs(future); + assertThat(pacer.nextFireTime).isEqualTo(NOW + delay); + assertThat(pacer.isScheduled()).isTrue(); } @Test public void cancel_initialize() { pacer.cancel(); assertThat(pacer.nextFireTime).isEqualTo(0); + assertThat(pacer.isScheduled()).isFalse(); assertThat(pacer.future).isNull(); } @@ -186,6 +198,25 @@ public void cancel_scheduled() { pacer.cancel(); verify(future).cancel(false); assertThat(pacer.future).isNull(); + assertThat(pacer.isScheduled()).isFalse(); assertThat(pacer.nextFireTime).isEqualTo(0); } + + @Test + public void isScheduled_nullFuture() { + pacer.future = null; + assertThat(pacer.isScheduled()).isFalse(); + } + + @Test + public void isScheduled_doneFuture() { + pacer.future = DisabledFuture.INSTANCE; + assertThat(pacer.isScheduled()).isFalse(); + } + + @Test + public void isScheduled_inFlight() { + pacer.future = new CompletableFuture<>(); + assertThat(pacer.isScheduled()).isTrue(); + } }