Skip to content

Commit

Permalink
refactor: thread reference in LoadingValueReference to detect recursi…
Browse files Browse the repository at this point in the history
…on (#6845)

- replaces the previous synchronized implementation to make it work gracefully with VirtualThreads
- remember the thread that created a LoadingValueReference to later determine whether the "loader" is the same thread as the waiting thread, and we are therefore inside a deadlock
  • Loading branch information
Christian Ortlepp authored and Christian Ortlepp committed Nov 24, 2023
1 parent c80fb95 commit 3171d45
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 9 deletions.
4 changes: 2 additions & 2 deletions guava-tests/test/com/google/common/cache/LocalCacheTest.java
Expand Up @@ -2724,13 +2724,13 @@ public void testRecursiveLoadWithProxy() throws InterruptedException
CacheLoader<String, String> loader = new CacheLoader<String, String>() {
@Override
public String load(String key) throws Exception {
return cache.get(key, identityLoader()); // recursive load, this should fail
return cache.get(key, identityLoader()); // recursive load (same as the initial one), this should fail
}
};
CacheLoader<String, String> proxyLoader = new CacheLoader<String, String>() {
@Override
public String load(String key) throws Exception {
return cache.get(otherKey, loader); // recursive load, this should fail
return cache.get(otherKey, loader); // loads another key, is ok
}
};
testLoadThrows(key, cache, proxyLoader);
Expand Down
22 changes: 15 additions & 7 deletions guava/src/com/google/common/cache/LocalCache.java
Expand Up @@ -2184,12 +2184,7 @@ V lockedGetOrLoad(K key, int hash, CacheLoader<? super K, V> loader) throws Exec

if (createNewEntry) {
try {
// Synchronizes on the entry to allow failing fast when a recursive load is
// detected. This may be circumvented when an entry is copied, but will fail fast most
// of the time.
synchronized (e) {
return loadSync(key, hash, loadingValueReference, loader);
}
return loadSync(key, hash, loadingValueReference, loader);
} finally {
statsCounter.recordMisses(1);
}
Expand All @@ -2205,7 +2200,14 @@ V waitForLoadingValue(ReferenceEntry<K, V> e, K key, ValueReference<K, V> valueR
throw new AssertionError();
}

checkState(!Thread.holdsLock(e), "Recursive load of: %s", key);
if (e.getValueReference() instanceof LoadingValueReference) {
// if the entry is still loading, we check whether the thread that
// is loading the entry is our current thread
// which would mean that we both load and wait for the entry
// in this case we fail fast instead of deadlocking
LoadingValueReference<K,V> le = (LoadingValueReference<K, V>)e.getValueReference();
checkState(le.getLoader() != Thread.currentThread(), "Recursive load of: %s", key);
}
// don't consider expiration as we're concurrent with loading
try {
V value = valueReference.waitForValue();
Expand Down Expand Up @@ -3517,12 +3519,15 @@ static class LoadingValueReference<K, V> implements ValueReference<K, V> {
final SettableFuture<V> futureValue = SettableFuture.create();
final Stopwatch stopwatch = Stopwatch.createUnstarted();

final Thread loader;

public LoadingValueReference() {
this(null);
}

public LoadingValueReference(@CheckForNull ValueReference<K, V> oldValue) {
this.oldValue = (oldValue == null) ? LocalCache.unset() : oldValue;
this.loader = Thread.currentThread();
}

@Override
Expand Down Expand Up @@ -3647,6 +3652,9 @@ public ValueReference<K, V> copyFor(
ReferenceQueue<V> queue, @CheckForNull V value, ReferenceEntry<K, V> entry) {
return this;
}
Thread getLoader() {
return this.loader;
}
}

static class ComputingValueReference<K, V> extends LoadingValueReference<K, V> {
Expand Down

0 comments on commit 3171d45

Please sign in to comment.