diff options
| -rw-r--r-- | src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java | 7 | ||||
| -rw-r--r-- | src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java | 55 |
2 files changed, 61 insertions, 1 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 e7674cd622..8cd502dd45 100644 --- a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java +++ b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java @@ -1017,8 +1017,13 @@ public final class ParallelEvaluator implements Evaluator { } catch (final SkyFunctionException builderException) { ReifiedSkyFunctionException reifiedBuilderException = new ReifiedSkyFunctionException(builderException, skyKey); + // In keep-going mode, we do not let SkyFunctions throw errors with missing deps -- we will + // restart them when their deps are done, so we can have a definitive error and definitive + // graph structure, thus avoiding non-determinism. It's completely reasonable for + // SkyFunctions to throw eagerly because they do not know if they are in keep-going mode. // Propagated transitive errors are treated the same as missing deps. - if (reifiedBuilderException.getRootCauseSkyKey().equals(skyKey)) { + if ((!keepGoing || !env.valuesMissing()) + && reifiedBuilderException.getRootCauseSkyKey().equals(skyKey)) { boolean shouldFailFast = !keepGoing || builderException.isCatastrophic(); if (shouldFailFast) { // After we commit this error to the graph but before the eval call completes with the diff --git a/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java index 56dcfb51f8..9c87572818 100644 --- a/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java +++ b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java @@ -1241,6 +1241,61 @@ public class ParallelEvaluatorTest { assertThat(error.getValue().getRootCauses()).containsExactly(errorKey); } + @Test + public void errorDepDoesntStopOtherDep() throws Exception { + graph = new InMemoryGraphImpl(); + final SkyKey errorKey = GraphTester.toSkyKey("error"); + tester.getOrCreate(errorKey).setHasError(true); + EvaluationResult<StringValue> result1 = eval(/*keepGoing=*/ true, ImmutableList.of(errorKey)); + assertThatEvaluationResult(result1).hasError(); + assertThatEvaluationResult(result1) + .hasErrorEntryForKeyThat(errorKey) + .hasExceptionThat() + .isNotNull(); + final SkyKey otherKey = GraphTester.toSkyKey("other"); + tester.getOrCreate(otherKey).setConstantValue(new StringValue("other")); + SkyKey topKey = GraphTester.toSkyKey("top"); + final Exception topException = new SomeErrorException("top exception"); + final AtomicInteger numComputes = new AtomicInteger(0); + tester + .getOrCreate(topKey) + .setBuilder( + new SkyFunction() { + @Nullable + @Override + public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException { + Map<SkyKey, ValueOrException<SomeErrorException>> values = + env.getValuesOrThrow( + ImmutableList.of(errorKey, otherKey), SomeErrorException.class); + if (numComputes.incrementAndGet() == 1) { + assertThat(env.valuesMissing()).isTrue(); + } else { + assertThat(numComputes.get()).isEqualTo(2); + assertThat(env.valuesMissing()).isFalse(); + } + try { + values.get(errorKey).get(); + throw new AssertionError("Should have thrown"); + } catch (SomeErrorException e) { + throw new SkyFunctionException(topException, Transience.PERSISTENT) {}; + } + } + + @Nullable + @Override + public String extractTag(SkyKey skyKey) { + return null; + } + }); + EvaluationResult<StringValue> result2 = eval(/*keepGoing=*/ true, ImmutableList.of(topKey)); + assertThatEvaluationResult(result2).hasError(); + assertThatEvaluationResult(result2) + .hasErrorEntryForKeyThat(topKey) + .hasExceptionThat() + .isSameAs(topException); + assertThat(numComputes.get()).isEqualTo(2); + } + /** * Make sure that multiple unfinished children can be cleared from a cycle value. */ |