Add catastrophe field to EvaluationResult so that callers can identify the cause of a catastrophic failure (this is distinct from a crash).
Also clean up catastrophe logic in ParallelEvaluator -- the catastrophic nature of an exception is important only if the build is keep_going, and only if the exception is catastrophic can we have an exception in the first place.
--
MOS_MIGRATED_REVID=111293164
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 c398f7a..64e1ff1 100644
--- a/src/main/java/com/google/devtools/build/skyframe/EvaluationResult.java
+++ b/src/main/java/com/google/devtools/build/skyframe/EvaluationResult.java
@@ -40,6 +40,7 @@
public class EvaluationResult<T extends SkyValue> {
private final boolean hasError;
+ @Nullable private final Exception catastrophe;
private final Map<SkyKey, T> resultMap;
private final Map<SkyKey, ErrorInfo> errorMap;
@@ -48,13 +49,18 @@
/**
* Constructor for the "completed" case. Used only by {@link Builder}.
*/
- private EvaluationResult(Map<SkyKey, T> result, Map<SkyKey, ErrorInfo> errorMap,
- boolean hasError, @Nullable WalkableGraph walkableGraph) {
+ private EvaluationResult(
+ Map<SkyKey, T> result,
+ Map<SkyKey, ErrorInfo> errorMap,
+ boolean hasError,
+ @Nullable Exception catastrophe,
+ @Nullable WalkableGraph walkableGraph) {
Preconditions.checkState(errorMap.isEmpty() || hasError,
"result=%s, errorMap=%s", result, errorMap);
this.resultMap = Preconditions.checkNotNull(result);
this.errorMap = Preconditions.checkNotNull(errorMap);
this.hasError = hasError;
+ this.catastrophe = catastrophe;
this.walkableGraph = walkableGraph;
}
@@ -75,6 +81,12 @@
return hasError;
}
+ /** @return catastrophic error encountered during evaluation, if any */
+ @Nullable
+ public Exception getCatastrophe() {
+ return catastrophe;
+ }
+
/**
* @return All successfully evaluated {@link SkyValue}s.
*/
@@ -152,6 +164,7 @@
private final Map<SkyKey, T> result = new HashMap<>();
private final Map<SkyKey, ErrorInfo> errors = new HashMap<>();
private boolean hasError = false;
+ @Nullable private Exception catastrophe = null;
private WalkableGraph walkableGraph = null;
/** Adds a value to the result. An error for this key must not already be present. */
@@ -184,11 +197,15 @@
}
public EvaluationResult<T> build() {
- return new EvaluationResult<>(result, errors, hasError, walkableGraph);
+ return new EvaluationResult<>(result, errors, hasError, catastrophe, walkableGraph);
}
public void setHasError(boolean hasError) {
this.hasError = hasError;
}
+
+ public void setCatastrophe(Exception catastrophe) {
+ this.catastrophe = catastrophe;
+ }
}
}
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 e2a6608..54b3cfd 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
@@ -1312,10 +1312,14 @@
// ErrorInfo could only be null if SchedulerException wrapped an InterruptedException, but
// that should have been propagated.
ErrorInfo errorInfo = Preconditions.checkNotNull(e.getErrorInfo(), errorKey);
- catastrophe = errorInfo.isCatastrophic();
- if (!catastrophe || !keepGoing) {
+ if (!keepGoing) {
bubbleErrorInfo = bubbleErrorUp(errorInfo, errorKey, skyKeys, visitor);
} else {
+ Preconditions.checkState(
+ errorInfo.isCatastrophic(),
+ "Scheduler exception only thrown for catastrophe in keep_going evaluation: %s",
+ e);
+ catastrophe = true;
// Bubbling the error up requires that graph edges are present for done nodes. This is not
// always the case in a keepGoing evaluation, since it is assumed that done nodes do not
// need to be traversed. In this case, we hope the caller is tolerant of a possibly empty
@@ -1507,10 +1511,17 @@
* {@code skyKeys} are known to be in the DONE state ({@code entry.isDone()} returns true).
*/
private <T extends SkyValue> EvaluationResult<T> constructResult(
- @Nullable ValueVisitor visitor, Iterable<SkyKey> skyKeys,
- Map<SkyKey, ValueWithMetadata> bubbleErrorInfo, boolean catastrophe) {
- Preconditions.checkState(!keepGoing || catastrophe || bubbleErrorInfo == null,
- "", skyKeys, bubbleErrorInfo);
+ @Nullable ValueVisitor visitor,
+ Iterable<SkyKey> skyKeys,
+ @Nullable Map<SkyKey, ValueWithMetadata> bubbleErrorInfo,
+ boolean catastrophe) {
+ Preconditions.checkState(
+ catastrophe == (keepGoing && bubbleErrorInfo != null),
+ "Catastrophe not consistent with keepGoing mode and bubbleErrorInfo: %s %s %s %s",
+ skyKeys,
+ catastrophe,
+ keepGoing,
+ bubbleErrorInfo);
EvaluationResult.Builder<T> result = EvaluationResult.builder();
List<SkyKey> cycleRoots = new ArrayList<>();
boolean hasError = false;
@@ -1556,6 +1567,20 @@
Preconditions.checkState(bubbleErrorInfo == null || hasError,
"If an error bubbled up, some top-level node must be in error", bubbleErrorInfo, skyKeys);
result.setHasError(hasError);
+ if (catastrophe) {
+ // We may not have a top-level node completed. Inform the caller of the catastrophic exception
+ // that shut down the evaluation so that it has some context.
+ ErrorInfo errorInfo =
+ Preconditions.checkNotNull(
+ Iterables.getOnlyElement(bubbleErrorInfo.values()).getErrorInfo(),
+ "bubbleErrorInfo should have contained element with errorInfo: %s",
+ bubbleErrorInfo);
+ Preconditions.checkState(
+ errorInfo.isCatastrophic(),
+ "bubbleErrorInfo should have contained element with catastrophe: %s",
+ bubbleErrorInfo);
+ result.setCatastrophe(errorInfo.getException());
+ }
return result.build();
}
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 55b3bb9..95f9e40 100644
--- a/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
@@ -556,25 +556,28 @@
SkyKey catastropheKey = GraphTester.toSkyKey("catastrophe");
SkyKey otherKey = GraphTester.toSkyKey("someKey");
- tester.getOrCreate(catastropheKey).setBuilder(new SkyFunction() {
- @Nullable
- @Override
- public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException {
- throw new SkyFunctionException(new SomeErrorException("bad"),
- Transience.PERSISTENT) {
- @Override
- public boolean isCatastrophic() {
- return true;
- }
- };
- }
+ final 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;
- }
- });
+ @Nullable
+ @Override
+ public String extractTag(SkyKey skyKey) {
+ return null;
+ }
+ });
tester.getOrCreate(otherKey).setBuilder(new SkyFunction() {
@Nullable
@@ -600,6 +603,7 @@
} else {
assertTrue(result.hasError());
assertThat(result.errorMap()).isEmpty();
+ assertThat(result.getCatastrophe()).isSameAs(catastrophe);
}
}
