From 98ea68a20ddd58f1ee4bb67410484a1c8794556a Mon Sep 17 00:00:00 2001
From: Nathan Harmata <nharmata@google.com>
Date: Mon, 19 Oct 2015 20:08:48 +0000
Subject: Don't log SchedulerExceptions.

RELNOTES:  None

--
MOS_MIGRATED_REVID=105787681
---
 .../build/lib/concurrent/AbstractQueueVisitor.java | 42 ++++++++++++++++------
 .../build/skyframe/InvalidatingNodeVisitor.java    |  6 ++--
 .../devtools/build/skyframe/ParallelEvaluator.java | 10 ++++--
 .../lib/concurrent/AbstractQueueVisitorTest.java   |  8 ++---
 4 files changed, 47 insertions(+), 19 deletions(-)

(limited to 'src')

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 5fe85bbf65..7a0ab061ac 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
@@ -648,25 +648,33 @@ public class AbstractQueueVisitor {
     }
   }
 
+  /** Classification of an error thrown by an action. */
+  protected enum ErrorClassification {
+    // All running actions should be stopped.
+    CRITICAL,
+    // Same as CRITICAL, but also log the error.
+    CRITICAL_AND_LOG,
+    // Other running actions should be left alone.
+    NOT_CRITICAL
+  }
+
   /**
-   * If this returns true, that means the exception {@code e} is critical
-   * and all running actions should be stopped. {@link Error}s are always considered critical.
+   * Classifies {@code e}. {@link Error}s are always classified as {@code CRITICAL_AND_LOG}.
    *
-   * <p>Default value - always false. If different behavior is needed
+   * <p>Default value - always treat errors as {@code NOT_CRITICAL}. If different behavior is needed
    * then we should override this method in subclasses.
    *
    * @param e the exception object to check
    */
-  protected boolean isCriticalError(Throwable e) {
-    return false;
+  protected ErrorClassification classifyError(Throwable e) {
+    return ErrorClassification.NOT_CRITICAL;
   }
 
-  private boolean isCriticalErrorInternal(Throwable e) {
-    boolean isCritical = isCriticalError(e) || (e instanceof Error);
-    if (isCritical) {
-      LOG.log(Level.WARNING, "Found critical error in queue visitor", e);
+  private ErrorClassification classifyErrorInternal(Throwable e) {
+    if (e instanceof Error) {
+      return ErrorClassification.CRITICAL_AND_LOG;
     }
-    return isCritical;
+    return classifyError(e);
   }
 
   /**
@@ -674,7 +682,19 @@ public class AbstractQueueVisitor {
    * to stop all jobs inside {@link #awaitTermination(boolean)}.
    */
   private synchronized void markToStopAllJobsIfNeeded(Throwable e) {
-    if (isCriticalErrorInternal(e) && !jobsMustBeStopped) {
+    boolean critical = false;
+    switch (classifyErrorInternal(e)) {
+        case CRITICAL_AND_LOG:
+          critical = true;
+          LOG.log(Level.WARNING, "Found critical error in queue visitor", e);
+          break;
+        case CRITICAL:
+          critical = true;
+          break;
+        default:
+          break;
+    }
+    if (critical && !jobsMustBeStopped) {
       jobsMustBeStopped = true;
       synchronized (zeroRemainingTasks) {
         zeroRemainingTasks.notify();
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 459c7ffc7d..cd6a986acd 100644
--- a/src/main/java/com/google/devtools/build/skyframe/InvalidatingNodeVisitor.java
+++ b/src/main/java/com/google/devtools/build/skyframe/InvalidatingNodeVisitor.java
@@ -114,8 +114,10 @@ public abstract class InvalidatingNodeVisitor<TGraph extends ThinNodeQueryableGr
   }
 
   @Override
-  protected boolean isCriticalError(Throwable e) {
-    return e instanceof RuntimeException;
+  protected ErrorClassification classifyError(Throwable e) {
+    return e instanceof RuntimeException
+        ? ErrorClassification.CRITICAL_AND_LOG
+        : ErrorClassification.NOT_CRITICAL;
   }
 
   protected abstract long count();
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 6dc31eeaf6..243577ecc6 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
@@ -607,8 +607,14 @@ public final class ParallelEvaluator implements Evaluator {
     }
 
     @Override
-    protected boolean isCriticalError(Throwable e) {
-      return e instanceof RuntimeException;
+    protected ErrorClassification classifyError(Throwable e) {
+      if (e instanceof SchedulerException) {
+        return ErrorClassification.CRITICAL;
+      }
+      if (e instanceof RuntimeException) {
+        return ErrorClassification.CRITICAL_AND_LOG;
+      }
+      return ErrorClassification.NOT_CRITICAL;
     }
 
     protected void waitForCompletion() throws InterruptedException {
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 19b4274267..e7798bcd85 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
@@ -533,8 +533,8 @@ public class AbstractQueueVisitorTest {
     }
 
     @Override
-    protected boolean isCriticalError(Throwable e) {
-      return true;
+    protected ErrorClassification classifyError(Throwable e) {
+      return ErrorClassification.CRITICAL;
     }
   }
 
@@ -545,8 +545,8 @@ public class AbstractQueueVisitorTest {
     }
 
     @Override
-    protected boolean isCriticalError(Throwable e) {
-      return false;
+    protected ErrorClassification classifyError(Throwable e) {
+      return ErrorClassification.NOT_CRITICAL;
     }
   }
 }
-- 
cgit v1.2.3