Skip to content

Commit 52d072a

Browse files
Christian OrtleppGoogle Java Core Libraries
Christian Ortlepp
authored and
Google Java Core Libraries
committed
Make LocalCache not use synchronized to detect recursive loads.
Fixes #6851 Fixes #6845 RELNOTES=n/a PiperOrigin-RevId: 586666218
1 parent f4c1264 commit 52d072a

File tree

4 files changed

+196
-26
lines changed

4 files changed

+196
-26
lines changed

‎android/guava-tests/test/com/google/common/cache/LocalCacheTest.java‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@
5858
import com.google.common.testing.NullPointerTester;
5959
import com.google.common.testing.SerializableTester;
6060
import com.google.common.testing.TestLogHandler;
61+
import com.google.common.util.concurrent.UncheckedExecutionException;
6162
import java.io.Serializable;
6263
import java.lang.ref.Reference;
6364
import java.lang.ref.ReferenceQueue;
@@ -2639,8 +2640,86 @@ public void testSerializationProxyManual() {
26392640
assertEquals(localCacheTwo.ticker, localCacheThree.ticker);
26402641
}
26412642

2643+
public void testLoadDifferentKeyInLoader() throws ExecutionException, InterruptedException {
2644+
LocalCache<String, String> cache = makeLocalCache(createCacheBuilder());
2645+
String key1 = "key1";
2646+
String key2 = "key2";
2647+
2648+
assertEquals(
2649+
key2,
2650+
cache.get(
2651+
key1,
2652+
new CacheLoader<String, String>() {
2653+
@Override
2654+
public String load(String key) throws Exception {
2655+
return cache.get(key2, identityLoader()); // loads a different key, should work
2656+
}
2657+
}));
2658+
}
2659+
2660+
public void testRecursiveLoad() throws InterruptedException {
2661+
LocalCache<String, String> cache = makeLocalCache(createCacheBuilder());
2662+
String key = "key";
2663+
CacheLoader<String, String> loader =
2664+
new CacheLoader<String, String>() {
2665+
@Override
2666+
public String load(String key) throws Exception {
2667+
return cache.get(key, identityLoader()); // recursive load, this should fail
2668+
}
2669+
};
2670+
testLoadThrows(key, cache, loader);
2671+
}
2672+
2673+
public void testRecursiveLoadWithProxy() throws InterruptedException {
2674+
String key = "key";
2675+
String otherKey = "otherKey";
2676+
LocalCache<String, String> cache = makeLocalCache(createCacheBuilder());
2677+
CacheLoader<String, String> loader =
2678+
new CacheLoader<String, String>() {
2679+
@Override
2680+
public String load(String key) throws Exception {
2681+
return cache.get(
2682+
key,
2683+
identityLoader()); // recursive load (same as the initial one), this should fail
2684+
}
2685+
};
2686+
CacheLoader<String, String> proxyLoader =
2687+
new CacheLoader<String, String>() {
2688+
@Override
2689+
public String load(String key) throws Exception {
2690+
return cache.get(otherKey, loader); // loads another key, is ok
2691+
}
2692+
};
2693+
testLoadThrows(key, cache, proxyLoader);
2694+
}
2695+
26422696
// utility methods
26432697

