From 023674e58b3733f5b11d7da6e587c3e6b16d8e6c Mon Sep 17 00:00:00 2001
From: Ben Manes <ben.manes@gmail.com>
Date: Tue, 1 Dec 2015 19:54:47 -0800
Subject: [PATCH] Cleanup swallows and logs exceptions

---
 .../caffeine/cache/BoundedLocalCache.java       | 17 +++++++++++++++--
 .../benmanes/caffeine/cache/ExpirationTest.java | 15 ++++++---------
 .../benmanes/caffeine/cache/ReferenceTest.java  | 16 +++++++---------
 3 files changed, 28 insertions(+), 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
index 5ff31b1c03..94cad08fda 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
@@ -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
index d515db43a3..731497ce42 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
@@ -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
index 35ea0562d2..bed6743014 100644
--- 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 -------------- */