Fix a bug in ParallelVisitor which prevents visitation task from being interrupted eagerly

The original code swallows the InterruptedException, sets the interrupt bit and
stops new tasks from being submitted. However it does not actively send an
interrupt signal to all running and pending tasks already in the pool. It is
partly due to the misleading syntax of awaitTermination, which actually quitely
waits for all tasks to exit even if interruptWorkers is set to true. Both
errors are fixed in this change.

RELNOTES: None
PiperOrigin-RevId: 157762029

2 files changed, 17 insertions, 18 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/concurrent/AbstractQueueVisitor.java b/src/main/java/com/google/devtools/build/lib/concurrent/AbstractQueueVisitor.java
index f82ead9512..7c0809d431 100644
--- a/src/main/java/com/google/devtools/build/lib/concurrent/AbstractQueueVisitor.java
+++ b/src/main/java/com/google/devtools/build/lib/concurrent/AbstractQueueVisitor.java
@@ -201,6 +201,20 @@ public class AbstractQueueVisitor implements QuiescingExecutor {
 
   @Override
   public final void awaitQuiescence(boolean interruptWorkers) throws InterruptedException {
+    Throwables.propagateIfPossible(catastrophe);
+    try {
+      synchronized (zeroRemainingTasks) {
+        while (remainingTasks.get() != 0 && !jobsMustBeStopped) {
+          zeroRemainingTasks.wait();
+        }
+      }
+    } catch (InterruptedException e) {
+      // Mark the visitor, so that it's known to be interrupted, and
+      // then break out of here, stop the worker threads and return ASAP,
+      // sending the interruption to the parent thread.
+      setInterrupted();
+    }
+
     awaitTermination(interruptWorkers);
   }
 
@@ -431,26 +445,12 @@ public class AbstractQueueVisitor implements QuiescingExecutor {
    * any worker thread failed unexpectedly.
    */
   protected final void awaitTermination(boolean interruptWorkers) throws InterruptedException {
-    Throwables.propagateIfPossible(catastrophe);
-    try {
-      synchronized (zeroRemainingTasks) {
-        while (remainingTasks.get() != 0 && !jobsMustBeStopped) {
-          zeroRemainingTasks.wait();
-        }
-      }
-    } catch (InterruptedException e) {
-      // Mark the visitor, so that it's known to be interrupted, and
-      // then break out of here, stop the worker threads and return ASAP,
-      // sending the interruption to the parent thread.
-      setInterrupted();
-    }
-
     reallyAwaitTermination(interruptWorkers);
 
     if (isInterrupted()) {
       // Set interrupted bit on current thread so that callers can see that it was interrupted. Note
       // that if the thread was interrupted while awaiting termination, we might not hit this
-      // codepath, but then the current thread's interrupt bit is already set, so we are fine.
+      // code path, but then the current thread's interrupt bit is already set, so we are fine.
       Thread.currentThread().interrupt();
     }
     // Throw the first unhandled (worker thread) exception in the main thread. We throw an unchecked
diff --git a/src/main/java/com/google/devtools/build/lib/query2/ParallelVisitor.java b/src/main/java/com/google/devtools/build/lib/query2/ParallelVisitor.java
index 73e291e118..e98a6afb7e 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/ParallelVisitor.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/ParallelVisitor.java
@@ -115,7 +115,7 @@ public abstract class ParallelVisitor<T> {
   }
 
   /** Factory for {@link ParallelVisitor} instances. */
-  public static interface Factory {
+  public interface Factory {
     ParallelVisitor<?> create();
   }
 
@@ -263,11 +263,10 @@ public abstract class ParallelVisitor<T> {
         } catch (InterruptedException e) {
           // If the main thread waiting for completion of the visitation is interrupted, we should
           // gracefully terminate all running and pending tasks before exit. If QueryException
-          // occured in any of the worker thread, awaitTerminationAndPropagateErrorsIfAny
+          // occurred in any of the worker thread, awaitTerminationAndPropagateErrorsIfAny
           // propagates the QueryException instead of InterruptedException.
           setInterrupted();
           awaitTerminationAndPropagateErrorsIfAny();
-          throw e;
         }
       }