From 9ab9fefc9199819f7adb305a3d31fd690202f3fa Mon Sep 17 00:00:00 2001
From: Janak Ramakrishnan <janakr@google.com>
Date: Fri, 26 Aug 2016 19:54:35 +0000
Subject: Fix resource leakage on interrupt in ResourceManager identified by
 philwo.

On interrupt, we never released any resources that we had asked to acquire, even though those resources would eventually be acquired.

--
MOS_MIGRATED_REVID=131431321
---
 .../build/lib/actions/ResourceManager.java         | 26 +++++++++---
 .../build/lib/actions/ResourceManagerTest.java     | 48 ++++++++++++++++++++++
 2 files changed, 69 insertions(+), 5 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/actions/ResourceManager.java b/src/main/java/com/google/devtools/build/lib/actions/ResourceManager.java
index 216f393c65..c22a1c097f 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ResourceManager.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ResourceManager.java
@@ -23,7 +23,6 @@ import com.google.devtools.build.lib.profiler.AutoProfiler;
 import com.google.devtools.build.lib.profiler.ProfilerTask;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.Preconditions;
-
 import java.util.Iterator;
 import java.util.LinkedList;
 import java.util.List;
@@ -211,6 +210,19 @@ public class ResourceManager {
       if (latch != null) {
         latch.await();
       }
+    } catch (InterruptedException e) {
+      // Synchronize on this to avoid any racing with #processWaitingThreads
+      synchronized (this) {
+        if (latch.getCount() == 0) {
+          // Resources already acquired by other side. Release them, but not inside this
+          // synchronized block to avoid deadlock.
+          release(resources);
+        } else {
+          // Inform other side that resources shouldn't be acquired.
+          latch.countDown();
+        }
+      }
+      throw e;
     } finally {
       threadLocked.set(resources.getCpuUsage() != 0 || resources.getMemoryMb() != 0
           || resources.getIoUsage() != 0 || resources.getLocalTestCount() != 0);
@@ -359,9 +371,14 @@ public class ResourceManager {
     Iterator<Pair<ResourceSet, CountDownLatch>> iterator = requestList.iterator();
     while (iterator.hasNext()) {
       Pair<ResourceSet, CountDownLatch> request = iterator.next();
-      if (areResourcesAvailable(request.first)) {
-        incrementResources(request.first);
-        request.second.countDown();
+      if (request.second.getCount() != 0) {
+        if (areResourcesAvailable(request.first)) {
+          incrementResources(request.first);
+          request.second.countDown();
+          iterator.remove();
+        }
+      } else {
+        // Cancelled by other side.
         iterator.remove();
       }
     }
@@ -405,7 +422,6 @@ public class ResourceManager {
     return cpuIsAvailable && ramIsAvailable && ioIsAvailable && localTestCountIsAvailable;
   }
 
-
   @VisibleForTesting
   synchronized int getWaitCount() {
     return requestList.size();
diff --git a/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java b/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java
index fa41f083d5..2c4177994f 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java
@@ -18,11 +18,13 @@ import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 import com.google.common.collect.ImmutableSet;
 import com.google.common.eventbus.EventBus;
 import com.google.devtools.build.lib.actions.ResourceManager.ResourceHandle;
 import com.google.devtools.build.lib.testutil.TestThread;
+import com.google.devtools.build.lib.testutil.TestUtils;
 import java.util.concurrent.CyclicBarrier;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
@@ -273,6 +275,52 @@ public class ResourceManagerTest {
     assertFalse(rm.inUse());
   }
 
+  @Test
+  public void testInterruptedAcquisitionClearsResources() throws Exception {
+    assertFalse(rm.inUse());
+    // Acquire a small amount of resources so that future requests can block (the initial request
+    // always succeeds even if it's for too much).
+    TestThread smallThread =
+        new TestThread() {
+          @Override
+          public void runTest() throws InterruptedException {
+            acquire(1, 0, 0, 0);
+          }
+        };
+    smallThread.start();
+    smallThread.joinAndAssertState(TestUtils.WAIT_TIMEOUT_MILLISECONDS);
+    TestThread thread1 =
+        new TestThread() {
+          @Override
+          public void runTest() {
+            Thread.currentThread().interrupt();
+            try {
+              acquire(1999, 0, 0, 0);
+              fail("Didn't throw interrupted exception");
+            } catch (InterruptedException e) {
+              // Expected.
+            }
+          }
+        };
+    thread1.start();
+    thread1.joinAndAssertState(TestUtils.WAIT_TIMEOUT_MILLISECONDS);
+    // This should process the queue. If the request from above is still present, it will take all
+    // the available memory. But it shouldn't.
+    rm.setAvailableResources(
+        ResourceSet.create(
+            /*memoryMb=*/ 2000.0, /*cpuUsage=*/ 1.0, /*ioUsage=*/ 1.0, /*testCount=*/ 2));
+    TestThread thread2 =
+        new TestThread() {
+          @Override
+          public void runTest() throws InterruptedException {
+            acquire(1999, 0, 0, 0);
+            release(1999, 0, 0, 0);
+          }
+        };
+    thread2.start();
+    thread2.joinAndAssertState(TestUtils.WAIT_TIMEOUT_MILLISECONDS);
+  }
+
   @Test
   public void testOutOfOrderAllocation() throws Exception {
     assertFalse(rm.inUse());
-- 
cgit v1.2.3