Remove ability of AbstractQueueVisitor to continue after an interrupt. That functionality was only used in tests.

--
MOS_MIGRATED_REVID=124841573

7 files changed, 42 insertions, 96 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 8c9fc30d65..3833ab3868 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
@@ -130,9 +130,6 @@ public class AbstractQueueVisitor implements QuiescingExecutor {
   /** If {@code true}, don't run new actions after an uncaught exception. */
   private final boolean failFastOnException;
 
-  /** If {@code true}, don't run new actions after an interrupt. */
-  private final boolean failFastOnInterrupt;
-
   /** If {@code true}, shut down the {@link ExecutorService} on completion. */
   private final boolean ownExecutorService;
 
@@ -152,7 +149,6 @@ public class AbstractQueueVisitor implements QuiescingExecutor {
    * @param keepAliveTime the keep-alive time for the {@link ExecutorService}, if applicable.
    * @param units the time units of keepAliveTime.
    * @param failFastOnException if {@code true}, don't run new actions after an uncaught exception.
-   * @param failFastOnInterrupt if {@code true}, don't run new actions after an interrupt.
    * @param poolName sets the name of threads spawned by the {@link ExecutorService}. If {@code
    *                 null}, default thread naming will be used.
    */
@@ -162,7 +158,6 @@ public class AbstractQueueVisitor implements QuiescingExecutor {
       long keepAliveTime,
       TimeUnit units,
       boolean failFastOnException,
-      boolean failFastOnInterrupt,
       String poolName) {
     this(
         concurrent,
@@ -170,7 +165,6 @@ public class AbstractQueueVisitor implements QuiescingExecutor {
         keepAliveTime,
         units,
         failFastOnException,
-        failFastOnInterrupt,
         poolName,
         EXECUTOR_FACTORY,
         ErrorClassifier.DEFAULT,
@@ -188,7 +182,6 @@ public class AbstractQueueVisitor implements QuiescingExecutor {
    * @param keepAliveTime the keep-alive time for the {@link ExecutorService}, if applicable.
    * @param units the time units of keepAliveTime.
    * @param failFastOnException if {@code true}, don't run new actions after an uncaught exception.
-   * @param failFastOnInterrupt if {@code true}, don't run new actions after an interrupt.
    * @param poolName sets the name of threads spawned by the {@link ExecutorService}. If {@code
    *                 null}, default thread naming will be used.
    * @param errorClassifier an error classifier used to determine whether to log and/or stop jobs.
@@ -200,7 +193,6 @@ public class AbstractQueueVisitor implements QuiescingExecutor {
       long keepAliveTime,
       TimeUnit units,
       boolean failFastOnException,
-      boolean failFastOnInterrupt,
       String poolName,
       ErrorClassifier errorClassifier,
       ErrorHandler errorHandler) {
@@ -210,7 +202,6 @@ public class AbstractQueueVisitor implements QuiescingExecutor {
         keepAliveTime,
         units,
         failFastOnException,
-        failFastOnInterrupt,
         poolName,
         EXECUTOR_FACTORY,
         errorClassifier,
@@ -228,7 +219,6 @@ public class AbstractQueueVisitor implements QuiescingExecutor {
    * @param keepAliveTime the keep-alive time for the {@link ExecutorService}, if applicable.
    * @param units the time units of keepAliveTime.
    * @param failFastOnException if {@code true}, don't run new actions after an uncaught exception.
-   * @param failFastOnInterrupt if {@code true}, don't run new actions after interrupt.
    * @param poolName sets the name of threads spawned by the {@link ExecutorService}. If {@code
    *                 null}, default thread naming will be used.
    * @param executorFactory the factory for constructing the executor service if {@code concurrent}
@@ -242,7 +232,6 @@ public class AbstractQueueVisitor implements QuiescingExecutor {
       long keepAliveTime,
       TimeUnit units,
       boolean failFastOnException,
-      boolean failFastOnInterrupt,
       String poolName,
       Function<ExecutorParams, ? extends ExecutorService> executorFactory,
       ErrorClassifier errorClassifier,
@@ -252,7 +241,6 @@ public class AbstractQueueVisitor implements QuiescingExecutor {
     Preconditions.checkNotNull(errorClassifier);
     this.concurrent = concurrent;
     this.failFastOnException = failFastOnException;
-    this.failFastOnInterrupt = failFastOnInterrupt;
     this.ownExecutorService = true;
     this.executorService =
         concurrent
@@ -267,59 +255,20 @@ public class AbstractQueueVisitor implements QuiescingExecutor {
   /**
    * Create the {@link AbstractQueueVisitor}.
    *
-   * @param concurrent {@code true} if concurrency should be enabled. Only set to {@code false}
-   *                   for debugging.
-   * @param parallelism a measure of parallelism for the {@link ExecutorService}, such as {@code
-   *                    parallelism} in {@link java.util.concurrent.ForkJoinPool}, or both {@code
-   *                    corePoolSize} and {@code maximumPoolSize} in {@link ThreadPoolExecutor}.
-   * @param keepAliveTime the keep-alive time for the {@link ExecutorService}, if applicable.
-   * @param units the time units of keepAliveTime.
-   * @param failFastOnException if {@code true}, don't run new actions after an uncaught exception.
-   * @param poolName sets the name of threads spawned by the {@link ExecutorService}. If {@code
-   *                 null}, default thread naming will be used.
-   */
-  public AbstractQueueVisitor(
-      boolean concurrent,
-      int parallelism,
-      long keepAliveTime,
-      TimeUnit units,
-      boolean failFastOnException,
-      String poolName) {
-    this(
-        concurrent,
-        parallelism,
-        keepAliveTime,
-        units,
-        failFastOnException,
-        true,
-        poolName,
-        EXECUTOR_FACTORY,
-        ErrorClassifier.DEFAULT,
-        ErrorHandler.NullHandler.INSTANCE);
-  }
-
-  /**
-   * Create the {@link AbstractQueueVisitor}.
-   *
    * @param executorService The {@link ExecutorService} to use.
    * @param shutdownOnCompletion If {@code true}, pass ownership of the {@link ExecutorService} to
    *                             this class. The service will be shut down after a
    *                             call to {@link #awaitQuiescence}. Callers must not shutdown the
    *                             {@link ExecutorService} while queue visitors use it.
    * @param failFastOnException if {@code true}, don't run new actions after an uncaught exception.
-   * @param failFastOnInterrupt if {@code true}, don't run new actions after an interrupt.
    */
   public AbstractQueueVisitor(
-      ExecutorService executorService,
-      boolean shutdownOnCompletion,
-      boolean failFastOnException,
-      boolean failFastOnInterrupt) {
+      ExecutorService executorService, boolean shutdownOnCompletion, boolean failFastOnException) {
     this(
         /*concurrent=*/ true,
         executorService,
         shutdownOnCompletion,
         failFastOnException,
-        failFastOnInterrupt,
         ErrorClassifier.DEFAULT,
         ErrorHandler.NullHandler.INSTANCE);
   }
@@ -335,18 +284,15 @@ public class AbstractQueueVisitor implements QuiescingExecutor {
    *                             call to {@link #awaitQuiescence}. Callers must not shut down the
    *                             {@link ExecutorService} while queue visitors use it.
    * @param failFastOnException if {@code true}, don't run new actions after an uncaught exception.
-   * @param failFastOnInterrupt if {@code true}, don't run new actions after an interrupt.
    */
   public AbstractQueueVisitor(
       boolean concurrent,
       ExecutorService executorService,
       boolean shutdownOnCompletion,
-      boolean failFastOnException,
-      boolean failFastOnInterrupt) {
+      boolean failFastOnException) {
     Preconditions.checkArgument(executorService != null || !concurrent);
     this.concurrent = concurrent;
     this.failFastOnException = failFastOnException;
-    this.failFastOnInterrupt = failFastOnInterrupt;
     this.ownExecutorService = shutdownOnCompletion;
     this.executorService = executorService;
     this.errorClassifier = ErrorClassifier.DEFAULT;
@@ -364,7 +310,6 @@ public class AbstractQueueVisitor implements QuiescingExecutor {
    *                             call to {@link #awaitQuiescence}. Callers must not shut down the
    *                             {@link ExecutorService} while queue visitors use it.
    * @param failFastOnException if {@code true}, don't run new actions after an uncaught exception.
-   * @param failFastOnInterrupt if {@code true}, don't run new actions after an interrupt.
    * @param errorClassifier an error classifier used to determine whether to log and/or stop jobs.
    * @param errorHandler a handler for classified errors.
    */
@@ -373,13 +318,11 @@ public class AbstractQueueVisitor implements QuiescingExecutor {
       ExecutorService executorService,
       boolean shutdownOnCompletion,
       boolean failFastOnException,
-      boolean failFastOnInterrupt,
       ErrorClassifier errorClassifier,
       ErrorHandler errorHandler) {
     Preconditions.checkArgument(executorService != null || !concurrent);
     this.concurrent = concurrent;
     this.failFastOnException = failFastOnException;
-    this.failFastOnInterrupt = failFastOnInterrupt;
     this.ownExecutorService = shutdownOnCompletion;
     this.executorService = executorService;
     this.errorClassifier = errorClassifier;
@@ -404,7 +347,6 @@ public class AbstractQueueVisitor implements QuiescingExecutor {
         keepAlive,
         units,
         false,
-        true,
         poolName,
         EXECUTOR_FACTORY,
         ErrorClassifier.DEFAULT,
@@ -580,15 +522,17 @@ public class AbstractQueueVisitor implements QuiescingExecutor {
 
   /** If this returns true, don't enqueue new actions. */
   protected boolean blockNewActions() {
-    return (failFastOnInterrupt && isInterrupted()) || (unhandled != null && failFastOnException);
+    return isInterrupted() || (unhandled != null && failFastOnException);
   }
 
   @VisibleForTesting
+  @Override
   public final CountDownLatch getExceptionLatchForTestingOnly() {
     return exceptionLatch;
   }
 
   @VisibleForTesting
+  @Override
   public final CountDownLatch getInterruptionLatchForTestingOnly() {
     return interruptedLatch;
   }
@@ -614,7 +558,6 @@ public class AbstractQueueVisitor implements QuiescingExecutor {
    * worker thread failed unexpectedly.
    */
   private void awaitTermination(boolean interruptWorkers) throws InterruptedException {
-    Preconditions.checkState(failFastOnInterrupt || !interruptWorkers);
     Throwables.propagateIfPossible(catastrophe);
     try {
       synchronized (zeroRemainingTasks) {
diff --git a/src/main/java/com/google/devtools/build/lib/concurrent/ForkJoinQuiescingExecutor.java b/src/main/java/com/google/devtools/build/lib/concurrent/ForkJoinQuiescingExecutor.java
index 5aed120470..54bb572665 100644
--- a/src/main/java/com/google/devtools/build/lib/concurrent/ForkJoinQuiescingExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/concurrent/ForkJoinQuiescingExecutor.java
@@ -30,7 +30,6 @@ public class ForkJoinQuiescingExecutor extends AbstractQueueVisitor {
         forkJoinPool,
         /*shutdownOnCompletion=*/ true,
         /*failFastOnException=*/ true,
-        /*failFastOnInterrupt=*/ true,
         errorClassifier,
         errorHandler);
   }
diff --git a/src/main/java/com/google/devtools/build/skyframe/InvalidatingNodeVisitor.java b/src/main/java/com/google/devtools/build/skyframe/InvalidatingNodeVisitor.java
index bb7f05dd69..6eec236b4e 100644
--- a/src/main/java/com/google/devtools/build/skyframe/InvalidatingNodeVisitor.java
+++ b/src/main/java/com/google/devtools/build/skyframe/InvalidatingNodeVisitor.java
@@ -110,7 +110,6 @@ public abstract class InvalidatingNodeVisitor<TGraph extends ThinNodeQueryableGr
             /*keepAliveTime=*/ 1,
             /*units=*/ TimeUnit.SECONDS,
             /*failFastOnException=*/ true,
-            /*failFastOnInterrupt=*/ true,
             "skyframe-invalidator",
             executorFactory,
             errorClassifier,
diff --git a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
index d9307c6132..9dfcd744bd 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
@@ -716,7 +716,6 @@ public final class ParallelEvaluator implements Evaluator {
               /*keepAliveTime=*/ 1,
               TimeUnit.SECONDS,
               /*failFastOnException*/ true,
-              /*failFastOnInterrupt*/ true,
               "skyframe-evaluator",
               VALUE_VISITOR_ERROR_CLASSIFIER,
               errorHandler);
diff --git a/src/test/java/com/google/devtools/build/lib/actions/ConcurrentMultimapWithHeadElementTest.java b/src/test/java/com/google/devtools/build/lib/actions/ConcurrentMultimapWithHeadElementTest.java
index 832ee7242d..456b394a85 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/ConcurrentMultimapWithHeadElementTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/ConcurrentMultimapWithHeadElementTest.java
@@ -134,8 +134,13 @@ public class ConcurrentMultimapWithHeadElementTest {
     private final AtomicInteger actionCount = new AtomicInteger(0);
 
     private StressTester() {
-      super(/*concurrent=*/true, 200, 1, TimeUnit.SECONDS,
-          /*failFastOnException=*/true, /*failFastOnInterrupt=*/true, "action-graph-test");
+      super(
+          /*concurrent=*/ true,
+          200,
+          1,
+          TimeUnit.SECONDS,
+          /*failFastOnException=*/ true,
+          "action-graph-test");
     }
 
     private void addAndRemove(final Boolean key, final Integer add, final Integer remove) {
diff --git a/src/test/java/com/google/devtools/build/lib/actions/MapBasedActionGraphTest.java b/src/test/java/com/google/devtools/build/lib/actions/MapBasedActionGraphTest.java
index 2dd07ed868..a527549e6c 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/MapBasedActionGraphTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/MapBasedActionGraphTest.java
@@ -81,8 +81,13 @@ public class MapBasedActionGraphTest {
     private final AtomicInteger actionCount = new AtomicInteger(0);
 
     private ActionRegisterer() {
-      super(/*concurrent=*/true, 200, 1, TimeUnit.SECONDS,
-          /*failFastOnException=*/true, /*failFastOnInterrupt=*/true, "action-graph-test");
+      super(
+          /*concurrent=*/ true,
+          200,
+          1,
+          TimeUnit.SECONDS,
+          /*failFastOnException=*/ true,
+          "action-graph-test");
       FileSystem fileSystem = new InMemoryFileSystem(BlazeClock.instance());
       Path path = fileSystem.getPath("/root/foo");
       output = new Artifact(path, Root.asDerivedRoot(path));
diff --git a/src/test/java/com/google/devtools/build/lib/concurrent/AbstractQueueVisitorTest.java b/src/test/java/com/google/devtools/build/lib/concurrent/AbstractQueueVisitorTest.java
index 7c751fce36..c22f5eb68a 100644
--- a/src/test/java/com/google/devtools/build/lib/concurrent/AbstractQueueVisitorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/concurrent/AbstractQueueVisitorTest.java
@@ -229,7 +229,7 @@ public class AbstractQueueVisitorTest {
     executor.awaitTermination(TestUtils.WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS);
   }
 
-  private void assertInterruptWorkers(ThreadPoolExecutor executor) throws Exception {
+  private static void assertInterruptWorkers(ThreadPoolExecutor executor) throws Exception {
     final CountDownLatch latch1 = new CountDownLatch(1);
     final CountDownLatch latch2 = new CountDownLatch(1);
     final boolean[] workerThreadInterrupted = { false };
@@ -266,14 +266,13 @@ public class AbstractQueueVisitorTest {
   @Test
   public void failFast() throws Exception {
     // In failFast mode, we only run actions queued before the exception.
-    assertFailFast(null, true, false, false, "a", "b");
+    assertFailFast(null, true, false, "a", "b");
 
     // In !failFast mode, we complete all queued actions.
-    assertFailFast(null, false, false, false, "a", "b", "1", "2");
+    assertFailFast(null, false, false, "a", "b", "1", "2");
 
     // Now check fail-fast on interrupt:
-    assertFailFast(null, false, true, true, "a", "b");
-    assertFailFast(null, false, false, true, "a", "b", "1", "2");
+    assertFailFast(null, false, true, "a", "b");
   }
 
   @Test
@@ -281,26 +280,29 @@ public class AbstractQueueVisitorTest {
     ThreadPoolExecutor executor = new ThreadPoolExecutor(5, 5, 0, TimeUnit.SECONDS,
                                                          new LinkedBlockingQueue<Runnable>());
     // In failFast mode, we only run actions queued before the exception.
-    assertFailFast(executor, true, false, false, "a", "b");
+    assertFailFast(executor, true, false, "a", "b");
 
     // In !failFast mode, we complete all queued actions.
-    assertFailFast(executor, false, false, false, "a", "b", "1", "2");
+    assertFailFast(executor, false, false, "a", "b", "1", "2");
 
     // Now check fail-fast on interrupt:
-    assertFailFast(executor, false, true, true, "a", "b");
-    assertFailFast(executor, false, false, true, "a", "b", "1", "2");
+    assertFailFast(executor, false, true, "a", "b");
 
     executor.shutdown();
     assertTrue(executor.awaitTermination(TestUtils.WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS));
   }
 
-  private void assertFailFast(ThreadPoolExecutor executor,
-                              boolean failFastOnException, boolean failFastOnInterrupt,
-                              boolean interrupt, String... expectedVisited) throws Exception {
+  private static void assertFailFast(
+      ThreadPoolExecutor executor,
+      boolean failFastOnException,
+      boolean interrupt,
+      String... expectedVisited)
+      throws Exception {
     assertTrue(executor == null || !executor.isShutdown());
-    AbstractQueueVisitor visitor = (executor == null)
-        ? new ConcreteQueueVisitor(failFastOnException, failFastOnInterrupt)
-        : new ConcreteQueueVisitor(executor, failFastOnException, failFastOnInterrupt);
+    AbstractQueueVisitor visitor =
+        (executor == null)
+            ? new ConcreteQueueVisitor(failFastOnException)
+            : new ConcreteQueueVisitor(executor, failFastOnException);
 
     List<String> visitedList = Collections.synchronizedList(Lists.<String>newArrayList());
 
@@ -457,7 +459,7 @@ public class AbstractQueueVisitorTest {
     assertTrue(criticalErrorSeen.get());
   }
 
-  private Runnable throwingRunnable() {
+  private static Runnable throwingRunnable() {
     return new Runnable() {
       @Override
       public void run() {
@@ -466,7 +468,7 @@ public class AbstractQueueVisitorTest {
     };
   }
 
-  private Runnable interruptingRunnable(final Thread thread) {
+  private static Runnable interruptingRunnable(final Thread thread) {
     return new Runnable() {
       @Override
       public void run() {
@@ -517,7 +519,7 @@ public class AbstractQueueVisitorTest {
     }
 
     public CountingQueueVisitor(ThreadPoolExecutor executor) {
-      super(executor, false, true, true);
+      super(executor, false, true);
     }
 
     public void enqueue() {
@@ -548,17 +550,12 @@ public class AbstractQueueVisitorTest {
       super(5, 3L, TimeUnit.SECONDS, THREAD_NAME);
     }
 
-    public ConcreteQueueVisitor(boolean failFast, boolean failFastOnInterrupt) {
-      super(true, 5, 3L, TimeUnit.SECONDS, failFast, failFastOnInterrupt, THREAD_NAME);
-    }
-
-    public ConcreteQueueVisitor(ThreadPoolExecutor executor, boolean failFast,
-        boolean failFastOnInterrupt) {
-      super(executor, /*shutdownOnCompletion=*/false, failFast, failFastOnInterrupt);
+    public ConcreteQueueVisitor(boolean failFast) {
+      super(true, 5, 3L, TimeUnit.SECONDS, failFast, THREAD_NAME);
     }
 
     public ConcreteQueueVisitor(ThreadPoolExecutor executor, boolean failFast) {
-      super(executor, /*shutdownOnCompletion=*/false, failFast, true);
+      super(executor, /*shutdownOnCompletion=*/ false, failFast);
     }
   }
 
@@ -570,7 +567,6 @@ public class AbstractQueueVisitorTest {
         executor,
         /*shutdownOnCompletion=*/ true,
         /*failFastOnException=*/ false,
-        /*failFastOnInterrupt=*/ true,
         new ErrorClassifier() {
           @Override
           protected ErrorClassification classifyException(Exception e) {