Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: make LocalCache not use synchronized to detect recursive loads (#6845) #6851

Closed
wants to merge 3 commits into from
Closed

refactor: make LocalCache not use synchronized to detect recursive loads (#6845) #6851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cb045df
test: recursive load fail-fast (#6845)
Nov 29, 2023
e80103f
test: recursive load with proxy (#6845)
Nov 29, 2023
498baf1
refactor: thread reference in LoadingValueReference to detect recursi…
Nov 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
70 changes: 70 additions & 0 deletions guava-tests/test/com/google/common/cache/LocalCacheTest.java
View file
Open in desktop
Expand Up @@ -58,6 +58,7 @@
import com.google.common.testing.NullPointerTester;
import com.google.common.testing.SerializableTester;
import com.google.common.testing.TestLogHandler;
import com.google.common.util.concurrent.UncheckedExecutionException;
import java.io.Serializable;
import java.lang.ref.Reference;
import java.lang.ref.ReferenceQueue;
Expand Down Expand Up @@ -2688,8 +2689,77 @@ public void testSerializationProxyManual() {
assertEquals(localCacheTwo.ticker, localCacheThree.ticker);
}

public void testLoadDifferentKeyInLoader() throws ExecutionException, InterruptedException {
LocalCache<String, String> cache = makeLocalCache(createCacheBuilder());
String key1 = "key1";
String key2 = "key2";

assertEquals(key2, cache.get(key1, new CacheLoader<String, String>() {
@Override
public String load(String key) throws Exception {
return cache.get(key2, identityLoader()); // loads a different key, should work
}
}));


}

public void testRecursiveLoad() throws InterruptedException {
LocalCache<String, String> cache = makeLocalCache(createCacheBuilder());
String key = "key";
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
}
};
testLoadThrows(key, cache, loader);
}

public void testRecursiveLoadWithProxy() throws InterruptedException
{
String key = "key";
String otherKey = "otherKey";
LocalCache<String, String> cache = makeLocalCache(createCacheBuilder());
CacheLoader<String, String> loader = new CacheLoader<String, String>() {
@Override
public String load(String key) throws Exception {
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); // loads another key, is ok
}
};
testLoadThrows(key, cache, proxyLoader);
}

// utility methods

private void testLoadThrows(String key, LocalCache<String, String> cache, CacheLoader<String,String> loader) throws InterruptedException
{
CountDownLatch doneSignal = new CountDownLatch(1);
Thread thread = new Thread(() -> {
try {
cache.get(key, loader);
} catch(UncheckedExecutionException | ExecutionException e) {
doneSignal.countDown();
}
});
thread.start();

boolean done = doneSignal.await(1, TimeUnit.SECONDS);
if (!done) {
StringBuilder builder = new StringBuilder();
for (StackTraceElement trace : thread.getStackTrace()) {
builder.append("\tat ").append(trace).append('\n');
}
fail(builder.toString());
}
}

/**
* Returns an iterable containing all combinations of maximumSize, expireAfterAccess/Write,
* weakKeys and weak/softValues.
Expand Down
22 changes: 15 additions & 7 deletions guava/src/com/google/common/cache/LocalCache.java
View file
Open in desktop
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) {
Copy link
Member

@cpovirk cpovirk Dec 1, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this together!

Would it make sense to check the valueReference instead of e.getValueReference()? I ask because I discovered during testing that the current code has a race: It checks e.getValueReference() instanceof LoadingValueReference, and then it casts e.getValueReference() to LoadingValueReference, but the first e.getValueReference() call might return a loading reference and the second a completed reference.

One way to fix that is to call e.getValueReference() only once (e.g., checkRecursiveLoad(key, e.getValueReference());). But it would seem easier and less fragile to use valueReference if that's correct.

The worst thing that I can think of about using valueReference is that we might check the loading thread unnecessarily (in the case that the reference completed between our earlier isLoading() check and this one). But that doesn't seem like a significant worry.

It actually looks like we could change waitForLoadingValue to require a LoadingValueReference instead of a plain ValueReference: The code already checks this above, so we could just perform a cast before the call instead of a conditional cast here.

I have pretty well talked myself into trying this :) I'll report back the results. But please do speak up if I'm missing anything!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concretely: https://github.com/google/guava/pull/6857/files#diff-98cf5b8d5ed981e94f4c745137a9b4ac3d0e896d0471cfb4e3baf85928c5053d

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No your right I think (and the race could have definitely become a problem, good catch!). Just getting the ValueReference directly and not from the entry also seems much more straightforward in general, I think I must have overlooked that it's already present as a method parameter.

I am a bit worried about changing the method signature to LoadingValueReference though. Although I don't see it creating any immediate problems in the code, it feels weird that we are inferring the "is loading" information both from the type and by calling the method. Thus far I think the code relies on calling the method, not the type (this test p.e sets the loading property without being of type LoadingValueReference, and when overlooking the code I didn't see any instanceof / typecasts for LoadingValueReference but quite a lot of calls to isLoading()). I'm not sure if it's a good idea to introduce two ways of checking for this property.

Of course we at least need to check the type before retrieving the loading thread, but we could do it so that if the value isLoading() but is not of type LoadingValueReference we simply omit the recursion check (since we don't have a choice) but otherwise continue as usual, without throwing a ClassCastException.

I don't generally like that we rely on isLoading() instead of the type, but since we seem to be doing it everywhere else I think we should not make an exception here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good catch! I had convinced myself that LoadingValueReference was the only type for which isLoading() was true, but I had neglected to look at the tests. I agree that the current setup is a little weird and also that it's not worth fighting against. I'll update my version and retest.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I suppose that another option is to pull getLoadingThread() up into ValueReference and have it return null in the non-loading case. But it's probably safest to keep a clear delineation between the prod LoadingValueReference, which has the invariants that isLoading() is always true and getLoadingThread() is always non-null, and whatever it is that the test implementation does, which at present does not have such invariants :))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, back to your approach, just using valueReference instead, in https://github.com/google/guava/pull/6857/files#diff-98cf5b8d5ed981e94f4c745137a9b4ac3d0e896d0471cfb4e3baf85928c5053d

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All right, let me know how when you find out anything new 👍.

// 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