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

2 files changed, 25 insertions, 6 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);
   }