Trigger maintainance if an iterator observes an expired entry (fixes #…

…487)
ben-manes · Jan 12, 2021 · 3ee6531 · 3ee6531
1 parent 259e4fe
commit 3ee6531
Show file tree

Hide file tree

Showing 2 changed files with 61 additions and 1 deletion.
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
@@ -3130,7 +3130,12 @@ public boolean hasNext() {
           next = iterator.next();
           value = next.getValue();
           key = next.getKey();
-          if (cache.hasExpired(next, now) || (key == null) || (value == null) || !next.isAlive()) {
+
+          boolean evictable = cache.hasExpired(next, now) || (key == null) || (value == null);
+          if (evictable || !next.isAlive()) {
+            if (evictable) {
+              cache.scheduleDrainBuffers();
+            }
             value = null;
             next = null;
             key = null;

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
@@ -21,6 +21,7 @@
 import static com.github.benmanes.caffeine.cache.testing.CacheSpec.Expiration.VARIABLE;
 import static com.github.benmanes.caffeine.cache.testing.CacheWriterVerifier.verifyWriter;
 import static com.github.benmanes.caffeine.cache.testing.HasRemovalNotifications.hasRemovalNotifications;
+import static com.github.benmanes.caffeine.testing.IsEmptyMap.emptyMap;
 import static com.github.benmanes.caffeine.testing.IsFutureValue.futureOf;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.aMapWithSize;
@@ -1423,6 +1424,24 @@ public void keySetToArray(Map<Integer, Integer> map, CacheContext context) {
     assertThat(map.keySet().toArray(), arrayWithSize(0));
   }
 
+  @Test(dataProvider = "caches")
+  @CacheSpec(removalListener = { Listener.DEFAULT, Listener.REJECTING },
+      population = { Population.SINGLETON, Population.PARTIAL, Population.FULL },
+      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,
+      implementation = Implementation.Caffeine)
+  public void keySet_iterator(Map<Integer, Integer> map, CacheContext context) {
+    context.ticker().advance(10, TimeUnit.MINUTES);
+    assertThat(map.keySet().iterator().hasNext(), is(false));
+    assertThat(map, is(emptyMap()));
+    assertThat(map, hasRemovalNotifications(context,
+        context.original().size(), RemovalCause.EXPIRED));
+    verifyWriter(context, (verifier, writer) ->
+        verifier.deletedAll(context.original(), RemovalCause.EXPIRED));
+  }
+
   @Test(dataProvider = "caches")
   @CacheSpec(removalListener = { Listener.DEFAULT, Listener.REJECTING },
       mustExpireWithAnyOf = { AFTER_ACCESS, AFTER_WRITE, VARIABLE },
@@ -1435,6 +1454,24 @@ public void valuesToArray(Map<Integer, Integer> map, CacheContext context) {
     assertThat(map.values().toArray(), arrayWithSize(0));
   }
 
+  @Test(dataProvider = "caches")
+  @CacheSpec(removalListener = { Listener.DEFAULT, Listener.REJECTING },
+      population = { Population.SINGLETON, Population.PARTIAL, Population.FULL },
+      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,
+      implementation = Implementation.Caffeine)
+  public void values_iterator(Map<Integer, Integer> map, CacheContext context) {
+    context.ticker().advance(10, TimeUnit.MINUTES);
+    assertThat(map.values().iterator().hasNext(), is(false));
+    assertThat(map, is(emptyMap()));
+    assertThat(map, hasRemovalNotifications(context,
+        context.original().size(), RemovalCause.EXPIRED));
+    verifyWriter(context, (verifier, writer) ->
+        verifier.deletedAll(context.original(), RemovalCause.EXPIRED));
+  }
+
   @Test(dataProvider = "caches")
   @CacheSpec(removalListener = { Listener.DEFAULT, Listener.REJECTING },
       mustExpireWithAnyOf = { AFTER_ACCESS, AFTER_WRITE, VARIABLE },
@@ -1447,6 +1484,24 @@ public void entrySetToArray(Map<Integer, Integer> map, CacheContext context) {
     assertThat(map.entrySet().toArray(), arrayWithSize(0));
   }
 
+  @Test(dataProvider = "caches")
+  @CacheSpec(removalListener = { Listener.DEFAULT, Listener.REJECTING },
+      population = { Population.SINGLETON, Population.PARTIAL, Population.FULL },
+      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,
+      implementation = Implementation.Caffeine)
+  public void entrySet_iterator(Map<Integer, Integer> map, CacheContext context) {
+    context.ticker().advance(10, TimeUnit.MINUTES);
+    assertThat(map.keySet().iterator().hasNext(), is(false));
+    assertThat(map, is(emptyMap()));
+    assertThat(map, hasRemovalNotifications(context,
+        context.original().size(), RemovalCause.EXPIRED));
+    verifyWriter(context, (verifier, writer) ->
+        verifier.deletedAll(context.original(), RemovalCause.EXPIRED));
+  }
+
   /**
    * Ensures that variable expiration is run, as it may not have due to expiring in coarse batches.
    */