commit 97feb57e948c73a435ce43a1da70236b1211441d
author nharmata <nharmata@google.com> Wed Jul 10 10:45:39 2019 -0700
committer Copybara-Service <copybara-worker@google.com> Wed Jul 10 10:46:37 2019 -0700
tree 6dc4a76e51b1bfa7f75bbea5f3691c165e95176b
parent b5c922574aadae434ef8cde75e6081a75ff4b8d7

Fix one check-then-act race in ExperimentalStateTracker#writeProgressBar.

#sampleOldestActions gets called via #writeProgressBar if #writeProgressBar observed 'activeActions.size() > 1'. But the 'activeActions' map may be empty by the time 'activeActions.size()' is evaluated again in #sampleOldestActions. In this case, we invoke the PriorityQueue ctor with an initialCapacity of 0 and the ctor throws an IllegalArgumentException.

RELNOTES: None
PiperOrigin-RevId: 257435797

src/main/java/com/google/devtools/build/lib/runtime/ExperimentalStateTracker.java

diff --git a/src/main/java/com/google/devtools/build/lib/runtime/ExperimentalStateTracker.java b/src/main/java/com/google/devtools/build/lib/runtime/ExperimentalStateTracker.java
index a2ba9ed..561aac7 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/ExperimentalStateTracker.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/ExperimentalStateTracker.java
@@ -798,21 +798,29 @@
   }
 
   private void sampleOldestActions(AnsiTerminalWriter terminalWriter) throws IOException {
-    int count = 0;
-    int totalCount = 0;
-    long nanoTime = clock.nanoTime();
-    int actionCount = activeActions.size();
-    Set<Artifact> toSkip = new HashSet<>();
-
+    // This method can only be called if 'activeActions.size()' was observed to be larger than 1 at
+    // some point in the past. But the 'activeActions' map can grow and shrink concurrent with this
+    // code here so we need to be very careful lest we fall victim to a check-then-act race.
+    int racyActiveActionsCount = activeActions.size();
     PriorityQueue<Map.Entry<Artifact, ActionState>> priorityHeap =
         new PriorityQueue<>(
-            activeActions.size(),
+            // The 'initialCapacity' parameter must be positive.
+            /*initialCapacity=*/ Math.max(racyActiveActionsCount, 1),
             Comparator.comparing(
                     (Map.Entry<Artifact, ActionState> entry) ->
                         entry.getValue().runningStrategiesBitmap == 0)
                 .thenComparingLong(entry -> entry.getValue().nanoStartTime)
                 .thenComparingInt(entry -> entry.getValue().hashCode()));
     priorityHeap.addAll(activeActions.entrySet());
+    // From this point on, we no longer consume 'activeActions'. So now it's sound to look at how
+    // many entries were added to 'priorityHeap' and act appropriately based on that count.
+    int actualObservedActiveActionsCount = priorityHeap.size();
+
+    int count = 0;
+    int totalCount = 0;
+    long nanoTime = clock.nanoTime();
+    Set<Artifact> toSkip = new HashSet<>();
+
     while (!priorityHeap.isEmpty()) {
       Map.Entry<Artifact, ActionState> entry = priorityHeap.poll();
       totalCount++;
@@ -825,12 +833,16 @@
         break;
       }
       int width =
-          targetWidth - 4 - ((count >= sampleSize && count < actionCount) ? AND_MORE.length() : 0);
+          targetWidth
+              - 4
+              - ((count >= sampleSize && count < actualObservedActiveActionsCount)
+                  ? AND_MORE.length()
+                  : 0);
       terminalWriter
           .newline()
           .append("    " + describeAction(entry.getValue(), nanoTime, width, toSkip));
     }
-    if (totalCount < actionCount) {
+    if (totalCount < actualObservedActiveActionsCount) {
       terminalWriter.append(AND_MORE);
     }
   }