2698+
private void testLoadThrows(
2699+
String key, LocalCache<String, String> cache, CacheLoader<String, String> loader)
2700+
throws InterruptedException {
2701+
CountDownLatch doneSignal = new CountDownLatch(1);
2702+
Thread thread =
2703+
new Thread(
2704+
() -> {
2705+
try {
2706+
cache.get(key, loader);
2707+
} catch (UncheckedExecutionException | ExecutionException e) {
2708+
doneSignal.countDown();
2709+
}
2710+
});
2711+
thread.start();
2712+
2713+
boolean done = doneSignal.await(1, TimeUnit.SECONDS);
2714+
if (!done) {
2715+
StringBuilder builder = new StringBuilder();
2716+
for (StackTraceElement trace : thread.getStackTrace()) {
2717+
builder.append("\tat ").append(trace).append('\n');
2718+
}
2719+
fail(builder.toString());
2720+
}
2721+
}
2722+
26442723
/**
26452724
* Returns an iterable containing all combinations of maximumSize, expireAfterAccess/Write,
26462725
* weakKeys and weak/softValues.

‎android/guava/src/com/google/common/cache/LocalCache.java‎

Lines changed: 19 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2068,7 +2068,7 @@ V get(K key, int hash, CacheLoader<? super K, V> loader) throws ExecutionExcepti
20682068
}
20692069
ValueReference<K, V> valueReference = e.getValueReference();
20702070
if (valueReference.isLoading()) {
2071-
return waitForLoadingValue(e, key, valueReference);
2071+
return waitForLoadingValue(e, key, (LoadingValueReference<K, V>) valueReference);
20722072
}
20732073
}
20742074
}
@@ -2180,28 +2180,27 @@ V lockedGetOrLoad(K key, int hash, CacheLoader<? super K, V> loader) throws Exec
21802180

21812181
if (createNewEntry) {
21822182
try {
2183-
// Synchronizes on the entry to allow failing fast when a recursive load is
2184-
// detected. This may be circumvented when an entry is copied, but will fail fast most
2185-
// of the time.
2186-
synchronized (e) {
2187-
return loadSync(key, hash, loadingValueReference, loader);
2188-
}
2183+
return loadSync(key, hash, loadingValueReference, loader);
21892184
} finally {
21902185
statsCounter.recordMisses(1);
21912186
}
21922187
} else {
21932188
// The entry already exists. Wait for loading.
2194-
return waitForLoadingValue(e, key, valueReference);
2189+
return waitForLoadingValue(e, key, (LoadingValueReference<K, V>) valueReference);
21952190
}
21962191
}
21972192

2198-
V waitForLoadingValue(ReferenceEntry<K, V> e, K key, ValueReference<K, V> valueReference)
2193+
V waitForLoadingValue(ReferenceEntry<K, V> e, K key, LoadingValueReference<K, V> valueReference)
21992194
throws ExecutionException {
2200-
if (!valueReference.isLoading()) {
2201-
throw new AssertionError();
2202-
}
22032195

2204-
checkState(!Thread.holdsLock(e), "Recursive load of: %s", key);
2196+
// We check whether the thread that is loading the entry is our current thread, which would
2197+
// mean that we both load and wait for the entry. In this case, we fail fast instead of
2198+
// deadlocking.
2199+
checkState(
2200+
valueReference.getLoadingThread() != Thread.currentThread(),
2201+
"Recursive load of: %s",
2202+
key);
2203+
22052204
// don't consider expiration as we're concurrent with loading
22062205
try {
22072206
V value = valueReference.waitForValue();
@@ -3427,6 +3426,8 @@ static class LoadingValueReference<K, V> implements ValueReference<K, V> {
34273426
final SettableFuture<V> futureValue = SettableFuture.create();
34283427
final Stopwatch stopwatch = Stopwatch.createUnstarted();
34293428

3429+
final Thread loadingThread;
3430+
34303431
public LoadingValueReference() {
34313432
this(LocalCache.<K, V>unset());
34323433
}
@@ -3438,6 +3439,7 @@ public LoadingValueReference() {
34383439
*/
34393440
public LoadingValueReference(ValueReference<K, V> oldValue) {
34403441
this.oldValue = oldValue;
3442+
this.loadingThread = Thread.currentThread();
34413443
}
34423444

34433445
@Override
@@ -3541,6 +3543,10 @@ public ValueReference<K, V> copyFor(
35413543
ReferenceQueue<V> queue, @CheckForNull V value, ReferenceEntry<K, V> entry) {
35423544
return this;
35433545
}
3546+
3547+
Thread getLoadingThread() {
3548+
return this.loadingThread;
3549+
}
35443550
}
35453551

35463552
// Queues

‎guava-tests/test/com/google/common/cache/LocalCacheTest.java‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@
5858
import com.google.common.testing.NullPointerTester;
5959
import com.google.common.testing.SerializableTester;
6060
import com.google.common.testing.TestLogHandler;
61+
import com.google.common.util.concurrent.UncheckedExecutionException;
6162
import java.io.Serializable;
6263
import java.lang.ref.Reference;
6364
import java.lang.ref.ReferenceQueue;
@@ -2688,8 +2689,86 @@ public void testSerializationProxyManual() {
26882689
assertEquals(localCacheTwo.ticker, localCacheThree.ticker);
26892690
}
26902691

2692+
public void testLoadDifferentKeyInLoader() throws ExecutionException, InterruptedException {
2693+
LocalCache<String, String> cache = makeLocalCache(createCacheBuilder());
2694+
String key1 = "key1";
2695+
String key2 = "key2";
2696+
2697+
assertEquals(
2698+
key2,
2699+
cache.get(
2700+
key1,
2701+
new CacheLoader<String, String>() {
2702+
@Override
2703+
public String load(String key) throws Exception {
2704+
return cache.get(key2, identityLoader()); // loads a different key, should work
2705+
}
2706+
}));
2707+
}
2708+
2709+
public void testRecursiveLoad() throws InterruptedException {
2710+
LocalCache<String, String> cache = makeLocalCache(createCacheBuilder());
2711+
String key = "key";
2712+
CacheLoader<String, String> loader =
2713+
new CacheLoader<String, String>() {
2714+
@Override
2715+
public String load(String key) throws Exception {
2716+
return cache.get(key, identityLoader()); // recursive load, this should fail
2717+
}
2718+
};
2719+
testLoadThrows(key, cache, loader);
2720+
}
2721+
2722+
public void testRecursiveLoadWithProxy() throws InterruptedException {
2723+
String key = "key";
2724+
String otherKey = "otherKey";
2725+
LocalCache<String, String> cache = makeLocalCache(createCacheBuilder());
2726+
CacheLoader<String, String> loader =
2727+
new CacheLoader<String, String>() {
2728+
@Override
2729+
public String load(String key) throws Exception {
2730+
return cache.get(
2731+
key,
2732+
identityLoader()); // recursive load (same as the initial one), this should fail
2733+
}
2734+
};
2735+
CacheLoader<String, String> proxyLoader =
2736+
new CacheLoader<String, String>() {
2737+
@Override
2738+
public String load(String key) throws Exception {
2739+
return cache.get(otherKey, loader); // loads another key, is ok
2740+
}
2741+
};
2742+
testLoadThrows(key, cache, proxyLoader);
2743+
}
2744+
26912745
// utility methods
26922746

