From cb045df691e4eb28d5baa8905beb24ddb5e076dd Mon Sep 17 00:00:00 2001 From: Christian Ortlepp Date: Wed, 29 Nov 2023 16:49:52 +0100 Subject: [PATCH 1/3] test: recursive load fail-fast (#6845) --- .../google/common/cache/LocalCacheTest.java | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/guava-tests/test/com/google/common/cache/LocalCacheTest.java b/guava-tests/test/com/google/common/cache/LocalCacheTest.java index 7cc67e840dc2..e0be1e24684e 100644 --- a/guava-tests/test/com/google/common/cache/LocalCacheTest.java +++ b/guava-tests/test/com/google/common/cache/LocalCacheTest.java @@ -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; @@ -2688,6 +2689,44 @@ public void testSerializationProxyManual() { assertEquals(localCacheTwo.ticker, localCacheThree.ticker); } + public void testRecursiveLoad() throws ExecutionException, InterruptedException { + LocalCache cache = makeLocalCache(createCacheBuilder()); + String key1 = "key1"; + String key2 = "key2"; + String key3 = "key3"; + + assertEquals(key2, cache.get(key1, new CacheLoader() { + @Override + public String load(String key) throws Exception { + return cache.get(key2, identityLoader()); // loads a different key, should work + } + })); + + CountDownLatch doneSignal = new CountDownLatch(1); + Thread thread = new Thread(() -> { + try { + cache.get(key3, new CacheLoader() { + @Override + public String load(String key) throws Exception { + return cache.get(key3, identityLoader()); // recursive load, this should fail + } + }); + } 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()); + } + } + // utility methods /** From e80103fba25917b60f6d7add66f2d0dc1261e938 Mon Sep 17 00:00:00 2001 From: Christian Ortlepp Date: Wed, 29 Nov 2023 16:49:52 +0100 Subject: [PATCH 2/3] test: recursive load with proxy (#6845) --- .../google/common/cache/LocalCacheTest.java | 51 +++++++++++++++---- 1 file changed, 41 insertions(+), 10 deletions(-) diff --git a/guava-tests/test/com/google/common/cache/LocalCacheTest.java b/guava-tests/test/com/google/common/cache/LocalCacheTest.java index e0be1e24684e..8eff69804227 100644 --- a/guava-tests/test/com/google/common/cache/LocalCacheTest.java +++ b/guava-tests/test/com/google/common/cache/LocalCacheTest.java @@ -2689,11 +2689,10 @@ public void testSerializationProxyManual() { assertEquals(localCacheTwo.ticker, localCacheThree.ticker); } - public void testRecursiveLoad() throws ExecutionException, InterruptedException { + public void testLoadDifferentKeyInLoader() throws ExecutionException, InterruptedException { LocalCache cache = makeLocalCache(createCacheBuilder()); String key1 = "key1"; String key2 = "key2"; - String key3 = "key3"; assertEquals(key2, cache.get(key1, new CacheLoader() { @Override @@ -2702,15 +2701,49 @@ public String load(String key) throws Exception { } })); + + } + + public void testRecursiveLoad() throws InterruptedException { + LocalCache cache = makeLocalCache(createCacheBuilder()); + String key = "key"; + CacheLoader loader = new CacheLoader() { + @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 cache = makeLocalCache(createCacheBuilder()); + CacheLoader loader = new CacheLoader() { + @Override + public String load(String key) throws Exception { + return cache.get(key, identityLoader()); // recursive load, this should fail + } + }; + CacheLoader proxyLoader = new CacheLoader() { + @Override + public String load(String key) throws Exception { + return cache.get(otherKey, loader); // recursive load, this should fail + } + }; + testLoadThrows(key, cache, proxyLoader); + } + + // utility methods + + private void testLoadThrows(String key, LocalCache cache, CacheLoader loader) throws InterruptedException + { CountDownLatch doneSignal = new CountDownLatch(1); Thread thread = new Thread(() -> { try { - cache.get(key3, new CacheLoader() { - @Override - public String load(String key) throws Exception { - return cache.get(key3, identityLoader()); // recursive load, this should fail - } - }); + cache.get(key, loader); } catch(UncheckedExecutionException | ExecutionException e) { doneSignal.countDown(); } @@ -2727,8 +2760,6 @@ public String load(String key) throws Exception { } } - // utility methods - /** * Returns an iterable containing all combinations of maximumSize, expireAfterAccess/Write, * weakKeys and weak/softValues. From 498baf1e585c361968ef0f3e2b25a77ba2a2345d Mon Sep 17 00:00:00 2001 From: Christian Ortlepp Date: Wed, 29 Nov 2023 16:49:52 +0100 Subject: [PATCH 3/3] refactor: thread reference in LoadingValueReference to detect recursion (#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 --- .../google/common/cache/LocalCacheTest.java | 4 ++-- .../com/google/common/cache/LocalCache.java | 22 +++++++++++++------ 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/guava-tests/test/com/google/common/cache/LocalCacheTest.java b/guava-tests/test/com/google/common/cache/LocalCacheTest.java index 8eff69804227..e7ab4df0dbd0 100644 --- a/guava-tests/test/com/google/common/cache/LocalCacheTest.java +++ b/guava-tests/test/com/google/common/cache/LocalCacheTest.java @@ -2724,13 +2724,13 @@ public void testRecursiveLoadWithProxy() throws InterruptedException CacheLoader loader = new CacheLoader() { @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 proxyLoader = new CacheLoader() { @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); diff --git a/guava/src/com/google/common/cache/LocalCache.java b/guava/src/com/google/common/cache/LocalCache.java index a38543dbb7b6..db3e72605686 100644 --- a/guava/src/com/google/common/cache/LocalCache.java +++ b/guava/src/com/google/common/cache/LocalCache.java @@ -2184,12 +2184,7 @@ V lockedGetOrLoad(K key, int hash, CacheLoader 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); } @@ -2205,7 +2200,14 @@ V waitForLoadingValue(ReferenceEntry e, K key, ValueReference 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 le = (LoadingValueReference)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(); @@ -3517,12 +3519,15 @@ static class LoadingValueReference implements ValueReference { final SettableFuture futureValue = SettableFuture.create(); final Stopwatch stopwatch = Stopwatch.createUnstarted(); + final Thread loader; + public LoadingValueReference() { this(null); } public LoadingValueReference(@CheckForNull ValueReference oldValue) { this.oldValue = (oldValue == null) ? LocalCache.unset() : oldValue; + this.loader = Thread.currentThread(); } @Override @@ -3647,6 +3652,9 @@ public ValueReference copyFor( ReferenceQueue queue, @CheckForNull V value, ReferenceEntry entry) { return this; } + Thread getLoader() { + return this.loader; + } } static class ComputingValueReference extends LoadingValueReference {