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 91d011ba03cad..0db4d718709d8 100644 --- a/core/src/main/java/org/elasticsearch/common/cache/Cache.java +++ b/core/src/main/java/org/elasticsearch/common/cache/Cache.java @@ -206,34 +206,33 @@ private static class CacheSegment { */ Entry get(K key, long now, Predicate> isExpired, Consumer> onExpiration) { CompletableFuture> future; - Entry entry = null; try (ReleasableLock ignored = readLock.acquire()) { future = map.get(key); } if (future != null) { + Entry entry; try { - entry = future.handle((ok, ex) -> { - if (ok != null && !isExpired.test(ok)) { - segmentStats.hit(); - ok.accessTime = now; - return ok; - } else { - segmentStats.miss(); - if (ok != null) { - assert isExpired.test(ok); - onExpiration.accept(ok); - } - return null; - } - }).get(); - } catch (ExecutionException | InterruptedException e) { + entry = future.get(); + } catch (ExecutionException e) { + assert future.isCompletedExceptionally(); + segmentStats.miss(); + return null; + } catch (InterruptedException e) { throw new IllegalStateException(e); } - } - else { + if (isExpired.test(entry)) { + segmentStats.miss(); + onExpiration.accept(entry); + return null; + } else { + segmentStats.hit(); + entry.accessTime = now; + return entry; + } + } else { segmentStats.miss(); + return null; } - return entry; } /** @@ -269,30 +268,18 @@ Tuple, Entry> put(K key, V value, long now) { /** * remove an entry from the segment * - * @param key the key of the entry to remove from the cache - * @return the removed entry if there was one, otherwise null + * @param key the key of the entry to remove from the cache + * @param onRemoval a callback for the removed entry */ - Entry remove(K key) { + void remove(K key, Consumer>> onRemoval) { CompletableFuture> future; - Entry entry = null; try (ReleasableLock ignored = writeLock.acquire()) { future = map.remove(key); } if (future != null) { - try { - entry = future.handle((ok, ex) -> { - if (ok != null) { - segmentStats.eviction(); - return ok; - } else { - return null; - } - }).get(); - } catch (ExecutionException | InterruptedException e) { - throw new IllegalStateException(e); - } + segmentStats.eviction(); + onRemoval.accept(future); } - return entry; } private static class SegmentStats { @@ -476,12 +463,18 @@ private void put(K key, V value, long now) { */ public void invalidate(K key) { CacheSegment segment = getCacheSegment(key); - Entry entry = segment.remove(key); - if (entry != null) { - try (ReleasableLock ignored = lruLock.acquire()) { - delete(entry, RemovalNotification.RemovalReason.INVALIDATED); + segment.remove(key, f -> { + try { + Entry entry = f.get(); + try (ReleasableLock ignored = lruLock.acquire()) { + delete(entry, RemovalNotification.RemovalReason.INVALIDATED); + } + } catch (ExecutionException e) { + // ok + } catch (InterruptedException e) { + throw new IllegalStateException(e); } - } + }); } /** @@ -580,7 +573,8 @@ public void remove() { /** * An LRU sequencing of the values in the cache. This sequence is not protected from mutations - * to the cache. The result of iteration under mutation is undefined. + * to the cache (except for {@link Iterator#remove()}. The result of iteration under any other mutation is + * undefined. * * @return an LRU-ordered {@link Iterable} over the values in the cache */ @@ -597,6 +591,11 @@ public boolean hasNext() { public V next() { return iterator.next().value; } + + @Override + public void remove() { + iterator.remove(); + } }; } @@ -626,7 +625,7 @@ public void remove() { Entry entry = current; if (entry != null) { CacheSegment segment = getCacheSegment(entry.key); - segment.remove(entry.key); + segment.remove(entry.key, f -> {}); try (ReleasableLock ignored = lruLock.acquire()) { current = null; delete(entry, RemovalNotification.RemovalReason.INVALIDATED); @@ -711,7 +710,7 @@ private void evictEntry(Entry entry) { CacheSegment segment = getCacheSegment(entry.key); if (segment != null) { - segment.remove(entry.key); + segment.remove(entry.key, f -> {}); } delete(entry, RemovalNotification.RemovalReason.EVICTED); } 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 334dae22500ed..9cdd5c84fade8 100644 --- a/core/src/test/java/org/elasticsearch/common/cache/CacheTests.java +++ b/core/src/test/java/org/elasticsearch/common/cache/CacheTests.java @@ -343,7 +343,6 @@ protected long now() { assertEquals(numberOfEntries, cache.stats().getEvictions()); } - @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/30428") public void testComputeIfAbsentDeadlock() throws BrokenBarrierException, InterruptedException { final int numberOfThreads = randomIntBetween(2, 32); final Cache cache =