From d498e66bf3e8b1c6fd2e2c9cc128b07c8df82370 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 8 May 2018 08:11:20 -0400 Subject: [PATCH 1/3] Avoid deadlocks in cache This commit avoids deadlocks in the cache by removing dangerous places where we try to take the LRU lock while completing a future. Instead, we block for the future to complete, and then execute the handling code under the LRU lock (for example, eviction). --- .../org/elasticsearch/common/cache/Cache.java | 79 +++++++++---------- .../common/cache/CacheTests.java | 1 - 2 files changed, 36 insertions(+), 44 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/cache/Cache.java b/server/src/main/java/org/elasticsearch/common/cache/Cache.java index 620612619104b..fbc00c8dde35e 100644 --- a/server/src/main/java/org/elasticsearch/common/cache/Cache.java +++ b/server/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) { + final 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 { + final 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); } - } + }); } /** @@ -632,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, e -> {}); try (ReleasableLock ignored = lruLock.acquire()) { current = null; delete(entry, RemovalNotification.RemovalReason.INVALIDATED); @@ -717,7 +710,7 @@ private void evictEntry(Entry entry) { CacheSegment segment = getCacheSegment(entry.key); if (segment != null) { - segment.remove(entry.key); + segment.remove(entry.key, e -> {}); } delete(entry, RemovalNotification.RemovalReason.EVICTED); } diff --git a/server/src/test/java/org/elasticsearch/common/cache/CacheTests.java b/server/src/test/java/org/elasticsearch/common/cache/CacheTests.java index 1ab38dff7eb7f..fe64fd16af68c 100644 --- a/server/src/test/java/org/elasticsearch/common/cache/CacheTests.java +++ b/server/src/test/java/org/elasticsearch/common/cache/CacheTests.java @@ -344,7 +344,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 = From ddbe45af0342ef90796e8da88bea0237c8ec2a05 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 8 May 2018 10:40:16 -0400 Subject: [PATCH 2/3] Final --- server/src/main/java/org/elasticsearch/common/cache/Cache.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/common/cache/Cache.java b/server/src/main/java/org/elasticsearch/common/cache/Cache.java index fbc00c8dde35e..7521c310d0586 100644 --- a/server/src/main/java/org/elasticsearch/common/cache/Cache.java +++ b/server/src/main/java/org/elasticsearch/common/cache/Cache.java @@ -465,7 +465,7 @@ public void invalidate(K key) { CacheSegment segment = getCacheSegment(key); segment.remove(key, f -> { try { - final Entry entry = f.get(); + Entry entry = f.get(); try (ReleasableLock ignored = lruLock.acquire()) { delete(entry, RemovalNotification.RemovalReason.INVALIDATED); } From 9344ce74907d6c5828e0cce8f3d5106d6713a348 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 8 May 2018 23:13:52 -0400 Subject: [PATCH 3/3] Variable names --- .../src/main/java/org/elasticsearch/common/cache/Cache.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/cache/Cache.java b/server/src/main/java/org/elasticsearch/common/cache/Cache.java index 7521c310d0586..0db4d718709d8 100644 --- a/server/src/main/java/org/elasticsearch/common/cache/Cache.java +++ b/server/src/main/java/org/elasticsearch/common/cache/Cache.java @@ -210,7 +210,7 @@ Entry get(K key, long now, Predicate> isExpired, Consumer entry; + Entry entry; try { entry = future.get(); } catch (ExecutionException e) { @@ -625,7 +625,7 @@ public void remove() { Entry entry = current; if (entry != null) { CacheSegment segment = getCacheSegment(entry.key); - segment.remove(entry.key, e -> {}); + segment.remove(entry.key, f -> {}); try (ReleasableLock ignored = lruLock.acquire()) { current = null; delete(entry, RemovalNotification.RemovalReason.INVALIDATED); @@ -710,7 +710,7 @@ private void evictEntry(Entry entry) { CacheSegment segment = getCacheSegment(entry.key); if (segment != null) { - segment.remove(entry.key, e -> {}); + segment.remove(entry.key, f -> {}); } delete(entry, RemovalNotification.RemovalReason.EVICTED); }