Some little fixes to ResourceManager.

- Make sure that empty ResourceSets are always == ResourceSet.ZERO and use that for easier comparison.
- No longer allow nested resource acquisition, because it may lead to deadlocks.

--
MOS_MIGRATED_REVID=131567446

3 files changed, 137 insertions, 20 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 c22a1c097f..9d5edc3153 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
@@ -201,7 +201,11 @@ public class ResourceManager {
    */
   public ResourceHandle acquireResources(ActionExecutionMetadata owner, ResourceSet resources)
       throws InterruptedException {
-    Preconditions.checkNotNull(resources);
+    Preconditions.checkNotNull(
+        resources, "acquireResources called with resources == NULL during %s", owner);
+    Preconditions.checkState(
+        !threadHasResources(), "acquireResources with existing resource lock during %s", owner);
+
     AutoProfiler p = profiled(owner, ProfilerTask.ACTION_LOCK);
     CountDownLatch latch = null;
     try {
@@ -224,8 +228,7 @@ public class ResourceManager {
       }
       throw e;
     } finally {
-      threadLocked.set(resources.getCpuUsage() != 0 || resources.getMemoryMb() != 0
-          || resources.getIoUsage() != 0 || resources.getLocalTestCount() != 0);
+      threadLocked.set(resources != ResourceSet.ZERO);
       acquired(owner);
 
       // Profile acquisition only if it waited for resource to become available.
@@ -233,6 +236,7 @@ public class ResourceManager {
         p.complete();
       }
     }
+
     return new ResourceHandle(this, owner, resources);
   }
 
@@ -242,7 +246,13 @@ public class ResourceManager {
    * @return a ResourceHandle iff the given resources were locked (all or nothing), null otherwise.
    */
   public ResourceHandle tryAcquire(ActionExecutionMetadata owner, ResourceSet resources) {
+    Preconditions.checkNotNull(
+        resources, "tryAcquire called with resources == NULL during %s", owner);
+    Preconditions.checkState(
+        !threadHasResources(), "tryAcquire with existing resource lock during %s", owner);
+
     boolean acquired = false;
+
     synchronized (this) {
       if (areResourcesAvailable(resources)) {
         incrementResources(resources);
@@ -251,8 +261,7 @@ public class ResourceManager {
     }
 
     if (acquired) {
-      threadLocked.set(resources.getCpuUsage() != 0 || resources.getMemoryMb() != 0
-          || resources.getIoUsage() != 0 || resources.getLocalTestCount() != 0);
+      threadLocked.set(resources != ResourceSet.ZERO);
       acquired(owner);
       return new ResourceHandle(this, owner, resources);
     }
@@ -313,6 +322,11 @@ public class ResourceManager {
    * <p>NB! This method must be thread-safe!
    */
   public void releaseResources(ActionExecutionMetadata owner, ResourceSet resources) {
+    Preconditions.checkNotNull(
+        resources, "releaseResources called with resources == NULL during %s", owner);
+    Preconditions.checkState(
+        threadHasResources(), "releaseResources without resource lock during %s", owner);
+
     boolean isConflict = false;
     AutoProfiler p = profiled(owner, ProfilerTask.ACTION_RELEASE);
     try {
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ResourceSet.java b/src/main/java/com/google/devtools/build/lib/actions/ResourceSet.java
index d191e3cd53..9668eba2cb 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ResourceSet.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ResourceSet.java
@@ -18,7 +18,6 @@ import com.google.common.base.Splitter;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.common.options.Converter;
 import com.google.devtools.common.options.OptionsParsingException;
-
 import java.util.Iterator;
 import java.util.NoSuchElementException;
 
@@ -63,6 +62,9 @@ public class ResourceSet {
    * local tests.
    */
   public static ResourceSet createWithRamCpuIo(double memoryMb, double cpuUsage, double ioUsage) {
+    if (memoryMb == 0 && cpuUsage == 0 && ioUsage == 0) {
+      return ZERO;
+    }
     return new ResourceSet(memoryMb, cpuUsage, ioUsage, 0);
   }
 
@@ -83,6 +85,9 @@ public class ResourceSet {
    */
   public static ResourceSet create(double memoryMb, double cpuUsage, double ioUsage,
       int localTestCount) {
+    if (memoryMb == 0 && cpuUsage == 0 && ioUsage == 0 && localTestCount == 0) {
+      return ZERO;
+    }
     return new ResourceSet(memoryMb, cpuUsage, ioUsage, localTestCount);
   }
 
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 2c4177994f..d05123e70c 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
@@ -73,7 +73,7 @@ public class ResourceManagerTest {
     rm.releaseResources(resourceOwner, ResourceSet.create(ram, cpu, io, tests));
   }
 
-  private void validate (int count) {
+  private void validate(int count) {
     assertEquals(count, counter.incrementAndGet());
   }
 
@@ -124,7 +124,15 @@ public class ResourceManagerTest {
 
     // When a request for CPU is made that would slightly overallocate CPU,
     // Then the request succeeds:
-    assertThat(acquireNonblocking(0, 0.6, 0, 0)).isNotNull();
+    TestThread thread1 =
+        new TestThread() {
+          @Override
+          public void runTest() throws Exception {
+            assertThat(acquireNonblocking(0, 0.6, 0, 0)).isNotNull();
+          }
+        };
+    thread1.start();
+    thread1.joinAndAssertState(10000);
   }
 
   @Test
@@ -136,20 +144,36 @@ public class ResourceManagerTest {
 
     // When a request for a large CPU allocation is made,
     // Then the request succeeds:
-    assertThat(acquireNonblocking(0, 0.99, 0, 0)).isNotNull();
+    TestThread thread1 =
+        new TestThread() {
+          @Override
+          public void runTest() throws Exception {
+            assertThat(acquireNonblocking(0, 0.99, 0, 0)).isNotNull();
+            // Cleanup
+            release(0, 0.99, 0, 0);
+          }
+        };
+    thread1.start();
+    thread1.joinAndAssertState(10000);
 
     // Cleanup
-    release(0, 1.089, 0, 0);
+    release(0, 0.099, 0, 0);
     assertFalse(rm.inUse());
 
-
     // Given that CPU has a large initial allocation:
     acquire(0, 0.99, 0, 0);
 
     // When a request for a small CPU allocation is made,
     // Then the request fails:
-    assertThat(acquireNonblocking(0, 0.099, 0, 0)).isNull();
-
+    TestThread thread2 =
+        new TestThread() {
+          @Override
+          public void runTest() throws Exception {
+            assertThat(acquireNonblocking(0, 0.099, 0, 0)).isNull();
+          }
+        };
+    thread2.start();
+    thread2.joinAndAssertState(10000);
     // Note that this behavior is surprising and probably not intended.
   }
 
@@ -162,7 +186,15 @@ public class ResourceManagerTest {
 
     // When a request for RAM is made that would slightly overallocate RAM,
     // Then the request fails:
-    assertThat(acquireNonblocking(600, 0, 0, 0)).isNull();
+    TestThread thread1 =
+        new TestThread() {
+          @Override
+          public void runTest() throws Exception {
+            assertThat(acquireNonblocking(600, 0, 0, 0)).isNull();
+          }
+        };
+    thread1.start();
+    thread1.joinAndAssertState(10000);
   }
 
   @Test
@@ -174,7 +206,15 @@ public class ResourceManagerTest {
 
     // When a request for IO is made that would slightly overallocate IO,
     // Then the request fails:
-    assertThat(acquireNonblocking(0, 0, 0.6, 0)).isNull();
+    TestThread thread1 =
+        new TestThread() {
+          @Override
+          public void runTest() throws Exception {
+            assertThat(acquireNonblocking(0, 0, 0.6, 0)).isNull();
+          }
+        };
+    thread1.start();
+    thread1.joinAndAssertState(10000);
   }
 
   @Test
@@ -186,7 +226,15 @@ public class ResourceManagerTest {
 
     // When a request for tests is made that would slightly overallocate tests,
     // Then the request fails:
-    assertThat(acquireNonblocking(0, 0, 0, 2)).isNull();
+    TestThread thread1 =
+        new TestThread() {
+          @Override
+          public void runTest() throws Exception {
+            assertThat(acquireNonblocking(0, 0, 0, 2)).isNull();
+          }
+        };
+    thread1.start();
+    thread1.joinAndAssertState(10000);
   }
 
   @Test
@@ -323,7 +371,11 @@ public class ResourceManagerTest {
 
   @Test
   public void testOutOfOrderAllocation() throws Exception {
+    final CyclicBarrier sync3 = new CyclicBarrier(2);
+    final CyclicBarrier sync4 = new CyclicBarrier(2);
+
     assertFalse(rm.inUse());
+
     TestThread thread1 = new TestThread () {
       @Override public void runTest() throws Exception {
         sync.await();
@@ -333,6 +385,7 @@ public class ResourceManagerTest {
         sync.await();
       }
     };
+
     TestThread thread2 = new TestThread() {
       @Override public void runTest() throws Exception {
         // Wait till other thread will be locked
@@ -349,24 +402,69 @@ public class ResourceManagerTest {
         release(200, 0.5, 0, 0);
       }
     };
-    acquire(900, 0.9, 0, 0);
+
+    TestThread thread3 =
+        new TestThread() {
+          @Override
+          public void runTest() throws Exception {
+            acquire(100, 0.4, 0, 0);
+            sync3.await();
+            sync3.await();
+            release(100, 0.4, 0, 0);
+          }
+        };
+
+    TestThread thread4 =
+        new TestThread() {
+          @Override
+          public void runTest() throws Exception {
+            acquire(750, 0.3, 0, 0);
+            sync4.await();
+            sync4.await();
+            release(750, 0.3, 0, 0);
+          }
+        };
+
+    // Lock 900 MB, 0.9 CPU in total (spread over three threads so that we can individually release
+    // parts of it).
+    acquire(50, 0.2, 0, 0);
+    thread3.start();
+    thread4.start();
+    sync3.await(1, TimeUnit.SECONDS);
+    sync4.await(1, TimeUnit.SECONDS);
     validate(1);
+
+    // Start thread1, which will try to acquire 900 MB, 0.5 CPU, but can't, so it has to wait.
     thread1.start();
     sync.await(1, TimeUnit.SECONDS);
+
+    // Start thread2, which will successfully acquire and release 100 MB, 0.1 CPU.
     thread2.start();
+    // Signal thread2 to acquire 200 MB and 0.5 CPU, which will block.
     sync2.await(1, TimeUnit.SECONDS);
-    //Waiting till both threads are locked.
+
+    // Waiting till both threads are locked.
     while (rm.getWaitCount() < 2) {
       Thread.yield();
     }
+
     validate(3); // Thread1 is now first in the queue and Thread2 is second.
-    release(100, 0.4, 0, 0); // This allows Thread2 to continue out of order.
+
+    // Release 100 MB, 0.4 CPU. This allows Thread2 to continue out of order.
+    sync3.await(1, TimeUnit.SECONDS);
     sync2.await(1, TimeUnit.SECONDS);
-    release(750, 0.3, 0, 0); // At this point thread1 will finally acquire resources.
+
+    // Release 750 MB, 0.3 CPU. At this point thread1 will finally acquire resources.
+    sync4.await(1, TimeUnit.SECONDS);
     sync.await(1, TimeUnit.SECONDS);
+
+    // Release all remaining resources.
     release(50, 0.2, 0, 0);
     thread1.join();
     thread2.join();
+    thread3.join();
+    thread4.join();
+
     assertFalse(rm.inUse());
   }