Skip to content

Commit

Permalink
Fix variable expiration overflowing with the maximum duration (fixes #…
Browse files Browse the repository at this point in the history 
…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.
  • Loading branch information
@ben-manes
ben-manes committed Feb 21, 2018
1 parent 20d9e0c commit 1119253
Show file tree
Hide file tree
Showing 5 changed files with 56 additions and 22 deletions.
11 changes: 6 additions & 5 deletions caffeine/src/main/java/com/github/benmanes/caffeine/cache/Async.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -31,7 +32,7 @@
* @author [email protected] (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() {}

Expand Down Expand Up @@ -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.
Expand All @@ -138,7 +139,7 @@ public long expireAfterCreate(K key, CompletableFuture<V> 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
Expand All @@ -150,7 +151,7 @@ public long expireAfterUpdate(K key, CompletableFuture<V> future,
: delegate.expireAfterUpdate(key, future.join(), currentTime, currentDuration);
return Math.min(duration, MAXIMUM_EXPIRY);
}
return Long.MAX_VALUE;
return ASYNC_EXPIRY;
}

@Override
Expand All @@ -160,7 +161,7 @@ public long expireAfterRead(K key, CompletableFuture<V> 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() {
Expand Down
23 changes: 12 additions & 11 deletions caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -154,6 +155,8 @@ abstract class BoundedLocalCache<K, V> extends BLCHeader.DrainStatusRef<K, V>
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<Object, Node<K, V>> data;
@Nullable final CacheLoader<K, V> cacheLoader;
Expand Down Expand Up @@ -724,13 +727,11 @@ void expireVariableEntries(long now) {
}

/** Returns if the entry has expired. */
@SuppressWarnings("ShortCircuitBoolean")
boolean hasExpired(Node<K, V> 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));
}

/**
Expand Down Expand Up @@ -866,7 +867,7 @@ void refreshIfNeeded(Node<K, V> 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)) {
Expand Down Expand Up @@ -941,7 +942,7 @@ void refreshIfNeeded(Node<K, V> node, long now) {
long expireAfterCreate(@Nullable K key, @Nullable V value, Expiry<K, V> 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;
}
Expand All @@ -961,7 +962,7 @@ long expireAfterUpdate(Node<K, V> 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;
}
Expand All @@ -981,7 +982,7 @@ long expireAfterRead(Node<K, V> 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;
}
Expand Down Expand Up @@ -1340,7 +1341,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);
Expand Down Expand Up @@ -3255,7 +3256,7 @@ final class BoundedVarExpiration implements VarExpiration<K, V> {
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);
}
Expand Down
9 changes: 5 additions & 4 deletions caffeine/src/test/java/com/github/benmanes/caffeine/cache/AsyncTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -112,13 +113,13 @@ public void asyncExpiry_pending() {
AsyncExpiry<Integer, Integer> expiry = makeAsyncExpiry(ONE_MINUTE, ONE_MINUTE, ONE_MINUTE);
CompletableFuture<Integer> future = new CompletableFuture<Integer>();

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());
}

Expand Down
31 changes: 31 additions & 0 deletions caffeine/src/test/java/com/github/benmanes/caffeine/cache/ExpireAfterVarTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -72,6 +76,33 @@
@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<Integer, Integer> 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();

assertThat(cache.get(key, Integer::new), sameInstance(key));
} finally {
done.set(true);
}
}

/* ---------------- Create -------------- */

@Test(dataProvider = "caches")
Expand Down
4 changes: 2 additions & 2 deletions gradle/dependencies.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ ext {
akka: '2.5.9',
commonsCompress: '1.16.1',
commonsLang3: '3.7',
config: '1.3.2',
config: '1.3.3',
errorProne: '2.2.0',
fastutil: '8.1.1',
flipTables: '1.0.2',
Expand Down Expand Up @@ -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',
Expand Down

0 comments on commit 1119253

Please sign in to comment.