Roll back [the change that made LocalCache avoid synchronized](15…

…12730). The `LocalCache` change led to [false reports of recursion during refresh](#6851 (comment)). 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](#6851 (comment)), but we'll need to see if the contributor can sign a CLA. This rollback un-fixes #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](#6851 (comment)). PiperOrigin-RevId: 605069776
google · Feb 7, 2024 · 0e1aebf · 0e1aebf
1 parent 476faf9
commit 0e1aebf
Show file tree

Hide file tree

Showing 2 changed files with 14 additions and 48 deletions.
diff --git 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<? super K, V> 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<K, V> e, K key, ValueReference<K, V> 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<K, V>) 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<K, V> implements ValueReference<K, V> {
     final SettableFuture<V> futureValue = SettableFuture.create();
     final Stopwatch stopwatch = Stopwatch.createUnstarted();
 
-    final Thread loadingThread;
-
     public LoadingValueReference() {
       this(LocalCache.<K, V>unset());
     }
@@ -3450,7 +3438,6 @@ public LoadingValueReference() {
      */
     public LoadingValueReference(ValueReference<K, V> oldValue) {
       this.oldValue = oldValue;
-      this.loadingThread = Thread.currentThread();
     }
 
     @Override
@@ -3554,10 +3541,6 @@ public ValueReference<K, V> copyFor(
         ReferenceQueue<V> queue, @CheckForNull V value, ReferenceEntry<K, V> 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
@@ -2184,7 +2184,12 @@ V lockedGetOrLoad(K key, int hash, CacheLoader<? super K, V> 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<K, V> e, K key, ValueReference<K, V> 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<K, V>) 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<K, V> implements ValueReference<K, V> {
     final SettableFuture<V> futureValue = SettableFuture.create();
     final Stopwatch stopwatch = Stopwatch.createUnstarted();
 
-    final Thread loadingThread;
-
     public LoadingValueReference() {
       this(null);
     }
 
     public LoadingValueReference(@CheckForNull ValueReference<K, V> oldValue) {
       this.oldValue = (oldValue == null) ? LocalCache.unset() : oldValue;
-      this.loadingThread = Thread.currentThread();
     }
 
     @Override
@@ -3660,10 +3647,6 @@ public ValueReference<K, V> copyFor(
         ReferenceQueue<V> queue, @CheckForNull V value, ReferenceEntry<K, V> entry) {
       return this;
     }
-
-    Thread getLoadingThread() {
-      return this.loadingThread;
-    }
   }
 
   static class ComputingValueReference<K, V> extends LoadingValueReference<K, V> {