Skip to content

Commit 7a5fa6b

Browse files
committed
fix: make LRU2Cache.computeIfAbsent thread-safe by holding lock across entire operation
LRU2Cache.computeIfAbsent() released the lock between get() and put(), breaking the atomicity that all other methods in this class maintain. This caused duplicate function computation under concurrency and could defeat the LRU-2 promotion logic. The fix holds the lock across the entire computeIfAbsent operation, inlining the LRU-2 promotion logic to avoid nested lock acquisition.
1 parent 63fe8c3 commit 7a5fa6b

2 files changed

Lines changed: 83 additions & 5 deletions

File tree

dubbo-common/src/main/java/org/apache/dubbo/common/utils/LRU2Cache.java

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -99,12 +99,23 @@ public V put(K key, V value) {
9999

100100
@Override
101101
public V computeIfAbsent(K key, Function<? super K, ? extends V> fn) {
102-
V value = get(key);
103-
if (value == null) {
104-
value = fn.apply(key);
105-
put(key, value);
102+
lock.lock();
103+
try {
104+
V value = super.get(key);
105+
if (value == null) {
106+
value = fn.apply(key);
107+
// Inline the LRU-2 promotion logic from put() to avoid releasing the lock
108+
if (preCache.containsKey(key)) {
109+
preCache.remove(key);
110+
super.put(key, value);
111+
} else {
112+
preCache.put(key, true);
113+
}
114+
}
115+
return value;
116+
} finally {
117+
lock.unlock();
106118
}
107-
return value;
108119
}
109120

110121
@Override

dubbo-common/src/test/java/org/apache/dubbo/common/utils/LRU2CacheTest.java

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,14 @@
1616
*/
1717
package org.apache.dubbo.common.utils;
1818

19+
import java.util.concurrent.CountDownLatch;
20+
import java.util.concurrent.atomic.AtomicInteger;
21+
1922
import org.junit.jupiter.api.Test;
2023

2124
import static org.hamcrest.MatcherAssert.assertThat;
2225
import static org.hamcrest.Matchers.equalTo;
26+
import static org.junit.jupiter.api.Assertions.assertEquals;
2327
import static org.junit.jupiter.api.Assertions.assertFalse;
2428
import static org.junit.jupiter.api.Assertions.assertTrue;
2529

@@ -101,6 +105,69 @@ void testTrimWhenCapacityReduced() {
101105
assertFalse(cache.containsKey("2"));
102106
}
103107

108+
@Test
109+
void testComputeIfAbsentBasic() {
110+
LRU2Cache<String, String> cache = new LRU2Cache<>(10);
111+
// First call: goes to preCache, function is called
112+
String result1 = cache.computeIfAbsent("key", k -> k.toUpperCase());
113+
assertEquals("KEY", result1);
114+
115+
// Second call: promotes from preCache to main cache, function is called again (LRU-2 semantics)
116+
String result2 = cache.computeIfAbsent("key", k -> k.toUpperCase());
117+
assertEquals("KEY", result2);
118+
119+
// Third call: found in main cache, function should NOT be called
120+
AtomicInteger callCount = new AtomicInteger(0);
121+
String result3 = cache.computeIfAbsent("key", k -> {
122+
callCount.incrementAndGet();
123+
return k.toUpperCase();
124+
});
125+
assertEquals("KEY", result3);
126+
assertEquals(0, callCount.get(), "Function should not be called when value is in main cache");
127+
}
128+
129+
@Test
130+
void testComputeIfAbsentConcurrency() throws InterruptedException {
131+
LRU2Cache<String, String> cache = new LRU2Cache<>(1000);
132+
int threadCount = 10;
133+
int iterations = 1000;
134+
135+
// Pre-warm: put keys twice to promote them into main cache
136+
for (int i = 0; i < 100; i++) {
137+
String key = "key-" + i;
138+
cache.put(key, key.toUpperCase());
139+
cache.put(key, key.toUpperCase());
140+
}
141+
142+
CountDownLatch startLatch = new CountDownLatch(1);
143+
CountDownLatch doneLatch = new CountDownLatch(threadCount);
144+
AtomicInteger errors = new AtomicInteger(0);
145+
146+
for (int t = 0; t < threadCount; t++) {
147+
new Thread(() -> {
148+
try {
149+
startLatch.await();
150+
for (int i = 0; i < iterations; i++) {
151+
String key = "key-" + (i % 100);
152+
String value = cache.computeIfAbsent(key, k -> k.toUpperCase());
153+
if (value == null || !value.equals(key.toUpperCase())) {
154+
errors.incrementAndGet();
155+
}
156+
}
157+
} catch (Exception e) {
158+
errors.incrementAndGet();
159+
} finally {
160+
doneLatch.countDown();
161+
}
162+
})
163+
.start();
164+
}
165+
166+
startLatch.countDown();
167+
doneLatch.await();
168+
assertEquals(0, errors.get(), "Concurrent computeIfAbsent should not produce errors");
169+
}
170+
104171
@Test
105172
void testPreCacheTrimWhenCapacityReduced() {
106173
LRU2Cache<String, Integer> cache = new LRU2Cache<>(5);

0 commit comments

Comments
 (0)