diff --git a/core/src/com/google/inject/internal/CycleDetectingLock.java b/core/src/com/google/inject/internal/CycleDetectingLock.java index d767015548..bbfa9fdefa 100644 --- a/core/src/com/google/inject/internal/CycleDetectingLock.java +++ b/core/src/com/google/inject/internal/CycleDetectingLock.java @@ -144,14 +144,25 @@ public ListMultimap 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 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 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; + } } } diff --git a/core/test/com/google/inject/internal/CycleDetectingLockTest.java b/core/test/com/google/inject/internal/CycleDetectingLockTest.java index 7c44717c42..e16cff09fe 100644 --- a/core/test/com/google/inject/internal/CycleDetectingLockTest.java +++ b/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 Future> 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> results = new ArrayList<>(); + CycleDetectingLockFactory factory = new CycleDetectingLockFactory<>(); + CycleDetectingLock 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 (var result : results) { + result.get(); + } + service.shutdown(); + } }