From d081eecbbb3d8c8781ef0835ea2331bb82a13b40 Mon Sep 17 00:00:00 2001 From: Sam Berlin Date: Fri, 14 Apr 2023 13:45:59 -0700 Subject: [PATCH] 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 --- .../inject/internal/CycleDetectingLock.java | 27 ++++++++++---- .../internal/CycleDetectingLockTest.java | 37 +++++++++++++++++++ 2 files changed, 56 insertions(+), 8 deletions(-) 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..2c18000002 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 (Future result : results) { + result.get(); + } + service.shutdown(); + } }