From 476e568b83076bd88b1204e0e4a15d7887d25d11 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 | 11 ++++--- .../caffeine/cache/BoundedLocalCache.java | 22 ++++++------- .../benmanes/caffeine/cache/AsyncTest.java | 9 +++--- .../caffeine/cache/ExpireAfterVarTest.java | 32 +++++++++++++++++++ gradle/dependencies.gradle | 2 +- 5 files changed, 55 insertions(+), 21 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..40cba95913 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,7 @@ * @author ben.manes@gmail.com (Ben Manes) */ final class Async { - static final long MAXIMUM_EXPIRY = (Long.MAX_VALUE >> 1); // 150 years + static final long ASYNC_EXPIRY = (Long.MAX_VALUE >> 1) + (Long.MAX_VALUE >> 2); // 220 years private Async() {} @@ -118,7 +119,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 + * {@code ASYNC_EXPIRY} 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. @@ -138,7 +139,7 @@ public long expireAfterCreate(K key, CompletableFuture future, long currentTi long duration = delegate.expireAfterCreate(key, future.join(), currentTime); return Math.min(duration, MAXIMUM_EXPIRY); } - return Long.MAX_VALUE; + return ASYNC_EXPIRY; } @Override @@ -150,7 +151,7 @@ public long expireAfterUpdate(K key, CompletableFuture future, : delegate.expireAfterUpdate(key, future.join(), currentTime, currentDuration); return Math.min(duration, MAXIMUM_EXPIRY); } - return Long.MAX_VALUE; + return ASYNC_EXPIRY; } @Override @@ -160,7 +161,7 @@ public long expireAfterRead(K key, CompletableFuture future, long duration = delegate.expireAfterRead(key, future.join(), currentTime, currentDuration); return Math.min(duration, MAXIMUM_EXPIRY); } - return Long.MAX_VALUE; + return ASYNC_EXPIRY; } 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..93e7ba69cd 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 @@ -15,6 +15,7 @@ */ package com.github.benmanes.caffeine.cache; +import static com.github.benmanes.caffeine.cache.Async.ASYNC_EXPIRY; import static com.github.benmanes.caffeine.cache.Caffeine.requireArgument; import static com.github.benmanes.caffeine.cache.Caffeine.requireState; import static com.github.benmanes.caffeine.cache.Node.EDEN; @@ -154,6 +155,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; @@ -725,12 +728,9 @@ void expireVariableEntries(long now) { /** Returns if the entry has expired. */ boolean hasExpired(Node node, long now) { - if (isComputingAsync(node)) { - return false; - } return (expiresAfterAccess() && (now - node.getAccessTime() >= expiresAfterAccessNanos())) - || (expiresAfterWrite() && (now - node.getWriteTime() >= expiresAfterWriteNanos())) - || (expiresVariable() && (now - node.getVariableTime() >= 0)); + | (expiresAfterWrite() && (now - node.getWriteTime() >= expiresAfterWriteNanos())) + | (expiresVariable() && (now - node.getVariableTime() >= 0)); } /** @@ -866,7 +866,7 @@ void refreshIfNeeded(Node node, long now) { K key; V oldValue; long oldWriteTime = node.getWriteTime(); - long refreshWriteTime = (now + Async.MAXIMUM_EXPIRY); + long refreshWriteTime = (now + ASYNC_EXPIRY); if (((now - oldWriteTime) > refreshAfterWriteNanos()) && ((key = node.getKey()) != null) && ((oldValue = node.getValue()) != null) && node.casWriteTime(oldWriteTime, refreshWriteTime)) { @@ -941,7 +941,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 isAsync ? (now + duration) : (now + Math.min(duration, MAXIMUM_EXPIRY)); } return 0L; } @@ -961,7 +961,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 isAsync ? (now + duration) : (now + Math.min(duration, MAXIMUM_EXPIRY)); } return 0L; } @@ -981,7 +981,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 isAsync ? (now + duration) : (now + Math.min(duration, MAXIMUM_EXPIRY)); } return 0L; } @@ -1340,7 +1340,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() + ASYNC_EXPIRY; setVariableTime(node, expirationTime); setAccessTime(node, expirationTime); setWriteTime(node, expirationTime); @@ -3255,7 +3255,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.min(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..94e7bbc938 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,8 @@ */ package com.github.benmanes.caffeine.cache; -import static com.github.benmanes.caffeine.cache.Async.MAXIMUM_EXPIRY; +import static com.github.benmanes.caffeine.cache.Async.ASYNC_EXPIRY; +import static com.github.benmanes.caffeine.cache.BoundedLocalCache.MAXIMUM_EXPIRY; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; @@ -112,13 +113,13 @@ public void asyncExpiry_pending() { AsyncExpiry expiry = makeAsyncExpiry(ONE_MINUTE, ONE_MINUTE, ONE_MINUTE); CompletableFuture future = new CompletableFuture(); - assertThat(expiry.expireAfterCreate(0, future, 1L), is(Long.MAX_VALUE)); + assertThat(expiry.expireAfterCreate(0, future, 1L), is(ASYNC_EXPIRY)); verify(expiry.delegate, never()).expireAfterCreate(any(), any(), anyLong()); - assertThat(expiry.expireAfterUpdate(0, future, 1L, 2L), is(Long.MAX_VALUE)); + assertThat(expiry.expireAfterUpdate(0, future, 1L, 2L), is(ASYNC_EXPIRY)); verify(expiry.delegate, never()).expireAfterUpdate(any(), any(), anyLong(), anyLong()); - assertThat(expiry.expireAfterRead(0, future, 1L, 2L), is(Long.MAX_VALUE)); + assertThat(expiry.expireAfterRead(0, future, 1L, 2L), is(ASYNC_EXPIRY)); verify(expiry.delegate, never()).expireAfterRead(any(), any(), anyLong(), anyLong()); } 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',