Skip to content

Commit abd1304

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 abd1304

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,11 +16,16 @@
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;
28+
import static org.junit.jupiter.api.Assertions.assertNotNull;
2429
import static org.junit.jupiter.api.Assertions.assertTrue;
2530

2631
class LRU2CacheTest {
@@ -101,6 +106,68 @@ void testTrimWhenCapacityReduced() {
101106
assertFalse(cache.containsKey("2"));
102107
}
103108

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