From d9b18158ed6a48e1e46fbfb0447085dd817034fc Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Mon, 8 May 2017 09:00:53 -0400 Subject: [PATCH 1/2] Fix cache expire after access This commit fixes a bug in the cache expire after access implementation. The bug is this: if you construct a cache with an expire after access of T, put a key, and then touch the key at some time t > T, the act of getting the key would update the access time for the entry before checking if the entry was expired. There are situations in which expire after access would be honored (e.g., if the cache needs to prune the LRU list to keep the cache under a certain weight, or a manual refresh was called) but this behavior is otherwise broken. --- .../org/elasticsearch/common/cache/Cache.java | 40 ++++++++++--------- .../common/cache/CacheTests.java | 22 ++++++++++ 2 files changed, 43 insertions(+), 19 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/cache/Cache.java b/core/src/main/java/org/elasticsearch/common/cache/Cache.java index 2297df67655d9..fdb8f0914718d 100644 --- a/core/src/main/java/org/elasticsearch/common/cache/Cache.java +++ b/core/src/main/java/org/elasticsearch/common/cache/Cache.java @@ -34,6 +34,7 @@ import java.util.concurrent.locks.ReentrantLock; import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.function.BiFunction; +import java.util.function.Predicate; import java.util.function.ToLongBiFunction; /** @@ -195,31 +196,32 @@ private static class CacheSegment { /** * get an entry from the segment * - * @param key the key of the entry to get from the cache - * @param now the access time of this entry + * @param key the key of the entry to get from the cache + * @param now the access time of this entry + * @param isExpired test if the entry is expired * @return the entry if there was one, otherwise null */ - Entry get(K key, long now) { + Entry get(K key, long now, Predicate> isExpired) { CompletableFuture> future; Entry entry = null; try (ReleasableLock ignored = readLock.acquire()) { future = map.get(key); } if (future != null) { - try { - entry = future.handle((ok, ex) -> { - if (ok != null) { - segmentStats.hit(); - ok.accessTime = now; - return ok; - } else { - segmentStats.miss(); - return null; - } - }).get(); - } catch (ExecutionException | InterruptedException e) { - throw new IllegalStateException(e); - } + try { + entry = future.handle((ok, ex) -> { + if (ok != null && !isExpired.test(ok)) { + segmentStats.hit(); + ok.accessTime = now; + return ok; + } else { + segmentStats.miss(); + return null; + } + }).get(); + } catch (ExecutionException | InterruptedException e) { + throw new IllegalStateException(e); + } } else { segmentStats.miss(); @@ -332,8 +334,8 @@ public V get(K key) { private V get(K key, long now) { CacheSegment segment = getCacheSegment(key); - Entry entry = segment.get(key, now); - if (entry == null || isExpired(entry, now)) { + Entry entry = segment.get(key, now, e -> isExpired(e, now)); + if (entry == null) { return null; } else { promote(entry, now); diff --git a/core/src/test/java/org/elasticsearch/common/cache/CacheTests.java b/core/src/test/java/org/elasticsearch/common/cache/CacheTests.java index 71fe7b262a699..7dbaba02897c6 100644 --- a/core/src/test/java/org/elasticsearch/common/cache/CacheTests.java +++ b/core/src/test/java/org/elasticsearch/common/cache/CacheTests.java @@ -257,6 +257,28 @@ protected long now() { } } + public void testSimpleExpireAfterAccess() { + AtomicLong now = new AtomicLong(); + Cache cache = new Cache() { + @Override + protected long now() { + return now.get(); + } + }; + cache.setExpireAfterAccessNanos(1); + now.set(0); + for (int i = 0; i < numberOfEntries; i++) { + cache.put(i, Integer.toString(i)); + } + for (int i = 0; i < numberOfEntries; i++) { + assertEquals(cache.get(i), Integer.toString(i)); + } + now.set(2); + for(int i = 0; i < numberOfEntries; i++) { + assertNull(cache.get(i)); + } + } + public void testExpirationAfterWrite() { AtomicLong now = new AtomicLong(); Cache cache = new Cache() { From 524469986ca2e2ba70f7f6e4d1e3756f8f620e0a Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Mon, 8 May 2017 09:48:16 -0400 Subject: [PATCH 2/2] Improve Javadocs --- core/src/main/java/org/elasticsearch/common/cache/Cache.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/elasticsearch/common/cache/Cache.java b/core/src/main/java/org/elasticsearch/common/cache/Cache.java index fdb8f0914718d..df30123c35b42 100644 --- a/core/src/main/java/org/elasticsearch/common/cache/Cache.java +++ b/core/src/main/java/org/elasticsearch/common/cache/Cache.java @@ -194,7 +194,8 @@ private static class CacheSegment { SegmentStats segmentStats = new SegmentStats(); /** - * get an entry from the segment + * get an entry from the segment; expired entries will be returned as null but not removed from the cache until the LRU list is + * pruned or a manual {@link Cache#refresh()} is performed * * @param key the key of the entry to get from the cache * @param now the access time of this entry