2747+
private void testLoadThrows(
2748+
String key, LocalCache<String, String> cache, CacheLoader<String, String> loader)
2749+
throws InterruptedException {
2750+
CountDownLatch doneSignal = new CountDownLatch(1);
2751+
Thread thread =
2752+
new Thread(
2753+
() -> {
2754+
try {
2755+
cache.get(key, loader);
2756+
} catch (UncheckedExecutionException | ExecutionException e) {
2757+
doneSignal.countDown();
2758+
}
2759+
});
2760+
thread.start();
2761+
2762+
boolean done = doneSignal.await(1, TimeUnit.SECONDS);
2763+
if (!done) {
2764+
StringBuilder builder = new StringBuilder();
2765+
for (StackTraceElement trace : thread.getStackTrace()) {
2766+
builder.append("\tat ").append(trace).append('\n');
2767+
}
2768+
fail(builder.toString());
2769+
}
2770+
}
2771+
26932772
/**
26942773
* Returns an iterable containing all combinations of maximumSize, expireAfterAccess/Write,
26952774
* weakKeys and weak/softValues.

‎guava/src/com/google/common/cache/LocalCache.java‎

Lines changed: 19 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2072,7 +2072,7 @@ V get(K key, int hash, CacheLoader<? super K, V> loader) throws ExecutionExcepti
20722072
}
20732073
ValueReference<K, V> valueReference = e.getValueReference();
20742074
if (valueReference.isLoading()) {
2075-
return waitForLoadingValue(e, key, valueReference);
2075+
return waitForLoadingValue(e, key, (LoadingValueReference<K, V>) valueReference);
20762076
}
20772077
}
20782078
}
@@ -2184,28 +2184,27 @@ V lockedGetOrLoad(K key, int hash, CacheLoader<? super K, V> loader) throws Exec
21842184

21852185
if (createNewEntry) {
21862186
try {
2187-
// Synchronizes on the entry to allow failing fast when a recursive load is
2188-
// detected. This may be circumvented when an entry is copied, but will fail fast most
2189-
// of the time.
2190-
synchronized (e) {
2191-
return loadSync(key, hash, loadingValueReference, loader);
2192-
}
2187+
return loadSync(key, hash, loadingValueReference, loader);
21932188
} finally {
21942189
statsCounter.recordMisses(1);
21952190
}
21962191
} else {
21972192
// The entry already exists. Wait for loading.
2198-
return waitForLoadingValue(e, key, valueReference);
2193+
return waitForLoadingValue(e, key, (LoadingValueReference<K, V>) valueReference);
21992194
}
22002195
}
22012196

2202-
V waitForLoadingValue(ReferenceEntry<K, V> e, K key, ValueReference<K, V> valueReference)
2197+
V waitForLoadingValue(ReferenceEntry<K, V> e, K key, LoadingValueReference<K, V> valueReference)
22032198
throws ExecutionException {
2204-
if (!valueReference.isLoading()) {
2205-
throw new AssertionError();
2206-
}
22072199

2208-
checkState(!Thread.holdsLock(e), "Recursive load of: %s", key);
2200+
// We check whether the thread that is loading the entry is our current thread, which would
2201+
// mean that we both load and wait for the entry. In this case, we fail fast instead of
2202+
// deadlocking.
2203+
checkState(
2204+
valueReference.getLoadingThread() != Thread.currentThread(),
2205+
"Recursive load of: %s",
2206+
key);
2207+
22092208
// don't consider expiration as we're concurrent with loading
22102209
try {
22112210
V value = valueReference.waitForValue();
@@ -3517,12 +3516,15 @@ static class LoadingValueReference<K, V> implements ValueReference<K, V> {
35173516
final SettableFuture<V> futureValue = SettableFuture.create();
35183517
final Stopwatch stopwatch = Stopwatch.createUnstarted();
35193518

3519+
final Thread loadingThread;
3520+
35203521
public LoadingValueReference() {
35213522
this(null);
35223523
}
35233524

35243525
public LoadingValueReference(@CheckForNull ValueReference<K, V> oldValue) {
35253526
this.oldValue = (oldValue == null) ? LocalCache.unset() : oldValue;
3527+
this.loadingThread = Thread.currentThread();
35263528
}
35273529

35283530
@Override
@@ -3647,6 +3649,10 @@ public ValueReference<K, V> copyFor(
36473649
ReferenceQueue<V> queue, @CheckForNull V value, ReferenceEntry<K, V> entry) {
36483650
return this;
36493651
}
3652+
3653+
Thread getLoadingThread() {
3654+
return this.loadingThread;
3655+
}
36503656
}
36513657

36523658
static class ComputingValueReference<K, V> extends LoadingValueReference<K, V> {

0 commit comments

Comments
 (0)