Fix calculation of JCache's access expiration (fixes #158)

If the entry had been eternal then, on access, the current time was
set to a duration with the unit of nanosecond instead of milliseconds.

The JCache expiration time sometimes assumed wall-clock behavior and
not nanoTime's. For writes there are still 3 magic values and those
will be removed in the v3 JCache rewrite (to use Caffeine's variable
expiration).

Improved the documentation for Expiry. Unfortunately it would probably
be best if the currentTime parameter was not provided. This was free
as already obtained from the ticker, but that defaults to nanoTime and
is not a wall-clock value. The main use cases for the parameter would be
to calculate against an external resource's timestamp, but that must be
done with a wall-clock resolution (System.currentTimeMillis). The focus
on implementation details and writing docs late meant this API gaffe
was overlooked, resulting in hinting towards an error prone usage.

caffeine/src/main/java/com/github/benmanes/caffeine/cache/Expiry.java

@@ -15,6 +15,8 @@
  */
 package com.github.benmanes.caffeine.cache;
 
+import javax.annotation.Nonnegative;
+import javax.annotation.Nonnull;
 import javax.annotation.concurrent.ThreadSafe;
 
 /**
@@ -30,39 +32,53 @@ public interface Expiry<K, V> {
    * Specifies that the entry should be automatically removed from the cache once the duration has
    * elapsed after the entry's creation. To indicate no expiration an entry may be given an
    * excessively long period, such as {@code Long#MAX_VALUE}.
+   * <p>
+   * <b>Note:</b> The {@code currentTime} is supplied by the configured {@link Ticker} and by
+   * default does not relate to system or wall-clock time. When calculating the duration based on a
+   * time stamp, the current time should be obtained independently.
    *
    * @param key the key represented by this entry
    * @param value the value represented by this entry
    * @param currentTime the current time, in nanoseconds
    * @return the length of time before the entry expires, in nanoseconds
    */
-  long expireAfterCreate(K key, V value, long currentTime);
+  long expireAfterCreate(@Nonnull K key, @Nonnull V value, long currentTime);
 
   /**
    * Specifies that the entry should be automatically removed from the cache once the duration has
    * elapsed after the replacement of its value. To indicate no expiration an entry may be given an
    * excessively long period, such as {@code Long#MAX_VALUE}. The {@code currentDuration} may be
    * returned to not modify the expiration time.
+   * <p>
+   * <b>Note:</b> The {@code currentTime} is supplied by the configured {@link Ticker} and by
+   * default does not relate to system or wall-clock time. When calculating the duration based on a
+   * time stamp, the current time should be obtained independently.
    *
    * @param key the key represented by this entry
    * @param value the value represented by this entry
    * @param currentTime the current time, in nanoseconds
    * @param currentDuration the current duration, in nanoseconds
    * @return the length of time before the entry expires, in nanoseconds
    */
-  long expireAfterUpdate(K key, V value, long currentTime, long currentDuration);
+  long expireAfterUpdate(@Nonnull K key, @Nonnull V value,
+      long currentTime, @Nonnegative long currentDuration);
 
   /**
    * Specifies that the entry should be automatically removed from the cache once the duration has
    * elapsed after its last read. To indicate no expiration an entry may be given an excessively
    * long period, such as {@code Long#MAX_VALUE}. The {@code currentDuration} may be returned to not
    * modify the expiration time.
+   * <p>
+   * <b>Note:</b> The {@code currentTime} is supplied by the configured {@link Ticker} and by
+   * default does not relate to system or wall-clock time. When calculating the duration based on a
+   * time stamp, the current time should be obtained independently.
    *
    * @param key the key represented by this entry
    * @param value the value represented by this entry
    * @param currentTime the current time, in nanoseconds
    * @param currentDuration the current duration, in nanoseconds
    * @return the length of time before the entry expires, in nanoseconds
    */
-  long expireAfterRead(K key, V value, long currentTime, long currentDuration);
+  long expireAfterRead(@Nonnull K key, @Nonnull V value,
+      long currentTime, @Nonnegative long currentDuration);
 }

caffeine/src/test/java/com/github/benmanes/caffeine/cache/TimerWheelTest.java

@@ -15,6 +15,7 @@
  */
 package com.github.benmanes.caffeine.cache;
 
+import static com.github.benmanes.caffeine.cache.TimerWheel.SPANS;
 import static java.util.stream.Collectors.toList;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.is;
