Cleanup swallows and logs exceptions

ben-manes · Dec 2, 2015 · 023674e · 023674e
1 parent 0e7daa2
commit 023674e
Show file tree

Hide file tree

Showing 3 changed files with 28 additions and 20 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
@@ -166,7 +166,7 @@ protected BoundedLocalCache(Caffeine<K, V> builder, boolean isAsync) {
         ? new BoundedBuffer<>()
         : Buffer.disabled();
     accessPolicy = (evicts() || expiresAfterAccess()) ? this::onAccess : e -> {};
-    drainBuffersTask = this::cleanUp;
+    drainBuffersTask = this::performCleanUp;
 
     if (evicts()) {
       setMaximum(builder.getMaximumWeight());
@@ -873,8 +873,8 @@ void scheduleDrainBuffers() {
         lazySetDrainStatus(PROCESSING);
         executor().execute(drainBuffersTask);
       } catch (Throwable t) {
-        cleanUp();
         logger.log(Level.WARNING, "Exception thrown when submitting maintenance task", t);
+        performCleanUp();
       } finally {
         evictionLock.unlock();
       }
@@ -883,6 +883,19 @@ void scheduleDrainBuffers() {
 
   @Override
   public void cleanUp() {
+    try {
+      performCleanUp();
+    } catch (Throwable t) {
+      logger.log(Level.SEVERE, "Exception thrown when performing the maintenance task", t);
+    }
+  }
+
+  /**
+   * Performs the maintenance work, blocking until the lock is acquired, and sets the state flags
+   * to avoid excess scheduling attempts. Any exception thrown, such as by
+   * {@link CacheWriter#delete()}, is propagated to the caller.
+   */
+  void performCleanUp() {
     evictionLock.lock();
     try {
       lazySetDrainStatus(PROCESSING);

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
@@ -339,21 +339,18 @@ public void cleanUp(Cache<Integer, Integer> cache, CacheContext context) {
     verifyWriter(context, (verifier, writer) -> verifier.deletions(count, RemovalCause.EXPIRED));
   }
 
-  @Test(dataProvider = "caches", expectedExceptions = DeleteException.class)
+  @Test(dataProvider = "caches")
   @CacheSpec(implementation = Implementation.Caffeine, keys = ReferenceType.STRONG,
       population = Population.FULL, requiresExpiration = true,
       expireAfterAccess = {Expire.DISABLED, Expire.ONE_MINUTE},
       expireAfterWrite = {Expire.DISABLED, Expire.ONE_MINUTE},
       compute = Compute.SYNC, writer = Writer.EXCEPTIONAL, removalListener = Listener.REJECTING)
   public void cleanUp_writerFails(Cache<Integer, Integer> cache, CacheContext context) {
-    try {
-      context.ticker().advance(1, TimeUnit.HOURS);
-      cache.cleanUp();
-    } finally {
-      context.disableRejectingCacheWriter();
-      context.ticker().advance(-1, TimeUnit.HOURS);
-      assertThat(cache.asMap(), equalTo(context.original()));
-    }
+    context.ticker().advance(1, TimeUnit.HOURS);
+    cache.cleanUp();
+    context.disableRejectingCacheWriter();
+    context.ticker().advance(-1, TimeUnit.HOURS);
+    assertThat(cache.asMap(), equalTo(context.original()));
   }
 
   /* ---------------- LoadingCache -------------- */

diff --git a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/ReferenceTest.java b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/ReferenceTest.java
@@ -335,21 +335,19 @@ public void cleanUp(Cache<Integer, Integer> cache, CacheContext context) {
     verifyWriter(context, (verifier, writer) -> verifier.deletions(count, RemovalCause.COLLECTED));
   }
 
-  @Test(dataProvider = "caches", expectedExceptions = DeleteException.class)
+  @Test(dataProvider = "caches")
   @CacheSpec(keys = ReferenceType.STRONG, values = {ReferenceType.WEAK, ReferenceType.SOFT},
       implementation = Implementation.Caffeine, expireAfterAccess = Expire.DISABLED,
       expireAfterWrite = Expire.DISABLED, maximumSize = MaximumSize.DISABLED,
       weigher = CacheWeigher.DEFAULT, population = Population.FULL, stats = Stats.ENABLED,
       compute = Compute.SYNC, removalListener = Listener.CONSUMING, writer = Writer.EXCEPTIONAL)
   public void cleanUp_writerFails(Cache<Integer, Integer> cache, CacheContext context) {
-    try {
-      context.clear();
-      GcFinalization.awaitFullGc();
-      cache.cleanUp();
-    } finally {
-      context.disableRejectingCacheWriter();
-      assertThat(cache.asMap().isEmpty(), is(false));
-    }
+    context.clear();
+    GcFinalization.awaitFullGc();
+    cache.cleanUp();
+
+    context.disableRejectingCacheWriter();
+    assertThat(cache.asMap().isEmpty(), is(false));
   }
 
   /* ---------------- LoadingCache -------------- */