Skip to content

Commit 8980fca

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 d510872 commit 8980fca

File tree

4 files changed

+206
-14
lines changed

4 files changed

+206
-14
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: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2180,12 +2180,7 @@ 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
}
@@ -2201,7 +2196,22 @@ V waitForLoadingValue(ReferenceEntry<K, V> e, K key, ValueReference<K, V> valueR
22012196
throw new AssertionError();
22022197
}
22032198

2204-
checkState(!Thread.holdsLock(e), "Recursive load of: %s", key);
2199+
// As of this writing, the only prod ValueReference implementation for which isLoading() is
2200+
// true is LoadingValueReference. (Note, however, that not all LoadingValueReference instances
2201+
// have isLoading()==true: LoadingValueReference has a subclass, ComputingValueReference, for
2202+
// which isLoading() is false!) However, that might change, and we already have a *test*
2203+
// implementation for which it doesn't hold. So we check instanceof to be safe.
2204+
if (valueReference instanceof LoadingValueReference) {
2205+
// We check whether the thread that is loading the entry is our current thread, which would
2206+
// mean that we are both loading and waiting for the entry. In this case, we fail fast
2207+
// instead of deadlocking.
2208+
checkState(
2209+
((LoadingValueReference<K, V>) valueReference).getLoadingThread()
2210+
!= Thread.currentThread(),
2211+
"Recursive load of: %s",
2212+
key);
2213+
}
2214+
22052215
// don't consider expiration as we're concurrent with loading
22062216
try {
22072217
V value = valueReference.waitForValue();
@@ -3427,6 +3437,8 @@ static class LoadingValueReference<K, V> implements ValueReference<K, V> {
34273437
final SettableFuture<V> futureValue = SettableFuture.create();
34283438
final Stopwatch stopwatch = Stopwatch.createUnstarted();
34293439

3440+
final Thread loadingThread;
3441+
34303442
public LoadingValueReference() {
34313443
this(LocalCache.<K, V>unset());
34323444
}
@@ -3438,6 +3450,7 @@ public LoadingValueReference() {
34383450
*/
34393451
public LoadingValueReference(ValueReference<K, V> oldValue) {
34403452
this.oldValue = oldValue;
3453+
this.loadingThread = Thread.currentThread();
34413454
}
34423455

34433456
@Override
@@ -3541,6 +3554,10 @@ public ValueReference<K, V> copyFor(
35413554
ReferenceQueue<V> queue, @CheckForNull V value, ReferenceEntry<K, V> entry) {
35423555
return this;
35433556
}
3557+
3558+
Thread getLoadingThread() {
3559+
return this.loadingThread;
3560+
}
35443561
}
35453562

35463563
// 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: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2184,12 +2184,7 @@ 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
}
@@ -2205,7 +2200,22 @@ V waitForLoadingValue(ReferenceEntry<K, V> e, K key, ValueReference<K, V> valueR
22052200
throw new AssertionError();
22062201
}
22072202

2208-
checkState(!Thread.holdsLock(e), "Recursive load of: %s", key);
2203+
// As of this writing, the only prod ValueReference implementation for which isLoading() is
2204+
// true is LoadingValueReference. (Note, however, that not all LoadingValueReference instances
2205+
// have isLoading()==true: LoadingValueReference has a subclass, ComputingValueReference, for
2206+
// which isLoading() is false!) However, that might change, and we already have a *test*
2207+
// implementation for which it doesn't hold. So we check instanceof to be safe.
2208+
if (valueReference instanceof LoadingValueReference) {
2209+
// We check whether the thread that is loading the entry is our current thread, which would
2210+
// mean that we are both loading and waiting for the entry. In this case, we fail fast
2211+
// instead of deadlocking.
2212+
checkState(
2213+
((LoadingValueReference<K, V>) valueReference).getLoadingThread()
2214+
!= Thread.currentThread(),
2215+
"Recursive load of: %s",
2216+
key);
2217+
}
2218+
22092219
// don't consider expiration as we're concurrent with loading
22102220
try {
22112221
V value = valueReference.waitForValue();
@@ -3517,12 +3527,15 @@ static class LoadingValueReference<K, V> implements ValueReference<K, V> {
35173527
final SettableFuture<V> futureValue = SettableFuture.create();
35183528
final Stopwatch stopwatch = Stopwatch.createUnstarted();
35193529

3530+
final Thread loadingThread;
3531+
35203532
public LoadingValueReference() {
35213533
this(null);
35223534
}
35233535

35243536
public LoadingValueReference(@CheckForNull ValueReference<K, V> oldValue) {
35253537
this.oldValue = (oldValue == null) ? LocalCache.unset() : oldValue;
3538+
this.loadingThread = Thread.currentThread();
35263539
}
35273540

35283541
@Override
@@ -3647,6 +3660,10 @@ public ValueReference<K, V> copyFor(
36473660
ReferenceQueue<V> queue, @CheckForNull V value, ReferenceEntry<K, V> entry) {
36483661
return this;
36493662
}
3663+
3664+
Thread getLoadingThread() {
3665+
return this.loadingThread;
3666+
}
36503667
}
36513668

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

0 commit comments

Comments
 (0)