@@ -118,7 +119,7 @@ public void schedule_fuzzy(long clock, long nanos, long[] times) {
   public Object[][] providesFuzzySchedule() {
     long[] times = new long[5_000];
     long clock = ThreadLocalRandom.current().nextLong();
-    long bound = clock + TimeUnit.DAYS.toNanos(10);
+    long bound = clock + TimeUnit.DAYS.toNanos(1) + SPANS[SPANS.length - 1];
     for (int i = 0; i < times.length; i++) {
       times[i] = ThreadLocalRandom.current().nextLong(clock + 1, bound);
     }

gradle/dependencies.gradle

@@ -26,14 +26,14 @@
 ext {
   versions = [
     akka: '2.5.1',
-    commons_compress: '1.13',
+    commons_compress: '1.14',
     commons_lang3: '3.5',
     config: '1.3.1',
     error_prone_annotations: '2.0.19',
     fastutil: '7.2.0',
     flip_tables: '1.0.2',
     guava: '21.0',
-    javapoet: '1.8.0',
+    javapoet: '1.9.0',
     jcache: '1.0.0',
     jsr305: '3.0.2',
     jsr330: '1',
@@ -50,7 +50,7 @@ ext {
     jctools: '2.0.1',
     junit: '4.12',
     mockito: '2.8.9',
-    pax_exam: '4.10.0',
+    pax_exam: '4.11.0',
     testng: '6.11',
     truth: '0.24',
   ]
@@ -69,10 +69,10 @@ ext {
     ohc: '0.6.1',
     rapidoid: '5.3.4',
     slf4j: '1.7.25',
-    tcache: '1.0.0',
+    tcache: '1.0.1',
   ]
   plugin_versions = [
-    buildscan: '1.7',
+    buildscan: '1.7.1',
     buildscan_recipes: '0.2.0',
     checkstyle: '7.7',
     coveralls: '2.8.1',
@@ -83,7 +83,7 @@ ext {
     huntbugs_plugin: '0.3.5',
     jacoco: '0.7.9',
     jmh: '0.3.1',
-    jmh_report: '0.1.0',
+    jmh_report: '0.2.0',
     nexus: '2.3.1',
     pmd: '5.6.1',
     semantic_versioning: '1.1.0',

jcache/src/main/java/com/github/benmanes/caffeine/jcache/CacheProxy.java

@@ -16,6 +16,8 @@
 package com.github.benmanes.caffeine.jcache;
 
 import static java.util.Objects.requireNonNull;
+import static java.util.stream.Collectors.toList;
+import static java.util.stream.Collectors.toMap;
 
 import java.util.ArrayList;
 import java.util.HashMap;
@@ -34,7 +36,6 @@
 import java.util.function.Supplier;
 import java.util.logging.Level;
 import java.util.logging.Logger;
-import java.util.stream.Collectors;
 
 import javax.annotation.Nullable;
 import javax.cache.Cache;
@@ -275,7 +276,7 @@ private void loadAllAndReplaceExisting(Set<? extends K> keys) {
   private void loadAllAndKeepExisting(Set<? extends K> keys) {
     List<K> keysToLoad = keys.stream()
         .filter(key -> !cache.asMap().containsKey(key))
-        .collect(Collectors.<K>toList());
+        .collect(toList());
     Map<K, V> result = cacheLoader.get().loadAll(keysToLoad);
     for (Map.Entry<K, V> entry : result.entrySet()) {
       if ((entry.getKey() != null) && (entry.getValue() != null)) {
@@ -352,10 +353,8 @@ protected V putNoCopyOrAwait(K key, V value, boolean publishToWriter, int[] puts
         expirable = null;
       }
       boolean created = (expirable == null);
-      long expireTimeMS = expireTimeMS(created
-          ? expiry::getExpiryForCreation
-          : expiry::getExpiryForUpdate);
-      if (expireTimeMS < 0) {
+      long expireTimeMS = getWriteExpireTimeMS(created);
+      if (expireTimeMS == Long.MIN_VALUE) {
         expireTimeMS = expirable.getExpireTimeMS();
       }
       if (expireTimeMS == 0) {
@@ -441,7 +440,7 @@ private boolean putIfAbsentNoAwait(K key, V value, boolean publishToWriter) {
       }
 
       absent[0] = true;
-      long expireTimeMS = expireTimeMS(expiry::getExpiryForCreation);
+      long expireTimeMS = getWriteExpireTimeMS(/* created */ true);
       if (expireTimeMS == 0) {
         return null;
       }
@@ -593,8 +592,8 @@ public boolean replace(K key, V oldValue, V newValue) {
       if (oldValue.equals(expirable.get())) {
         publishToCacheWriter(writer::write, () -> new EntryProxy<>(key, expirable.get()));
         dispatcher.publishUpdated(this, key, expirable.get(), copyOf(newValue));
-        long expireTimeMS = expireTimeMS(expiry::getExpiryForUpdate);
-        if (expireTimeMS < 0) {
+        long expireTimeMS = getWriteExpireTimeMS(/* created */ false);
+        if (expireTimeMS == Long.MIN_VALUE) {
           expireTimeMS = expirable.getExpireTimeMS();
         }
         result = new Expirable<>(newValue, expireTimeMS);
@@ -686,8 +685,8 @@ private V replaceNoCopyOrAwait(K key, V value) {
       }
 
       publishToCacheWriter(writer::write, () -> new EntryProxy<>(key, value));
-      long expireTimeMS = expireTimeMS(expiry::getExpiryForUpdate);
-      if (expireTimeMS < 0) {
+      long expireTimeMS = getWriteExpireTimeMS(/* created */ false);
+      if (expireTimeMS == Long.MIN_VALUE) {
         expireTimeMS = expirable.getExpireTimeMS();
       }
       dispatcher.publishUpdated(this, key, expirable.get(), copy);
@@ -810,10 +809,7 @@ private Expirable<V> postProcess(Expirable<V> expirable,
         }
         return expirable;
       case READ: {
-        long expireTimeMS = expireTimeMS(expiry::getExpiryForAccess);
-        if (expireTimeMS >= 0) {
-          expirable.setExpireTimeMS(expireTimeMS);
-        }
+        setAccessExpirationTime(expirable, 0L);
         return expirable;
       }
       case CREATED:
@@ -822,13 +818,13 @@ private Expirable<V> postProcess(Expirable<V> expirable,
       case LOADED:
         statistics.recordPuts(1L);
         dispatcher.publishCreated(this, entry.getKey(), entry.getValue());
-        return new Expirable<>(entry.getValue(), expireTimeMS(expiry::getExpiryForCreation));
+        return new Expirable<>(entry.getValue(), getWriteExpireTimeMS(/* created */ true));
       case UPDATED: {
         statistics.recordPuts(1L);
         publishToCacheWriter(writer::write, () -> entry);
         dispatcher.publishUpdated(this, entry.getKey(), expirable.get(), entry.getValue());
-        long expireTimeMS = expireTimeMS(expiry::getExpiryForUpdate);
-        if (expireTimeMS < 0) {
+        long expireTimeMS = getWriteExpireTimeMS(/* created */ false);
+        if (expireTimeMS == Long.MIN_VALUE) {
           expireTimeMS = expirable.getExpireTimeMS();
         }
         return new Expirable<>(entry.getValue(), expireTimeMS);
@@ -966,12 +962,12 @@ private <T> void publishToCacheWriter(Consumer<T> action, Supplier<T> data) {
 
   /** Writes all of the entries to the cache writer if write-through is enabled. */
   private <T> CacheWriterException writeAllToCacheWriter(Map<? extends K, ? extends V> map) {
-    if (!configuration.isWriteThrough()  || map.isEmpty()) {
+    if (!configuration.isWriteThrough() || map.isEmpty()) {
       return null;
     }
     List<Cache.Entry<? extends K, ? extends V>> entries = map.entrySet().stream()
         .map(entry -> new EntryProxy<>(entry.getKey(), entry.getValue()))
-        .collect(Collectors.toList());
+        .collect(toList());
     try {
       writer.writeAll(entries);
       return null;
@@ -1038,11 +1034,11 @@ protected final void requireNotClosed() {
    * Returns a deep copy of the map if value-based caching is enabled.
    *
    * @param map the mapping of keys to expirable values
-   * @return a copy of the mappings if storing by value or the same instance if by reference
+   * @return a deep or shallow copy of the mappings depending on the store by value setting
    */
   protected final Map<K, V> copyMap(Map<K, Expirable<V>> map) {
     ClassLoader classLoader = cacheManager.getClassLoader();
-    return map.entrySet().stream().collect(Collectors.toMap(
+    return map.entrySet().stream().collect(toMap(
         entry -> copier.copy(entry.getKey(), classLoader),
         entry -> copier.copy(entry.getValue().get(), classLoader)));
   }
@@ -1073,8 +1069,8 @@ protected final void setAccessExpirationTime(Expirable<?> expirable, long curren
       } else if (duration.isEternal()) {
         expirable.setExpireTimeMS(Long.MAX_VALUE);
       } else {
-        if (currentTimeMS == 0) {
-          currentTimeMS = ticker.read();
+        if (currentTimeMS == 0L) {
+          currentTimeMS = currentTimeMillis();
         }
         long expireTimeMS = duration.getAdjustedTime(currentTimeMS);
         expirable.setExpireTimeMS(expireTimeMS);
@@ -1085,30 +1081,31 @@ protected final void setAccessExpirationTime(Expirable<?> expirable, long curren
   }
 
   /**
-   * Returns the time when the entry will expire based on the supplied expiration function.
+   * Returns the time when the entry will expire.
    *
-   * @param expires the expiration function
-   * @return the time when the entry will expire, or negative if the time should not be changed
+   * @param created if the write is an insert or update
+   * @return the time when the entry will expire, zero if it should expire immediately,
+   *         Long.MIN_VALUE if it should not be changed, or Long.MAX_VALUE if eternal
    */
-  protected final long expireTimeMS(Supplier<Duration> expires) {
+  protected final long getWriteExpireTimeMS(boolean created) {
     try {
-      Duration duration = expires.get();
+      Duration duration = created ? expiry.getExpiryForCreation() : expiry.getExpiryForUpdate();
       if (duration == null) {
-        return -1;
+        return Long.MIN_VALUE;
       } else if (duration.isZero()) {
-        return 0;
+        return 0L;
       } else if (duration.isEternal()) {
         return Long.MAX_VALUE;
       }
       return duration.getAdjustedTime(currentTimeMillis());
     } catch (Exception e) {
       logger.log(Level.WARNING, "Failed to get the policy's expiration time", e);
-      return -1;
+      return Long.MIN_VALUE;
     }
   }
 
   /** An iterator to safely expose the cache entries. */
-  private final class EntryIterator implements Iterator<Cache.Entry<K, V>> {
+  final class EntryIterator implements Iterator<Cache.Entry<K, V>> {
     final Iterator<Map.Entry<K, Expirable<V>>> delegate = cache.asMap().entrySet().iterator();
     Map.Entry<K, Expirable<V>> current;
     Map.Entry<K, Expirable<V>> cursor;

jcache/src/main/java/com/github/benmanes/caffeine/jcache/Expirable.java

@@ -49,7 +49,7 @@ public void setExpireTimeMS(long expireTimeMS) {
 
   /** Returns if the value has expired and is eligible for eviction. */
   public boolean hasExpired(long currentTimeMS) {
-    return (expireTimeMS <= currentTimeMS);
+    return (currentTimeMS - expireTimeMS) >= 0;
   }
 
   /** Returns if the value will never expire. */

jcache/src/test/java/com/github/benmanes/caffeine/jcache/AbstractJCacheTest.java

@@ -44,6 +44,7 @@
  */
 @Test(singleThreaded = true)
 public abstract class AbstractJCacheTest {
+  protected static final long START_TIME_MS = System.currentTimeMillis();
   protected static final long EXPIRY_DURATION = TimeUnit.MINUTES.toMillis(1);
 
   protected static final Integer KEY_1 = 1, VALUE_1 = -1;
@@ -68,7 +69,7 @@ public void beforeClass() {
 
   @BeforeMethod(alwaysRun = true)
   public void before() {
-    ticker = new FakeTicker();
+    ticker = new FakeTicker().advance(START_TIME_MS, TimeUnit.MILLISECONDS);
     jcache = (CacheProxy<Integer, Integer>) cacheManager.createCache("jcache", getConfiguration());
     jcacheLoading = (LoadingCacheProxy<Integer, Integer>) cacheManager.createCache(
         "jcacheLoading", getLoadingConfiguration());
@@ -86,7 +87,8 @@ public void after() {
   /* ---------------- Utility methods ------------- */
 
   @Nullable
-  protected static Expirable<Integer> getExpirable(CacheProxy<Integer, Integer> cache, Integer key) {
+  protected static Expirable<Integer> getExpirable(
+      CacheProxy<Integer, Integer> cache, Integer key) {
     return cache.cache.getIfPresent(key);
   }
 

jcache/src/test/java/com/github/benmanes/caffeine/jcache/event/EventTypeFilterTest.java

@@ -53,6 +53,6 @@ public void equals() {
 
   @Test
   public void hash() {
-    filter.hashCode();
+    assertThat(filter.hashCode(), is(filter.hashCode()));
   }
 }