mirror of
https://github.com/spring-projects/spring-framework.git
synced 2026-09-17 08:24:13 +00:00
Prevent double size decrement in ConcurrentLruCache
markAsRemoved() transitions a node to the removed state and decrements the current size, but it did not check whether the node had already been removed. The eviction path and an explicit removal can process the same node in sequence: when a write drain runs a queued AddTask whose eviction polls a node that a concurrent remove(K) has already taken out of the cache, the eviction decrements the size, and the queued RemovalTask for the same node decrements it again. The sibling transition markForRemoval() guards against invalid transitions; this one did not. Each extra decrement makes currentSize permanently smaller than the number of cached entries, so eviction stops triggering and the cache exceeds its capacity for good, silently. A bounded two-thread stress run accumulates the drift reliably: before the change the cache stabilized far above its capacity in 20 out of 20 runs. markAsRemoved() now returns without decrementing when the entry is already in the removed state, mirroring the guard in markForRemoval(). The removed state is terminal, so each node is counted down exactly once. The new test races explicit removals against eviction and then verifies that the cache converges back to its capacity; it also asserts that the racing thread ran and terminated cleanly. Closes gh-37268 Signed-off-by: junhyeong9812 <pickjog@gmail.com>
This commit is contained in:
@@ -199,11 +199,15 @@ public final class ConcurrentLruCache<K, V> {
|
||||
}
|
||||
|
||||
/*
|
||||
* Transition the node to the {@code removed} state and decrement the current size of the cache.
|
||||
* Transition the node to the {@code removed} state and decrement the
|
||||
* current size of the cache, unless the node has already been removed.
|
||||
*/
|
||||
private void markAsRemoved(Node<K, V> node) {
|
||||
for (; ; ) {
|
||||
CacheEntry<V> current = node.get();
|
||||
if (current.state == CacheEntryState.REMOVED) {
|
||||
return;
|
||||
}
|
||||
CacheEntry<V> removed = new CacheEntry<>(current.value, CacheEntryState.REMOVED);
|
||||
if (node.compareAndSet(current, removed)) {
|
||||
this.currentSize.lazySet(this.currentSize.get() - 1);
|
||||
|
||||
@@ -16,6 +16,10 @@
|
||||
|
||||
package org.springframework.util;
|
||||
|
||||
import java.util.concurrent.atomic.AtomicBoolean;
|
||||
import java.util.concurrent.atomic.AtomicInteger;
|
||||
import java.util.concurrent.atomic.AtomicReference;
|
||||
|
||||
import org.junit.jupiter.api.Test;
|
||||
|
||||
import static org.assertj.core.api.Assertions.assertThat;
|
||||
@@ -108,4 +112,45 @@ class ConcurrentLruCacheTests {
|
||||
assertThat(this.cache.contains("k3")).isTrue();
|
||||
}
|
||||
|
||||
@Test
|
||||
void removeRacingWithEvictionDoesNotExceedCapacity() throws Exception {
|
||||
ConcurrentLruCache<Integer, String> cache = new ConcurrentLruCache<>(2, key -> "value" + key);
|
||||
AtomicBoolean stop = new AtomicBoolean();
|
||||
AtomicInteger removals = new AtomicInteger();
|
||||
AtomicReference<Throwable> failure = new AtomicReference<>();
|
||||
Thread remover = new Thread(() -> {
|
||||
try {
|
||||
while (!stop.get()) {
|
||||
cache.get(0);
|
||||
if (cache.remove(0)) {
|
||||
removals.incrementAndGet();
|
||||
}
|
||||
}
|
||||
}
|
||||
catch (Throwable ex) {
|
||||
failure.set(ex);
|
||||
}
|
||||
});
|
||||
remover.start();
|
||||
try {
|
||||
for (int i = 1; i <= 50_000; i++) {
|
||||
cache.get(i);
|
||||
}
|
||||
}
|
||||
finally {
|
||||
stop.set(true);
|
||||
remover.join(5000);
|
||||
}
|
||||
int budget = 50_000;
|
||||
int key = 100_000;
|
||||
while (cache.size() > cache.capacity() && budget-- > 0) {
|
||||
cache.get(key++);
|
||||
}
|
||||
|
||||
assertThat(remover.isAlive()).isFalse();
|
||||
assertThat(failure.get()).isNull();
|
||||
assertThat(removals.get()).isGreaterThan(0);
|
||||
assertThat(cache.size()).isLessThanOrEqualTo(cache.capacity());
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user