From 141babaa8df2079be83c8d2352864d999808a6db Mon Sep 17 00:00:00 2001 From: cpovirk Date: Wed, 7 Feb 2024 11:37:14 -0800 Subject: [PATCH] Roll back [the change that made `LocalCache` avoid `synchronized`](https://github.com/google/guava/commit/1512730820a99e329a475b7143b7295775ef26ba). The `LocalCache` change led to [false reports of recursion during refresh](https://github.com/google/guava/pull/6851#issuecomment-1931276822). This CL keeps the new tests from the `LocalCache` change, which already would have passed before the CL. Ideally we would additionally include [tests that demonstrate the refresh problem](https://github.com/google/guava/pull/6851#issuecomment-1931276822), but we'll need to see if the contributor can sign a CLA. This rollback un-fixes https://github.com/google/guava/issues/6845. Users of `common.cache` and virtual threads will likely have to wait until `synchronized` no longer pins threads. (And at that point, if not earlier, they should migrate to [Caffeine](https://github.com/ben-manes/caffeine) :)) RELNOTES=`cache`: Fixed a bug that could cause [false "recursive load" reports during refresh](https://github.com/google/guava/pull/6851#issuecomment-1931276822). PiperOrigin-RevId: 605049501 --- .../com/google/common/cache/LocalCache.java | 31 +++++-------------- .../com/google/common/cache/LocalCache.java | 31 +++++-------------- 2 files changed, 14 insertions(+), 48 deletions(-) diff --git a/android/guava/src/com/google/common/cache/LocalCache.java b/android/guava/src/com/google/common/cache/LocalCache.java index 6fb3945df7ee..f0a7699e0700 100644 --- a/android/guava/src/com/google/common/cache/LocalCache.java +++ b/android/guava/src/com/google/common/cache/LocalCache.java @@ -2180,7 +2180,12 @@ V lockedGetOrLoad(K key, int hash, CacheLoader loader) throws Exec if (createNewEntry) { try { - return loadSync(key, hash, loadingValueReference, loader); + // 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); + } } finally { statsCounter.recordMisses(1); } @@ -2196,22 +2201,7 @@ V waitForLoadingValue(ReferenceEntry e, K key, ValueReference valueR throw new AssertionError(); } - // As of this writing, the only prod ValueReference implementation for which isLoading() is - // true is LoadingValueReference. (Note, however, that not all LoadingValueReference instances - // have isLoading()==true: LoadingValueReference has a subclass, ComputingValueReference, for - // which isLoading() is false!) However, that might change, and we already have a *test* - // implementation for which it doesn't hold. So we check instanceof to be safe. - if (valueReference instanceof LoadingValueReference) { - // We check whether the thread that is loading the entry is our current thread, which would - // mean that we are both loading and waiting for the entry. In this case, we fail fast - // instead of deadlocking. - checkState( - ((LoadingValueReference) valueReference).getLoadingThread() - != Thread.currentThread(), - "Recursive load of: %s", - key); - } - + checkState(!Thread.holdsLock(e), "Recursive load of: %s", key); // don't consider expiration as we're concurrent with loading try { V value = valueReference.waitForValue(); @@ -3437,8 +3427,6 @@ static class LoadingValueReference implements ValueReference { final SettableFuture futureValue = SettableFuture.create(); final Stopwatch stopwatch = Stopwatch.createUnstarted(); - final Thread loadingThread; - public LoadingValueReference() { this(LocalCache.unset()); } @@ -3450,7 +3438,6 @@ public LoadingValueReference() { */ public LoadingValueReference(ValueReference oldValue) { this.oldValue = oldValue; - this.loadingThread = Thread.currentThread(); } @Override @@ -3554,10 +3541,6 @@ public ValueReference copyFor( ReferenceQueue queue, @CheckForNull V value, ReferenceEntry entry) { return this; } - - Thread getLoadingThread() { - return this.loadingThread; - } } // Queues diff --git a/guava/src/com/google/common/cache/LocalCache.java b/guava/src/com/google/common/cache/LocalCache.java index dd3590357ed0..a38543dbb7b6 100644 --- a/guava/src/com/google/common/cache/LocalCache.java +++ b/guava/src/com/google/common/cache/LocalCache.java @@ -2184,7 +2184,12 @@ V lockedGetOrLoad(K key, int hash, CacheLoader loader) throws Exec if (createNewEntry) { try { - return loadSync(key, hash, loadingValueReference, loader); + // 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); + } } finally { statsCounter.recordMisses(1); } @@ -2200,22 +2205,7 @@ V waitForLoadingValue(ReferenceEntry e, K key, ValueReference valueR throw new AssertionError(); } - // As of this writing, the only prod ValueReference implementation for which isLoading() is - // true is LoadingValueReference. (Note, however, that not all LoadingValueReference instances - // have isLoading()==true: LoadingValueReference has a subclass, ComputingValueReference, for - // which isLoading() is false!) However, that might change, and we already have a *test* - // implementation for which it doesn't hold. So we check instanceof to be safe. - if (valueReference instanceof LoadingValueReference) { - // We check whether the thread that is loading the entry is our current thread, which would - // mean that we are both loading and waiting for the entry. In this case, we fail fast - // instead of deadlocking. - checkState( - ((LoadingValueReference) valueReference).getLoadingThread() - != Thread.currentThread(), - "Recursive load of: %s", - key); - } - + checkState(!Thread.holdsLock(e), "Recursive load of: %s", key); // don't consider expiration as we're concurrent with loading try { V value = valueReference.waitForValue(); @@ -3527,15 +3517,12 @@ static class LoadingValueReference implements ValueReference { final SettableFuture futureValue = SettableFuture.create(); final Stopwatch stopwatch = Stopwatch.createUnstarted(); - final Thread loadingThread; - public LoadingValueReference() { this(null); } public LoadingValueReference(@CheckForNull ValueReference oldValue) { this.oldValue = (oldValue == null) ? LocalCache.unset() : oldValue; - this.loadingThread = Thread.currentThread(); } @Override @@ -3660,10 +3647,6 @@ public ValueReference copyFor( ReferenceQueue queue, @CheckForNull V value, ReferenceEntry entry) { return this; } - - Thread getLoadingThread() { - return this.loadingThread; - } } static class ComputingValueReference extends LoadingValueReference {