Don't allow subclasses of AQV to treat Errors as non-critical. Also update the documentation for AQV#work to reflect the semantics of critical errors.

--
MOS_MIGRATED_REVID=103140100

1 files changed, 9 insertions, 4 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 fe96b61be2..21c1c66111 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
@@ -365,8 +365,9 @@ public class AbstractQueueVisitor {
    * rethrown, since it may indicate a programming bug. If callers handle the unchecked exception,
    * they may check the interrupted bit to see if the pool was interrupted.
    *
-   * @param interruptWorkers if true, interrupt worker threads when main thread gets an interrupt.
-   *        If false, just wait for them to terminate normally.
+   * @param interruptWorkers if true, interrupt worker threads if main thread gets an interrupt or
+   *        if a worker throws a critical error (see {@link #isCriticalError(Throwable)}). If
+   *        false, just wait for them to terminate normally.
    */
   protected void work(boolean interruptWorkers) throws InterruptedException {
     if (concurrent) {
@@ -633,7 +634,7 @@ public class AbstractQueueVisitor {
 
   /**
    * If this returns true, that means the exception {@code e} is critical
-   * and all running actions should be stopped.
+   * and all running actions should be stopped. {@link Error}s are always considered critical.
    *
    * <p>Default value - always false. If different behavior is needed
    * then we should override this method in subclasses.
@@ -644,6 +645,10 @@ public class AbstractQueueVisitor {
     return false;
   }
 
+  private boolean isCriticalErrorInternal(Throwable e) {
+    return isCriticalError(e) || (e instanceof Error);
+  }
+
   private void setRejectedExecutionHandler() {
     if (ownThreadPool) {
       pool.setRejectedExecutionHandler(new RejectedExecutionHandler() {
@@ -660,7 +665,7 @@ public class AbstractQueueVisitor {
    * to stop all jobs inside {@link #awaitTermination(boolean)}.
    */
   private synchronized void markToStopAllJobsIfNeeded(Throwable e) {
-    if (isCriticalError(e) && !jobsMustBeStopped) {
+    if (isCriticalErrorInternal(e) && !jobsMustBeStopped) {
       jobsMustBeStopped = true;
       synchronized (zeroRemainingTasks) {
         zeroRemainingTasks.notify();