Prevent endless loop in lock cycle detection. The following scenario …

…would previously fail:

 * T1: Enter `lockOrDetectPotentialLocksCycle`:
    - Lock CAF.class, add itself to `lockThreadIsWaitingOn`, unlock CAF.class
    - Lock the cycle detecting lock (CDL)
    - Lock CAF.class, mark itself as `lockOwnerThread`, remove itself from `lockThreadIsWaitingOn`
    - Exit `lockOrDetectPotentialLocksCycle`
 * T1: Re-enter `lockOrDetectPotentialLocksCycle`:
    - Lock CAF.class, add itself to `lockThreadIsWaitingOn`, unlock CAF.class
 T2: Enter `lockOrDetectPotentialLocksCycle`
    - Lock CAF.class, invoke `detectPotentialLocksCycle`.

At this point, `detectPotentialLocksCycle` will now loop forever, because the `lockOwnerThread` is also in `lockThreadIsWaitingOn`. During the course of looping forever, it will OOM, because it's building up an in-memory structure of what it thinks are cycles.

The solution is to avoid the re-entrant T1 from adding itself to `lockThreadIsWaitingOn` if it's already the `lockOwnerThread`. It's guaranteed that it won't relinquish the lock concurrently, because it's the exact same thread that owns it.

Fixes #1635 and Fixes #1510

PiperOrigin-RevId: 524376697

core/src/com/google/inject/internal/CycleDetectingLock.java

@@ -144,14 +144,25 @@ public ListMultimap<Thread, ID> lockOrDetectPotentialLocksCycle() {
         final Thread currentThread = Thread.currentThread();
         synchronized (CycleDetectingLockFactory.class) {
           checkState();
-          // Add this lock to the waiting map to ensure it is included in any reported lock cycle.
-          lockThreadIsWaitingOn.put(currentThread, this);
-          ListMultimap<Thread, ID> locksInCycle = detectPotentialLocksCycle();
-          if (!locksInCycle.isEmpty()) {
-            // We aren't actually going to wait for this lock, so remove it from the map.
-            lockThreadIsWaitingOn.remove(currentThread);
-            // potential deadlock is found, we don't try to take this lock
-            return locksInCycle;
+          // Only do work if this thread doesn't already own the lock.
+          // If we're attempting to re-enter our own lock, then we're not going to wait to lock.
+          // Otherwise, if we add ourselves to `lockThreadIsWaitingOn`, another thread attempting to
+          // lock may end up looping forever while detecting cycles (which will OOM).  See
+          // https://github.com/google/guice/issues/1510 &
+          // https://github.com/google/guice/pull/1635.
+          // Note that it's not possible for the owner thread to concurrently relinquish the lock,
+          // because _this_ is the owner thread, and the next line of code this thread will execute
+          // is `lockImplementation.lock()`.
+          if (lockOwnerThread != currentThread) {
+            // Add this lock to the waiting map to ensure it is included in any reported lock cycle.
+            lockThreadIsWaitingOn.put(currentThread, this);
+            ListMultimap<Thread, ID> locksInCycle = detectPotentialLocksCycle();
+            if (!locksInCycle.isEmpty()) {
+              // We aren't actually going to wait for this lock, so remove it from the map.
+              lockThreadIsWaitingOn.remove(currentThread);
+              // potential deadlock is found, we don't try to take this lock
+              return locksInCycle;
+            }
           }
         }
 

core/test/com/google/inject/internal/CycleDetectingLockTest.java

@@ -1,16 +1,21 @@
 package com.google.inject.internal;
 
+import static com.google.common.truth.Truth.assertThat;
+
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ListMultimap;
 import com.google.common.collect.Multimaps;
 import com.google.inject.internal.CycleDetectingLock.CycleDetectingLockFactory;
 import com.google.inject.internal.CycleDetectingLock.CycleDetectingLockFactory.ReentrantCycleDetectingLock;
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
 import java.util.concurrent.CyclicBarrier;
+import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
 import java.util.concurrent.FutureTask;
+import java.util.concurrent.Phaser;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.locks.ReentrantLock;
 import junit.framework.TestCase;
@@ -230,4 +235,36 @@ private static <T> Future<ListMultimap<Thread, T>> grabLocksInThread(
     thread.start();
     return future;
   }
+
+  // Tests issues https://github.com/google/guice/pull/1635 &
+  // https://github.com/google/guice/issues/1510
+  // These were problems with the implementation that caused the impl to loop forever and OOM.
+  public void testConcurrentReentrance() throws Exception {
+    int numConcurrentLockers = 8;
+    ExecutorService service = Executors.newFixedThreadPool(numConcurrentLockers);
+    List<Future<?>> results = new ArrayList<>();
+    CycleDetectingLockFactory<String> factory = new CycleDetectingLockFactory<>();
+    CycleDetectingLock<String> lock = factory.create("circles");
+    // Use a phaser to increase the chances that the code runs concurrently, so we can hit
+    // the conditions that trigger the failure. (Even so, with the bug, this test had a failure rate
+    // of ~0.4%, so will need to be run many times to guarantee it's fixed.)
+    Phaser phaser = new Phaser(1);
+    for (int i = 0; i < numConcurrentLockers; ++i) {
+      phaser.register();
+      results.add(
+          service.submit(
+              () -> {
+                phaser.arriveAndAwaitAdvance();
+                assertThat(lock.lockOrDetectPotentialLocksCycle()).isEmpty();
+                assertThat(lock.lockOrDetectPotentialLocksCycle()).isEmpty();
+                lock.unlock();
+                lock.unlock();
+              }));
+    }
+    phaser.arriveAndDeregister();
+    for (Future<?> result : results) {
+      result.get();
+    }
+    service.shutdown();
+  }
 }