Keep track of any RuntimeExceptions thrown during evaluation. Previously, RuntimeExceptions that were thrown after an InterruptedException was thrown would be swallowed.

--
MOS_MIGRATED_REVID=105902192

4 files changed, 153 insertions, 80 deletions

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 243577ecc6..dd2089aa18 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
@@ -41,7 +41,6 @@ import com.google.devtools.build.skyframe.EvaluationProgressReceiver.EvaluationS
 import com.google.devtools.build.skyframe.MemoizingEvaluator.EmittedEventState;
 import com.google.devtools.build.skyframe.NodeEntry.DependencyState;
 import com.google.devtools.build.skyframe.NodeEntry.DirtyState;
-import com.google.devtools.build.skyframe.Scheduler.SchedulerException;
 import com.google.devtools.build.skyframe.SkyFunctionException.ReifiedSkyFunctionException;
 
 import java.util.ArrayDeque;
@@ -596,6 +595,7 @@ public final class ParallelEvaluator implements Evaluator {
   private class ValueVisitor extends AbstractQueueVisitor {
     private AtomicBoolean preventNewEvaluations = new AtomicBoolean(false);
     private final Set<SkyKey> inflightNodes = Sets.newConcurrentHashSet();
+    private final Set<RuntimeException> crashes = Sets.newConcurrentHashSet();
 
     private ValueVisitor(int threadCount) {
       super(/*concurrent*/true,
@@ -652,6 +652,14 @@ public final class ParallelEvaluator implements Evaluator {
       return preventNewEvaluations.compareAndSet(false, true);
     }
 
+    void noteCrash(RuntimeException e) {
+      crashes.add(e);
+    }
+
+    Collection<RuntimeException> getCrashes() {
+      return crashes;
+    }
+
     void notifyDone(SkyKey key) {
       inflightNodes.remove(key);
     }
@@ -906,7 +914,9 @@ public final class ParallelEvaluator implements Evaluator {
         // Programmer error (most likely NPE or a failed precondition in a SkyFunction). Output
         // some context together with the exception.
         String msg = prepareCrashMessage(skyKey, state.getInProgressReverseDeps());
-        throw new RuntimeException(msg, re);
+        RuntimeException ex = new RuntimeException(msg, re);
+        visitor.noteCrash(ex);
+        throw ex;
       } finally {
         env.doneBuilding();
         long elapsedTimeNanos = Profiler.nanoTimeMaybe() - startTime;
@@ -1200,6 +1210,10 @@ public final class ParallelEvaluator implements Evaluator {
     try {
       visitor.waitForCompletion();
     } catch (final SchedulerException e) {
+      if (!visitor.getCrashes().isEmpty()) {
+        reporter.handle(Event.error("Crashes detected: " + visitor.getCrashes()));
+        throw Iterables.getFirst(visitor.getCrashes(), null);
+      }
       Throwables.propagateIfPossible(e.getCause(), InterruptedException.class);
       if (Thread.interrupted()) {
         // As per the contract of AbstractQueueVisitor#work, if an unchecked exception is thrown and
@@ -1228,6 +1242,7 @@ public final class ParallelEvaluator implements Evaluator {
                     graph.get(errorKey).getValueMaybeWithMetadata()));
       }
     }
+    Preconditions.checkState(visitor.getCrashes().isEmpty(), visitor.getCrashes());
 
     // Successful evaluation, either because keepGoing or because we actually did succeed.
     // TODO(bazel-team): Maybe report root causes during the build for lower latency.
diff --git a/src/main/java/com/google/devtools/build/skyframe/Scheduler.java b/src/main/java/com/google/devtools/build/skyframe/Scheduler.java
deleted file mode 100644
index 8c3795a2be..0000000000
--- a/src/main/java/com/google/devtools/build/skyframe/Scheduler.java
+++ /dev/null
@@ -1,78 +0,0 @@
-// Copyright 2014 The Bazel Authors. All rights reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//    http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-package com.google.devtools.build.skyframe;
-
-import com.google.common.base.Preconditions;
-
-import javax.annotation.Nullable;
-
-/**
- * A work queue -- takes {@link Runnable}s and runs them when requested.
- */
-interface Scheduler {
-  /**
-   * Schedules a new action to be eventually done.
-   */
-  void schedule(Runnable action);
-
-  /**
-   * Runs the actions that have been scheduled. These actions can in turn schedule new actions,
-   * which will be run as well.
-   *
-   * @throw SchedulerException wrapping a scheduled action's exception.
-   */
-  void run() throws SchedulerException;
-
-  /**
-   * Wrapper exception that {@link Runnable}s can throw, to be caught and handled
-   * by callers of {@link #run}.
-   */
-  static class SchedulerException extends RuntimeException {
-    private final SkyKey failedValue;
-    private final ErrorInfo errorInfo;
-
-    private SchedulerException(@Nullable Exception cause, @Nullable ErrorInfo errorInfo,
-        SkyKey failedValue) {
-      super(errorInfo != null ? errorInfo.getException() : cause);
-      this.errorInfo = errorInfo;
-      this.failedValue = Preconditions.checkNotNull(failedValue, errorInfo);
-    }
-
-    /**
-     * Returns a SchedulerException wrapping an expected error, e.g. an error describing an expected
-     * build failure when trying to evaluate the given value, that should cause Skyframe to produce
-     * useful error information to the user.
-     */
-    static SchedulerException ofError(ErrorInfo errorInfo, SkyKey failedValue) {
-      Preconditions.checkNotNull(errorInfo);
-      return new SchedulerException(errorInfo.getException(), errorInfo, failedValue);
-    }
-
-    /**
-     * Returns a SchedulerException wrapping an InterruptedException, e.g. if the user interrupts
-     * the build, that should cause Skyframe to exit as soon as possible.
-     */
-    static SchedulerException ofInterruption(InterruptedException cause, SkyKey failedValue) {
-      return new SchedulerException(cause, null, failedValue);
-    }
-
-    SkyKey getFailedValue() {
-      return failedValue;
-    }
-
-    @Nullable ErrorInfo getErrorInfo() {
-      return errorInfo;
-    }
-  }
-}
diff --git a/src/main/java/com/google/devtools/build/skyframe/SchedulerException.java b/src/main/java/com/google/devtools/build/skyframe/SchedulerException.java
new file mode 100644
index 0000000000..a1ade91fa2
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/skyframe/SchedulerException.java
@@ -0,0 +1,58 @@
+// Copyright 2015 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.skyframe;
+
+import com.google.common.base.Preconditions;
+
+import javax.annotation.Nullable;
+
+/** Wrapper exception that {@link Runnable}s can throw. */
+class SchedulerException extends RuntimeException {
+  private final SkyKey failedValue;
+  private final ErrorInfo errorInfo;
+
+  private SchedulerException(
+      @Nullable Exception cause, @Nullable ErrorInfo errorInfo, SkyKey failedValue) {
+    super(errorInfo != null ? errorInfo.getException() : cause);
+    this.errorInfo = errorInfo;
+    this.failedValue = Preconditions.checkNotNull(failedValue, errorInfo);
+  }
+
+  /**
+   * Returns a SchedulerException wrapping an expected error, e.g. an error describing an expected
+   * build failure when trying to evaluate the given value, that should cause Skyframe to produce
+   * useful error information to the user.
+   */
+  static SchedulerException ofError(ErrorInfo errorInfo, SkyKey failedValue) {
+    Preconditions.checkNotNull(errorInfo);
+    return new SchedulerException(errorInfo.getException(), errorInfo, failedValue);
+  }
+
+  /**
+   * Returns a SchedulerException wrapping an InterruptedException, e.g. if the user interrupts
+   * the build, that should cause Skyframe to exit as soon as possible.
+   */
+  static SchedulerException ofInterruption(InterruptedException cause, SkyKey failedValue) {
+    return new SchedulerException(cause, null, failedValue);
+  }
+
+  SkyKey getFailedValue() {
+    return failedValue;
+  }
+
+  @Nullable
+  ErrorInfo getErrorInfo() {
+    return errorInfo;
+  }
+}
diff --git a/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java b/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
index f19c1d1e66..2187cb7da3 100644
--- a/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
@@ -217,6 +217,84 @@ public class MemoizingEvaluatorTest {
   }
 
   @Test
+  public void crashAfterInterruptCrashes() throws Exception {
+    SkyKey failKey = GraphTester.skyKey("fail");
+    SkyKey badInterruptkey = GraphTester.skyKey("bad-interrupt");
+    tester.getOrCreate(failKey).setHasError(true);
+    // Given a SkyFunction implementation which is improperly coded to throw a runtime exception
+    // when it is interrupted,
+    tester
+        .getOrCreate(badInterruptkey)
+        .setBuilder(
+            new SkyFunction() {
+              @Nullable
+              @Override
+              public SkyValue compute(SkyKey skyKey, Environment env) {
+                try {
+                  Thread.sleep(TestUtils.WAIT_TIMEOUT_MILLISECONDS);
+                  throw new AssertionError("Shouldn't have slept so long");
+                } catch (InterruptedException e) {
+                  throw new RuntimeException("I don't like being woken up!");
+                }
+              }
+
+              @Nullable
+              @Override
+              public String extractTag(SkyKey skyKey) {
+                return null;
+              }
+            });
+
+    try {
+      // When it is interrupted during evaluation (here, caused by the failure of a sibling node
+      // during a no-keep-going evaluation),
+      EvaluationResult<StringValue> unexpectedResult =
+          tester.eval(/*keepGoing=*/ false, badInterruptkey, failKey);
+      fail(unexpectedResult.toString());
+    } catch (RuntimeException e) {
+      // Then the Evaluator#evaluate call throws a RuntimeException e where e.getCause() is the
+      // RuntimeException thrown by that SkyFunction.
+      assertThat(e.getCause()).hasMessage("I don't like being woken up!");
+    }
+  }
+
+  @Test
+  public void interruptAfterFailFails() throws Exception {
+    SkyKey failKey = GraphTester.skyKey("fail");
+    SkyKey interruptedKey = GraphTester.skyKey("interrupted");
+    tester.getOrCreate(failKey).setHasError(true);
+    // Given a SkyFunction implementation that is properly coded to as not to throw a
+    // runtime exception when it is interrupted,
+    tester
+        .getOrCreate(interruptedKey)
+        .setBuilder(
+            new SkyFunction() {
+              @Nullable
+              @Override
+              public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException {
+                Thread.sleep(TestUtils.WAIT_TIMEOUT_MILLISECONDS);
+                throw new AssertionError("Shouldn't have slept so long");
+              }
+
+              @Nullable
+              @Override
+              public String extractTag(SkyKey skyKey) {
+                return null;
+              }
+            });
+
+    // When it is interrupted during evaluation (here, caused by the failure of a sibling node
+    // during a no-keep-going evaluation),
+    EvaluationResult<StringValue> result =
+        tester.eval(/*keepGoing=*/ false, interruptedKey, failKey);
+    // Then the Evaluator#evaluate call returns an EvaluationResult that has no error for the
+    // interrupted SkyFunction.
+    assertWithMessage(result.toString()).that(result.hasError()).isTrue();
+    assertWithMessage(result.toString()).that(result.getError(failKey)).isNotNull();
+    assertWithMessage(result.toString()).that(result.getError(interruptedKey)).isNull();
+  }
+
+  @Test
   public void deleteValues() throws Exception {
     tester.getOrCreate("top").setComputedValue(CONCATENATE)
         .addDependency("d1").addDependency("d2").addDependency("d3");