mirror of
https://github.com/spring-projects/spring-framework.git
synced 2026-09-23 22:59:03 +00:00
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
This commit is contained in:
@@ -180,18 +180,22 @@ public final class ConcurrentLruCache<K, V> {
|
||||
}
|
||||
|
||||
/**
|
||||
* Immediately remove all entries from this cache.
|
||||
* Remove all entries from this cache.
|
||||
* <p>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<K, V> 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();
|
||||
|
||||
@@ -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<Integer, String> cache = new ConcurrentLruCache<>(2, key -> "value" + key);
|
||||
|
||||
Reference in New Issue
Block a user