From 8a785d5efed046fe961b2f97a1ce51391ec577d2 Mon Sep 17 00:00:00 2001 From: Ben Manes Date: Tue, 20 Feb 2018 23:35:51 -0800 Subject: [PATCH] Fix variable expiration overflowing with the maximum duration (fixes #217) When the duration is set to the maximum length, Long.MAX_VALUE nanoseconds, the calcuation of expirationTime - currentTime > 0 may overflow and be negative. This will not occur if the same thread calculates both timestamps. It may occur across threads when the expirationTime is concurrently updated using a later base time than t1's reading of the currentTime. This can occur whenever the maintenance work is triggered to sweep expired entries and a user thread accesses the entry. The later timestamp plus the maximum duration results in an overflow, causing the remaining time to be negative, and therefore causes the cache to expire the entry. The internal maximum is now capped at Long.MAX_VALUE / 2 or ~150 years. This should give a broad safety net to avoid these concurrency-inducing overflows in normal code. --- .../github/benmanes/caffeine/cache/Async.java | 24 ++++++-------- .../caffeine/cache/BoundedLocalCache.java | 14 ++++---- .../benmanes/caffeine/cache/AsyncTest.java | 12 ------- .../caffeine/cache/ExpireAfterVarTest.java | 32 +++++++++++++++++++ gradle/dependencies.gradle | 2 +- 5 files changed, 50 insertions(+), 34 deletions(-) diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Async.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Async.java index 3af148844c..610e014100 100644 --- a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Async.java +++ b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Async.java @@ -15,6 +15,7 @@ */ package com.github.benmanes.caffeine.cache; +import static com.github.benmanes.caffeine.cache.BoundedLocalCache.MAXIMUM_EXPIRY; import static java.util.Objects.requireNonNull; import java.io.Serializable; @@ -31,7 +32,6 @@ * @author ben.manes@gmail.com (Ben Manes) */ final class Async { - static final long MAXIMUM_EXPIRY = (Long.MAX_VALUE >> 1); // 150 years private Async() {} @@ -120,8 +120,7 @@ Object writeReplace() { * An expiry for asynchronous computations. When the value is being loaded this expiry returns * {@code Long.MAX_VALUE} to indicate that the entry should not be evicted due to an expiry * constraint. If the value is computed successfully the entry must be reinserted so that the - * expiration is updated and the expiration timeouts reflect the value once present. The value - * maximum range is reserved to coordinate the asynchronous life cycle. + * expiration is updated and the expiration timeouts reflect the value once present. */ static final class AsyncExpiry implements Expiry>, Serializable { private static final long serialVersionUID = 1L; @@ -134,21 +133,18 @@ static final class AsyncExpiry implements Expiry>, @Override public long expireAfterCreate(K key, CompletableFuture future, long currentTime) { - if (isReady(future)) { - long duration = delegate.expireAfterCreate(key, future.join(), currentTime); - return Math.min(duration, MAXIMUM_EXPIRY); - } - return Long.MAX_VALUE; + return isReady(future) + ? delegate.expireAfterCreate(key, future.join(), currentTime) + : Long.MAX_VALUE; } @Override public long expireAfterUpdate(K key, CompletableFuture future, long currentTime, long currentDuration) { if (isReady(future)) { - long duration = (currentDuration > MAXIMUM_EXPIRY) + return (currentDuration > MAXIMUM_EXPIRY) ? delegate.expireAfterCreate(key, future.join(), currentTime) : delegate.expireAfterUpdate(key, future.join(), currentTime, currentDuration); - return Math.min(duration, MAXIMUM_EXPIRY); } return Long.MAX_VALUE; } @@ -156,11 +152,9 @@ public long expireAfterUpdate(K key, CompletableFuture future, @Override public long expireAfterRead(K key, CompletableFuture future, long currentTime, long currentDuration) { - if (isReady(future)) { - long duration = delegate.expireAfterRead(key, future.join(), currentTime, currentDuration); - return Math.min(duration, MAXIMUM_EXPIRY); - } - return Long.MAX_VALUE; + return isReady(future) + ? delegate.expireAfterRead(key, future.join(), currentTime, currentDuration) + : Long.MAX_VALUE; } Object writeReplace() { 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 fbedc3c0b9..23877a37b6 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 @@ -154,6 +154,8 @@ abstract class BoundedLocalCache extends BLCHeader.DrainStatusRef static final double PERCENT_MAIN_PROTECTED = 0.80d; /** The maximum time window between entry updates before the expiration must be reordered. */ static final long EXPIRE_WRITE_TOLERANCE = TimeUnit.SECONDS.toNanos(1); + /** The maximum duration before an entry expires. */ + static final long MAXIMUM_EXPIRY = (Long.MAX_VALUE >> 1); // 150 years final ConcurrentHashMap> data; @Nullable final CacheLoader cacheLoader; @@ -866,7 +868,7 @@ void refreshIfNeeded(Node node, long now) { K key; V oldValue; long oldWriteTime = node.getWriteTime(); - long refreshWriteTime = (now + Async.MAXIMUM_EXPIRY); + long refreshWriteTime = (now + MAXIMUM_EXPIRY); if (((now - oldWriteTime) > refreshAfterWriteNanos()) && ((key = node.getKey()) != null) && ((oldValue = node.getValue()) != null) && node.casWriteTime(oldWriteTime, refreshWriteTime)) { @@ -941,7 +943,7 @@ void refreshIfNeeded(Node node, long now) { long expireAfterCreate(@Nullable K key, @Nullable V value, Expiry expiry, long now) { if (expiresVariable() && (key != null) && (value != null)) { long duration = expiry.expireAfterCreate(key, value, now); - return (now + duration); + return (now + Math.min(duration, MAXIMUM_EXPIRY)); } return 0L; } @@ -961,7 +963,7 @@ long expireAfterUpdate(Node node, @Nullable K key, if (expiresVariable() && (key != null) && (value != null)) { long currentDuration = Math.max(1, node.getVariableTime() - now); long duration = expiry.expireAfterUpdate(key, value, now, currentDuration); - return (now + duration); + return (now + Math.min(duration, MAXIMUM_EXPIRY)); } return 0L; } @@ -981,7 +983,7 @@ long expireAfterRead(Node node, @Nullable K key, if (expiresVariable() && (key != null) && (value != null)) { long currentDuration = Math.max(1, node.getVariableTime() - now); long duration = expiry.expireAfterRead(key, value, now, currentDuration); - return (now + duration); + return (now + Math.min(duration, MAXIMUM_EXPIRY)); } return 0L; } @@ -1340,7 +1342,7 @@ public void run() { if (isComputingAsync(node)) { synchronized (node) { if (!Async.isReady((CompletableFuture) node.getValue())) { - long expirationTime = expirationTicker().read() + Long.MAX_VALUE; + long expirationTime = expirationTicker().read() + MAXIMUM_EXPIRY; setVariableTime(node, expirationTime); setAccessTime(node, expirationTime); setWriteTime(node, expirationTime); @@ -3255,7 +3257,7 @@ final class BoundedVarExpiration implements VarExpiration { long durationNanos = TimeUnit.NANOSECONDS.convert(duration, unit); synchronized (node) { now = cache.expirationTicker().read(); - node.setVariableTime(now + durationNanos); + node.setVariableTime(now + Math.max(durationNanos, MAXIMUM_EXPIRY)); } cache.afterRead(node, now, /* recordHit */ false); } diff --git a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/AsyncTest.java b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/AsyncTest.java index 3086aa0e03..5211857c04 100644 --- a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/AsyncTest.java +++ b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/AsyncTest.java @@ -15,7 +15,6 @@ */ package com.github.benmanes.caffeine.cache; -import static com.github.benmanes.caffeine.cache.Async.MAXIMUM_EXPIRY; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; @@ -138,17 +137,6 @@ public void asyncExpiry_completed() { verify(expiry.delegate).expireAfterRead(0, 100, 1L, 2L); } - @Test - public void asyncExpiry_bounded() { - AsyncExpiry expiry = makeAsyncExpiry( - Long.MAX_VALUE, Long.MAX_VALUE, Long.MAX_VALUE); - CompletableFuture future = CompletableFuture.completedFuture(100); - - assertThat(expiry.expireAfterCreate(0, future, 1L), is(MAXIMUM_EXPIRY)); - assertThat(expiry.expireAfterUpdate(0, future, 1L, 2L), is(MAXIMUM_EXPIRY)); - assertThat(expiry.expireAfterRead(0, future, 1L, 2L), is(MAXIMUM_EXPIRY)); - } - @DataProvider(name = "successful") public Object[][] providesSuccessful() { return new Object[][] {{ CompletableFuture.completedFuture(1) }}; diff --git a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/ExpireAfterVarTest.java b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/ExpireAfterVarTest.java index 21ffc8af83..4ae80806a0 100644 --- a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/ExpireAfterVarTest.java +++ b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/ExpireAfterVarTest.java @@ -17,6 +17,7 @@ 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.Awaits.await; import static com.github.benmanes.caffeine.testing.IsEmptyMap.emptyMap; import static com.github.benmanes.caffeine.testing.IsFutureValue.futureOf; import static java.util.concurrent.TimeUnit.NANOSECONDS; @@ -27,6 +28,7 @@ import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.nullValue; +import static org.hamcrest.Matchers.sameInstance; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.anyLong; @@ -42,6 +44,7 @@ import java.util.Set; import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; import org.testng.annotations.Listeners; import org.testng.annotations.Test; @@ -59,6 +62,7 @@ import com.github.benmanes.caffeine.cache.testing.CacheValidationListener; import com.github.benmanes.caffeine.cache.testing.CheckNoStats; import com.github.benmanes.caffeine.cache.testing.CheckNoWriter; +import com.github.benmanes.caffeine.testing.ConcurrentTestHarness; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; @@ -72,6 +76,34 @@ @Test(dataProviderClass = CacheProvider.class) public final class ExpireAfterVarTest { + @Test(dataProvider = "caches") + @CacheSpec(expiryTime = Expire.FOREVER, + expiry = { CacheExpiry.CREATE, CacheExpiry.WRITE, CacheExpiry.ACCESS }) + public void expiry_bounds(Cache cache, CacheContext context) { + context.ticker().advance(System.nanoTime()); + AtomicBoolean running = new AtomicBoolean(); + AtomicBoolean done = new AtomicBoolean(); + Integer key = context.absentKey(); + cache.put(key, key); + + try { + ConcurrentTestHarness.execute(() -> { + while (!done.get()) { + context.ticker().advance(1, TimeUnit.MINUTES); + cache.get(key, Integer::new); + running.set(true); + } + }); + await().untilTrue(running); + cache.cleanUp(); + done.set(true); + + assertThat(cache.get(key, Integer::new), sameInstance(key)); + } finally { + done.set(true); + } + } + /* ---------------- Create -------------- */ @Test(dataProvider = "caches") diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index c54915907b..03fb14e1dd 100644 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -59,7 +59,7 @@ ext { concurrentlinkedhashmap: '1.4.2', ehcache2: '2.10.4', ehcache3: '3.4.0', - elasticSearch: '6.2.1', + elasticSearch: '6.2.2', expiringMap: '0.5.8', jackrabbit: '1.8.2', jamm: '0.3.2',