From 9108f9b1df5e74692b9ec8a543aa2f2a2e0d2235 Mon Sep 17 00:00:00 2001 From: janakr Date: Thu, 11 Nov 2021 13:00:19 -0800 Subject: [PATCH] Stop requiring that a catastrophe remains a catastrophe when it's bubbled up. The salient characteristic of a catastrophe in Skyframe is that it shuts down a keep-going build. How it's transformed during error bubbling is not a concern. PiperOrigin-RevId: 409222485 --- .../AbstractExceptionalParallelEvaluator.java | 65 +++++--------- .../build/skyframe/EvaluationResult.java | 4 +- .../com/google/devtools/build/skyframe/BUILD | 1 + .../build/skyframe/ParallelEvaluatorTest.java | 89 +++++++++++++++---- 4 files changed, 97 insertions(+), 62 deletions(-) diff --git a/src/main/java/com/google/devtools/build/skyframe/AbstractExceptionalParallelEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/AbstractExceptionalParallelEvaluator.java index 5ad8afaf97f380..6cce52bd2ac4f4 100644 --- a/src/main/java/com/google/devtools/build/skyframe/AbstractExceptionalParallelEvaluator.java +++ b/src/main/java/com/google/devtools/build/skyframe/AbstractExceptionalParallelEvaluator.java @@ -19,7 +19,6 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.flogger.GoogleLogger; -import com.google.devtools.build.lib.bugreport.BugReport; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible; import com.google.devtools.build.lib.events.ExtendedEventHandler; @@ -597,8 +596,9 @@ EvaluationResult constructResult( bubbleErrorInfo); EvaluationResult.Builder result = EvaluationResult.builder(); List cycleRoots = new ArrayList<>(); - boolean nonCycleErrorFound = false; + boolean haveKeys = false; for (SkyKey skyKey : skyKeys) { + haveKeys = true; SkyValue unwrappedValue = maybeGetValueFromError( skyKey, graph.get(null, Reason.PRE_OR_POST_EVALUATION, skyKey), bubbleErrorInfo); @@ -618,7 +618,6 @@ EvaluationResult constructResult( Preconditions.checkState(value != null || errorInfo != null, skyKey); if (!evaluatorContext.keepGoing() && errorInfo != null) { // value will be null here unless the value was already built on a prior keepGoing build. - nonCycleErrorFound = true; result.addError(skyKey, errorInfo); continue; } @@ -626,7 +625,6 @@ EvaluationResult constructResult( // Note that we must be in the keepGoing case. Only make this value an error if it doesn't // have a value. The error shouldn't matter to the caller since the value succeeded after a // fashion. - nonCycleErrorFound = true; result.addError(skyKey, errorInfo); } else { result.addResult(skyKey, value); @@ -635,50 +633,35 @@ EvaluationResult constructResult( if (!cycleRoots.isEmpty()) { cycleDetector.checkForCycles(cycleRoots, result, evaluatorContext); } - if (catastrophe && bubbleErrorInfo != null && !result.hasCatastrophe()) { - // We may not have a top-level node completed. Inform the caller of at least one catastrophic - // exception that shut down the evaluation so that it has some context. - // TODO(b/159006108): Sometimes we get here and not every exception is catastrophic, so we - // alert when that happens. If we didn't need to guard against that case, we could simply - // take the last element of bubbleErrorInfo#values() and make that the catastrophe. - boolean catastropheFound = false; - @Nullable Exception nonCatastrophicExceptionForBugHandler = null; - for (ValueWithMetadata valueWithMetadata : bubbleErrorInfo.values()) { + if (haveKeys && result.isEmpty()) { + Preconditions.checkState( + catastrophe && bubbleErrorInfo != null, + "No top-level keys were present but we did not have catastrophic error bubbling: %s %s %s" + + " %s", + skyKeys, + catastrophe, + bubbleErrorInfo, + cycleRoots); + boolean exceptionFound = false; + for (ValueWithMetadata valueWithMetadata : + ImmutableList.copyOf(bubbleErrorInfo.values()).reverse()) { ErrorInfo errorInfo = Preconditions.checkNotNull( valueWithMetadata.getErrorInfo(), "bubbleErrorInfo should have contained element with errorInfo: %s", bubbleErrorInfo); - if (errorInfo.isCatastrophic()) { - if (!result.hasCatastrophe()) { - result.setCatastrophe(errorInfo.getException()); - } - catastropheFound = true; - } else { - // Alert for the known bug of a non-catastrophic exception. - BugReport.sendBugReport( - new IllegalStateException( - String.format( - "bubbleErrorInfo should have contained element with catastrophe: %s" - + " (bubbleErrorInfo: %s)", - valueWithMetadata, bubbleErrorInfo))); - if (errorInfo.getException() != null) { - nonCatastrophicExceptionForBugHandler = errorInfo.getException(); - } + if (errorInfo.getException() != null) { + result.setCatastrophe(errorInfo.getException()); + exceptionFound = true; + break; } } - if (!catastropheFound && !nonCycleErrorFound) { - Preconditions.checkNotNull( - nonCatastrophicExceptionForBugHandler, - "There were no exceptions in bubbleErrorInfo despite a catastrophic failure (%s)", - bubbleErrorInfo); - // Alert for the never-seen bug of *no* catastrophic exceptions. - BugReport.sendBugReport( - new IllegalStateException( - "No element in bubbleErrorInfo was catastrophic: " + bubbleErrorInfo, - nonCatastrophicExceptionForBugHandler)); - result.setCatastrophe(nonCatastrophicExceptionForBugHandler); - } + Preconditions.checkState( + exceptionFound, + "Evaluation of %s terminated early with no exception (%s %s %s)", + skyKeys, + bubbleErrorInfo, + result); } EvaluationResult builtResult = result.build(); Preconditions.checkState( diff --git a/src/main/java/com/google/devtools/build/skyframe/EvaluationResult.java b/src/main/java/com/google/devtools/build/skyframe/EvaluationResult.java index 065a4720e05cf6..29f5985a2bca3a 100644 --- a/src/main/java/com/google/devtools/build/skyframe/EvaluationResult.java +++ b/src/main/java/com/google/devtools/build/skyframe/EvaluationResult.java @@ -196,8 +196,8 @@ public Builder setCatastrophe(Exception catastrophe) { return this; } - boolean hasCatastrophe() { - return this.catastrophe != null; + boolean isEmpty() { + return this.result.isEmpty() && this.errors.isEmpty(); } } } diff --git a/src/test/java/com/google/devtools/build/skyframe/BUILD b/src/test/java/com/google/devtools/build/skyframe/BUILD index f93f763231eee1..29ae40cdc8e88c 100644 --- a/src/test/java/com/google/devtools/build/skyframe/BUILD +++ b/src/test/java/com/google/devtools/build/skyframe/BUILD @@ -72,6 +72,7 @@ java_library( "//third_party:junit4", "//third_party:mockito", "//third_party:truth", + "@com_google_testparameterinjector//:testparameterinjector", ], ) 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 1809bd6806c4c6..cf929a9c697d2c 100644 --- a/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java +++ b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java @@ -19,6 +19,7 @@ import static com.google.devtools.build.lib.testutil.EventIterableSubjectFactory.assertThatEvents; import static com.google.devtools.build.skyframe.EvaluationResultSubjectFactory.assertThatEvaluationResult; import static com.google.devtools.build.skyframe.GraphTester.CONCATENATE; +import static com.google.devtools.build.skyframe.GraphTester.skyKey; import static org.junit.Assert.assertThrows; import static org.junit.Assert.fail; @@ -52,6 +53,8 @@ import com.google.devtools.build.skyframe.NotifyingHelper.Listener; import com.google.devtools.build.skyframe.NotifyingHelper.Order; import com.google.devtools.build.skyframe.SkyFunctionException.Transience; +import com.google.testing.junit.testparameterinjector.TestParameter; +import com.google.testing.junit.testparameterinjector.TestParameterInjector; import java.util.ArrayList; import java.util.List; import java.util.Map; @@ -66,16 +69,14 @@ import java.util.function.Supplier; import javax.annotation.Nullable; import org.junit.After; +import org.junit.Assume; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; import org.mockito.Mockito; -/** - * Tests for {@link ParallelEvaluator}. - */ -@RunWith(JUnit4.class) +/** Tests for {@link ParallelEvaluator}. */ +@RunWith(TestParameterInjector.class) public class ParallelEvaluatorTest { private static final SkyFunctionName CHILD_TYPE = SkyFunctionName.createHermetic("child"); private static final SkyFunctionName PARENT_TYPE = SkyFunctionName.createHermetic("parent"); @@ -850,21 +851,10 @@ public void shouldBuildOneTarget() throws Exception { } @Test - public void catastropheHaltsBuild_KeepGoing_KeepEdges() throws Exception { - catastrophicBuild(true, true); - } - - @Test - public void catastropheHaltsBuild_KeepGoing_NoKeepEdges() throws Exception { - catastrophicBuild(true, false); - } - - @Test - public void catastropheInBuild_NoKeepGoing_KeepEdges() throws Exception { - catastrophicBuild(false, true); - } + public void catastrophicBuild(@TestParameter boolean keepGoing, @TestParameter boolean keepEdges) + throws Exception { + Assume.assumeTrue(keepGoing || keepEdges); - private void catastrophicBuild(boolean keepGoing, boolean keepEdges) throws Exception { graph = new InMemoryGraphImpl(keepEdges); SkyKey catastropheKey = GraphTester.toSkyKey("catastrophe"); @@ -949,6 +939,67 @@ public String extractTag(SkyKey skyKey) { assertThat(result.getCatastrophe()).isEqualTo(catastrophe); } + @Test + public void catastropheBubblesIntoNonCatastrophe() throws Exception { + graph = new InMemoryGraphImpl(); + SkyKey catastropheKey = GraphTester.toSkyKey("catastrophe"); + Exception catastrophe = new SomeErrorException("bad"); + tester + .getOrCreate(catastropheKey) + .setBuilder( + new SkyFunction() { + @Nullable + @Override + public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException { + throw new SkyFunctionException(catastrophe, Transience.PERSISTENT) { + @Override + public boolean isCatastrophic() { + return true; + } + }; + } + + @Nullable + @Override + public String extractTag(SkyKey skyKey) { + return null; + } + }); + SkyKey topKey = skyKey("top"); + tester + .getOrCreate(topKey) + .setBuilder( + new SkyFunction() { + @Nullable + @Override + public SkyValue compute(SkyKey skyKey, Environment env) + throws SkyFunctionException, InterruptedException { + try { + env.getValueOrThrow(catastropheKey, SomeErrorException.class); + } catch (SomeErrorException e) { + throw new SkyFunctionException( + new SomeErrorException("We got: " + e.getMessage()), Transience.PERSISTENT) { + @Override + public boolean isCatastrophic() { + return false; + } + }; + } + return null; + } + + @Nullable + @Override + public String extractTag(SkyKey skyKey) { + return null; + } + }); + EvaluationResult result = eval(/*keepGoing=*/ true, ImmutableList.of(topKey)); + + assertThat(result.getError(topKey).getException()).isInstanceOf(SomeErrorException.class); + assertThat(result.getError(topKey).getException()).hasMessageThat().isEqualTo("We got: bad"); + } + @Test public void incrementalCycleWithCatastropheAndFailedBubbleUp() throws Exception { SkyKey topKey = GraphTester.toSkyKey("top");