From bb7ea37f1b3d47a4309bfacd7511cd1c6f4c2297 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Thu, 17 Sep 2026 12:13:28 +0200 Subject: [PATCH] Drain pending writes before evicting in ConcurrentLruCache.clear() Prior to this commit, clear() polled the eviction queue to remove entries and only afterward drained the pending write operations queue. Consequently, a put() whose AddTask had not yet been linked into the eviction queue -- for example, because it lost the race to self-drain while clear() held the eviction lock -- would only be applied by that trailing drain, linking the entry into the eviction queue right after clear() had already finished removing everything it could see. The practical effect was that an entry already fully added to the cache could still be present immediately after clear() returned, with no further concurrent activity required at that point. To address that, this commit revises clear() so that it drains the pending write operations queue before polling the eviction queue, so any write that was already queued gets cleaned up along with everything else. However, a put() that is genuinely concurrent with an in-progress clear() call can still survive, which is consistent with the cache's weak-consistency design. Thanks to @guanchengang for raising gh-37286, which prompted this fix. Closes gh-37287 --- .../util/ConcurrentLruCache.java | 8 +++-- .../util/ConcurrentLruCacheTests.java | 36 +++++++++++++++++-- 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/spring-core/src/main/java/org/springframework/util/ConcurrentLruCache.java b/spring-core/src/main/java/org/springframework/util/ConcurrentLruCache.java index 6c27ec2b928..acf38125ad3 100644 --- a/spring-core/src/main/java/org/springframework/util/ConcurrentLruCache.java +++ b/spring-core/src/main/java/org/springframework/util/ConcurrentLruCache.java @@ -180,18 +180,22 @@ public final class ConcurrentLruCache { } /** - * Immediately remove all entries from this cache. + * Remove all entries from this cache. + *

This method does not block or synchronize with in-flight + * writes. An entry added concurrently while this method is running + * may still survive this call, as if it had been added immediately + * afterward. */ public void clear() { this.evictionLock.lock(); try { + this.writeOperations.drainAll(); Node node; while ((node = this.evictionQueue.poll()) != null) { this.cache.remove(node.key, node); markAsRemoved(node); } this.readOperations.clear(); - this.writeOperations.drainAll(); } finally { this.evictionLock.unlock(); diff --git a/spring-core/src/test/java/org/springframework/util/ConcurrentLruCacheTests.java b/spring-core/src/test/java/org/springframework/util/ConcurrentLruCacheTests.java index b4dfadf08e0..c6e2ef56f80 100644 --- a/spring-core/src/test/java/org/springframework/util/ConcurrentLruCacheTests.java +++ b/spring-core/src/test/java/org/springframework/util/ConcurrentLruCacheTests.java @@ -16,9 +16,11 @@ package org.springframework.util; +import java.lang.reflect.Field; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; +import java.util.concurrent.locks.Lock; import org.junit.jupiter.api.Test; @@ -61,7 +63,7 @@ class ConcurrentLruCacheTests { @Test void getAndSize() { assertThat(this.cache.capacity()).isEqualTo(2); - assertThat(this.cache.size()).isEqualTo(0); + assertThat(this.cache.size()).isZero(); assertThat(this.cache.get("k1")).isEqualTo("k1value"); assertThat(this.cache.size()).isEqualTo(1); assertThat(this.cache.contains("k1")).isTrue(); @@ -102,7 +104,7 @@ class ConcurrentLruCacheTests { assertThat(this.cache.contains("k1")).isTrue(); assertThat(this.cache.contains("k2")).isTrue(); this.cache.clear(); - assertThat(this.cache.size()).isEqualTo(0); + assertThat(this.cache.size()).isZero(); assertThat(this.cache.contains("k1")).isFalse(); assertThat(this.cache.contains("k2")).isFalse(); assertThat(this.cache.get("k3")).isEqualTo("k3value"); @@ -112,6 +114,36 @@ class ConcurrentLruCacheTests { assertThat(this.cache.contains("k3")).isTrue(); } + @Test // gh-37287 + void clearRemovesEntryWithPendingWriteOperation() throws Exception { + String key = "k1"; + + // Hold the eviction lock so that the put() triggered below cannot drain + // its own AddTask and is left pending in the write operations queue. + Field evictionLockField = ConcurrentLruCache.class.getDeclaredField("evictionLock"); + evictionLockField.setAccessible(true); + Lock evictionLock = (Lock) evictionLockField.get(this.cache); + + evictionLock.lock(); + try { + Thread putTrigger = new Thread(() -> this.cache.get(key)); + putTrigger.start(); + putTrigger.join(5000); + assertThat(putTrigger.isAlive()).isFalse(); + } + finally { + evictionLock.unlock(); + } + + assertThat(this.cache.size()).as("cache size").isEqualTo(1); + assertThat(this.cache.contains(key)).as("contains %s", key).isTrue(); + + this.cache.clear(); + + assertThat(this.cache.size()).as("cache size").isZero(); + assertThat(this.cache.contains(key)).as("contains %s", key).isFalse(); + } + @Test void removeRacingWithEvictionDoesNotExceedCapacity() throws Exception { ConcurrentLruCache cache = new ConcurrentLruCache<>(2, key -> "value" + key);