diff options
5 files changed, 246 insertions, 61 deletions
diff --git a/src/main/java/com/google/devtools/build/skyframe/CycleInfo.java b/src/main/java/com/google/devtools/build/skyframe/CycleInfo.java index 9759122def..a410ec51ca 100644 --- a/src/main/java/com/google/devtools/build/skyframe/CycleInfo.java +++ b/src/main/java/com/google/devtools/build/skyframe/CycleInfo.java @@ -39,7 +39,7 @@ public class CycleInfo implements Serializable { this(ImmutableList.<SkyKey>of(), cycle); } - CycleInfo(Iterable<SkyKey> pathToCycle, Iterable<SkyKey> cycle) { + public CycleInfo(Iterable<SkyKey> pathToCycle, Iterable<SkyKey> cycle) { this.pathToCycle = ImmutableList.copyOf(pathToCycle); this.cycle = ImmutableList.copyOf(cycle); } diff --git a/src/main/java/com/google/devtools/build/skyframe/ErrorInfo.java b/src/main/java/com/google/devtools/build/skyframe/ErrorInfo.java index 210b8ed3f5..67c8da96b7 100644 --- a/src/main/java/com/google/devtools/build/skyframe/ErrorInfo.java +++ b/src/main/java/com/google/devtools/build/skyframe/ErrorInfo.java @@ -15,6 +15,7 @@ package com.google.devtools.build.skyframe; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Iterables; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; @@ -32,47 +33,37 @@ import javax.annotation.Nullable; * <p>This is intended only for use in alternative {@code MemoizingEvaluator} implementations. */ public class ErrorInfo implements Serializable { - /** The set of descendants of this value that failed to build. */ - // Non-final only to allow for serialization. - private transient NestedSet<SkyKey> rootCauses; - - /** - * An exception thrown upon a value's failure to build. The exception is used for reporting, and - * thus may ultimately be rethrown by the caller. As well, during a --nokeep_going evaluation, if - * an error value is encountered from an earlier --keep_going build, the exception to be thrown is - * taken from here. - */ - @Nullable private final Exception exception; - private final SkyKey rootCauseOfException; - private final Iterable<CycleInfo> cycles; - - private final boolean isTransient; - private final boolean isCatastrophic; - - public ErrorInfo(ReifiedSkyFunctionException builderException) { - this.rootCauseOfException = builderException.getRootCauseSkyKey(); - this.rootCauses = NestedSetBuilder.create(Order.STABLE_ORDER, rootCauseOfException); - this.exception = Preconditions.checkNotNull(builderException.getCause(), builderException); - this.cycles = ImmutableList.of(); - this.isTransient = builderException.isTransient(); - this.isCatastrophic = builderException.isCatastrophic(); + /** Create an ErrorInfo from a {@link ReifiedSkyFunctionException}. */ + public static ErrorInfo fromException(ReifiedSkyFunctionException skyFunctionException) { + SkyKey rootCauseSkyKey = skyFunctionException.getRootCauseSkyKey(); + Exception rootCauseException = skyFunctionException.getCause(); + return new ErrorInfo( + NestedSetBuilder.create(Order.STABLE_ORDER, rootCauseSkyKey), + Preconditions.checkNotNull(rootCauseException, "Cause null %s", rootCauseException), + rootCauseSkyKey, + /*cycles=*/ ImmutableList.<CycleInfo>of(), + skyFunctionException.isTransient(), + skyFunctionException.isCatastrophic()); } - ErrorInfo(CycleInfo cycleInfo) { - this.rootCauses = NestedSetBuilder.emptySet(Order.STABLE_ORDER); - this.exception = null; - this.rootCauseOfException = null; - this.cycles = ImmutableList.of(cycleInfo); - this.isTransient = false; - this.isCatastrophic = false; + /** Create an ErrorInfo from a {@link CycleInfo}. */ + static ErrorInfo fromCycle(CycleInfo cycleInfo) { + return new ErrorInfo( + /*rootCauses=*/ NestedSetBuilder.<SkyKey>emptySet(Order.STABLE_ORDER), + /*exception=*/ null, + /*rootCauseOfException=*/ null, + ImmutableList.of(cycleInfo), + /*isTransient=*/ false, + /*isCatostrophic=*/ false); } - public ErrorInfo(SkyKey currentValue, Collection<ErrorInfo> childErrors) { - Preconditions.checkNotNull(currentValue); - Preconditions.checkState(!childErrors.isEmpty(), - "Error value %s with no exception must depend on another error value", currentValue); - NestedSetBuilder<SkyKey> builder = NestedSetBuilder.stableOrder(); + /** Create an ErrorInfo from a collection of existing errors. */ + public static ErrorInfo fromChildErrors(SkyKey currentValue, Collection<ErrorInfo> childErrors) { + Preconditions.checkNotNull(currentValue, "currentValue must not be null"); + Preconditions.checkState(!childErrors.isEmpty(), "childErrors may not be empty"); + + NestedSetBuilder<SkyKey> rootCausesBuilder = NestedSetBuilder.stableOrder(); ImmutableList.Builder<CycleInfo> cycleBuilder = ImmutableList.builder(); Exception firstException = null; SkyKey firstChildKey = null; @@ -83,18 +74,48 @@ public class ErrorInfo implements Serializable { firstException = child.getException(); firstChildKey = child.getRootCauseOfException(); } - builder.addTransitive(child.rootCauses); + rootCausesBuilder.addTransitive(child.rootCauses); cycleBuilder.addAll(CycleInfo.prepareCycles(currentValue, child.cycles)); isCatastrophic |= child.isCatastrophic(); } - this.rootCauses = builder.build(); - this.exception = firstException; - this.rootCauseOfException = firstChildKey; - this.cycles = cycleBuilder.build(); - // Parent errors should not be transient -- we depend on the child's transience, if any, to - // force re-evaluation if necessary. - this.isTransient = false; - this.isCatastrophic = isCatastrophic; + + return new ErrorInfo( + rootCausesBuilder.build(), + firstException, + firstChildKey, + cycleBuilder.build(), + // Parent errors should not be transient -- we depend on the child's transience, if any, to + // force re-evaluation if necessary. + /*isTransient=*/ false, + isCatastrophic); + } + + // Non-final only to allow for serialization. + private transient NestedSet<SkyKey> rootCauses; + + @Nullable private final Exception exception; + private final SkyKey rootCauseOfException; + + private final ImmutableList<CycleInfo> cycles; + + private final boolean isTransient; + private final boolean isCatastrophic; + + public ErrorInfo(NestedSet<SkyKey> rootCauses, @Nullable Exception exception, + SkyKey rootCauseOfException, ImmutableList<CycleInfo> cycles, boolean isTransient, + boolean isCatostrophic) { + Preconditions.checkState(exception != null || !Iterables.isEmpty(cycles), + "At least one of exception and cycles must be non-null/empty, respectively"); + Preconditions.checkState((exception == null) == (rootCauseOfException == null), + "exception and rootCauseOfException must both be null or non-null, got %s %s", + exception, rootCauseOfException); + + this.rootCauses = rootCauses; + this.exception = exception; + this.rootCauseOfException = rootCauseOfException; + this.cycles = cycles; + this.isTransient = isTransient; + this.isCatastrophic = isCatostrophic; } @Override @@ -116,6 +137,10 @@ public class ErrorInfo implements Serializable { /** * The exception thrown when building a value. May be null if value's only error is depending * on a cycle. + * + * <p>The exception is used for reporting and thus may ultimately be rethrown by the caller. + * As well, during a --nokeep_going evaluation, if an error value is encountered from an earlier + * --keep_going build, the exception to be thrown is taken from here. */ @Nullable public Exception getException() { return exception; 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 e8de292b26..6c080f4b38 100644 --- a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java +++ b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java @@ -117,7 +117,7 @@ public final class ParallelEvaluator implements Evaluator { return state.getValue(); } } - + /** An general interface for {@link ParallelEvaluator} to receive objects of type {@code T}. */ public interface Receiver<T> { // TODO(dmarting): should we just make it a common object for all Bazel codebase? @@ -269,7 +269,7 @@ public final class ParallelEvaluator implements Evaluator { */ private void finalizeErrorInfo() { if (errorInfo == null && !childErrorInfos.isEmpty()) { - errorInfo = new ErrorInfo(skyKey, childErrorInfos); + errorInfo = ErrorInfo.fromChildErrors(skyKey, childErrorInfos); } } @@ -826,7 +826,7 @@ public final class ParallelEvaluator implements Evaluator { } registerNewlyDiscoveredDepsForDoneEntry(skyKey, state, env); - ErrorInfo errorInfo = new ErrorInfo(reifiedBuilderException); + ErrorInfo errorInfo = ErrorInfo.fromException(reifiedBuilderException); env.setError(errorInfo); env.commit(/*enqueueParents=*/keepGoing); if (!shouldFailFast) { @@ -1297,9 +1297,9 @@ public final class ParallelEvaluator implements Evaluator { ReifiedSkyFunctionException reifiedBuilderException = new ReifiedSkyFunctionException(builderException, parent); if (reifiedBuilderException.getRootCauseSkyKey().equals(parent)) { - error = new ErrorInfo(reifiedBuilderException); + error = ErrorInfo.fromException(reifiedBuilderException); bubbleErrorInfo.put(errorKey, - ValueWithMetadata.error(new ErrorInfo(errorKey, ImmutableSet.of(error)), + ValueWithMetadata.error(ErrorInfo.fromChildErrors(errorKey, ImmutableSet.of(error)), env.buildEvents(/*missingChildren=*/true))); continue; } @@ -1314,7 +1314,7 @@ public final class ParallelEvaluator implements Evaluator { } // Builder didn't throw an exception, so just propagate this one up. bubbleErrorInfo.put(errorKey, - ValueWithMetadata.error(new ErrorInfo(errorKey, ImmutableSet.of(error)), + ValueWithMetadata.error(ErrorInfo.fromChildErrors(errorKey, ImmutableSet.of(error)), env.buildEvents(/*missingChildren=*/true))); } @@ -1483,7 +1483,7 @@ public final class ParallelEvaluator implements Evaluator { "Value %s was not successfully evaluated, but had no child errors. ValueEntry: %s", key, entry); SkyFunctionEnvironment env = new SkyFunctionEnvironment(key, directDeps, visitor); - env.setError(new ErrorInfo(key, errorDeps)); + env.setError(ErrorInfo.fromChildErrors(key, errorDeps)); env.commit(/*enqueueParents=*/false); } @@ -1531,8 +1531,8 @@ public final class ParallelEvaluator implements Evaluator { getChildrenErrors(entry.getTemporaryDirectDeps(), /*unfinishedChild=*/cycleChild); CycleInfo cycleInfo = new CycleInfo(cycle); // Add in this cycle. - allErrors.add(new ErrorInfo(cycleInfo)); - env.setError(new ErrorInfo(key, allErrors)); + allErrors.add(ErrorInfo.fromCycle(cycleInfo)); + env.setError(ErrorInfo.fromChildErrors(key, allErrors)); env.commit(/*enqueueParents=*/false); continue; } else { @@ -1540,7 +1540,7 @@ public final class ParallelEvaluator implements Evaluator { // path) and return. Preconditions.checkState(graphPath.get(0).equals(root), "%s not reached from %s. ValueEntry: %s", key, root, entry); - return new ErrorInfo(new CycleInfo(graphPath.subList(0, cycleStart), cycle)); + return ErrorInfo.fromCycle(new CycleInfo(graphPath.subList(0, cycleStart), cycle)); } } diff --git a/src/test/java/com/google/devtools/build/skyframe/ErrorInfoTest.java b/src/test/java/com/google/devtools/build/skyframe/ErrorInfoTest.java new file mode 100644 index 0000000000..2a5db2d2be --- /dev/null +++ b/src/test/java/com/google/devtools/build/skyframe/ErrorInfoTest.java @@ -0,0 +1,160 @@ +// 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 static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; +import com.google.devtools.build.lib.collect.nestedset.Order; +import com.google.devtools.build.skyframe.SkyFunctionException.ReifiedSkyFunctionException; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +import java.io.IOException; + +/** Tests for the non-trivial creation logic of {@link ErrorInfo}. */ +@RunWith(JUnit4.class) +public class ErrorInfoTest { + + /** Dummy SkyFunctionException implementation for the sake of testing. */ + private static class DummySkyFunctionException extends SkyFunctionException { + private final boolean isCatastrophic; + + public DummySkyFunctionException(Exception cause, boolean isTransient, + boolean isCatastrophic) { + super(cause, isTransient ? Transience.TRANSIENT : Transience.PERSISTENT); + this.isCatastrophic = isCatastrophic; + } + + @Override + public boolean isCatastrophic() { + return isCatastrophic; + } + } + + @Test + public void testFromException() { + Exception exception = new IOException("ehhhhh"); + SkyKey causeOfException = new SkyKey(SkyFunctionName.create("CAUSE"), 1234); + DummySkyFunctionException dummyException = + new DummySkyFunctionException(exception, /*isTransient=*/ true, /*isCatastrophic=*/ false); + + ErrorInfo errorInfo = ErrorInfo.fromException( + new ReifiedSkyFunctionException(dummyException, causeOfException)); + + assertThat(errorInfo.getRootCauses()).containsExactly(causeOfException); + assertThat(errorInfo.getException()).isSameAs(exception); + assertThat(errorInfo.getRootCauseOfException()).isSameAs(causeOfException); + assertThat(errorInfo.getCycleInfo()).isEmpty(); + assertThat(errorInfo.isTransient()).isTrue(); + assertThat(errorInfo.isCatastrophic()).isFalse(); + } + + @Test + public void testFromCycle() { + CycleInfo cycle = new CycleInfo( + ImmutableList.of(new SkyKey(SkyFunctionName.create("PATH"), 1234)), + ImmutableList.of(new SkyKey(SkyFunctionName.create("CYCLE"), 4321))); + + ErrorInfo errorInfo = ErrorInfo.fromCycle(cycle); + + assertThat(errorInfo.getRootCauses()).isEmpty(); + assertThat(errorInfo.getException()).isNull(); + assertThat(errorInfo.getRootCauseOfException()).isNull(); + assertThat(errorInfo.isTransient()).isFalse(); + assertThat(errorInfo.isCatastrophic()).isFalse(); + } + + @Test + public void testFromChildErrors() { + CycleInfo cycle = new CycleInfo( + ImmutableList.of(new SkyKey(SkyFunctionName.create("PATH"), 1234)), + ImmutableList.of(new SkyKey(SkyFunctionName.create("CYCLE"), 4321))); + ErrorInfo cycleErrorInfo = ErrorInfo.fromCycle(cycle); + + Exception exception1 = new IOException("ehhhhh"); + SkyKey causeOfException1 = new SkyKey(SkyFunctionName.create("CAUSE1"), 1234); + DummySkyFunctionException dummyException1 = + new DummySkyFunctionException(exception1, /*isTransient=*/ true, /*isCatastrophic=*/ false); + ErrorInfo exceptionErrorInfo1 = ErrorInfo.fromException( + new ReifiedSkyFunctionException(dummyException1, causeOfException1)); + + // N.B this ErrorInfo will be catastrophic. + Exception exception2 = new IOException("blahhhhh"); + SkyKey causeOfException2 = new SkyKey(SkyFunctionName.create("CAUSE2"), 5678); + DummySkyFunctionException dummyException2 = + new DummySkyFunctionException(exception2, /*isTransient=*/ true, /*isCatastrophic=*/ true); + ErrorInfo exceptionErrorInfo2 = ErrorInfo.fromException( + new ReifiedSkyFunctionException(dummyException2, causeOfException2)); + + SkyKey currentKey = new SkyKey(SkyFunctionName.create("CURRENT"), 9876); + + ErrorInfo errorInfo = ErrorInfo.fromChildErrors( + currentKey, ImmutableList.of(cycleErrorInfo, exceptionErrorInfo1, exceptionErrorInfo2)); + + assertThat(errorInfo.getRootCauses()).containsExactly(causeOfException1, causeOfException2); + + // For simplicity we test the current implementation detail that we choose the first non-null + // (exception, cause) pair that we encounter. This isn't necessarily a requirement of the + // interface, but it makes the test convenient and is a way to document the current behavior. + assertThat(errorInfo.getException()).isSameAs(exception1); + assertThat(errorInfo.getRootCauseOfException()).isSameAs(causeOfException1); + + assertThat(errorInfo.getCycleInfo()).containsExactly( + new CycleInfo( + ImmutableList.of(currentKey, Iterables.getOnlyElement(cycle.getPathToCycle())), + cycle.getCycle())); + assertThat(errorInfo.isTransient()).isFalse(); + assertThat(errorInfo.isCatastrophic()).isTrue(); + } + + @Test + public void testCannotCreateErrorInfoWithoutExceptionOrCycle() { + try { + new ErrorInfo( + NestedSetBuilder.<SkyKey>emptySet(Order.COMPILE_ORDER), + /*exception=*/ null, + /*rootCauseOfException=*/ null, + ImmutableList.<CycleInfo>of(), + false, + false); + } catch (IllegalStateException e) { + // Brittle, but confirms we failed for the right reason. + assertThat(e) + .hasMessage("At least one of exception and cycles must be non-null/empty, respectively"); + } + } + + @Test + public void testCannotCreateErrorInfoWithExceptionButNoRootCause() { + try { + new ErrorInfo( + NestedSetBuilder.<SkyKey>emptySet(Order.COMPILE_ORDER), + new IOException("foo"), + /*rootCauseOfException=*/ null, + ImmutableList.<CycleInfo>of(), + false, + false); + } catch (IllegalStateException e) { + // Brittle, but confirms we failed for the right reason. + assertThat(e.getMessage()) + .startsWith("exception and rootCauseOfException must both be null or non-null"); + } + } +} + diff --git a/src/test/java/com/google/devtools/build/skyframe/InMemoryNodeEntryTest.java b/src/test/java/com/google/devtools/build/skyframe/InMemoryNodeEntryTest.java index ffd6a2c137..de1224a299 100644 --- a/src/test/java/com/google/devtools/build/skyframe/InMemoryNodeEntryTest.java +++ b/src/test/java/com/google/devtools/build/skyframe/InMemoryNodeEntryTest.java @@ -115,7 +115,7 @@ public class InMemoryNodeEntryTest { ReifiedSkyFunctionException exception = new ReifiedSkyFunctionException( new GenericFunctionException(new SomeErrorException("oops"), Transience.PERSISTENT), key("cause")); - ErrorInfo errorInfo = new ErrorInfo(exception); + ErrorInfo errorInfo = ErrorInfo.fromException(exception); assertThat(setValue(entry, /*value=*/null, errorInfo, /*graphVersion=*/0L)).isEmpty(); assertTrue(entry.isDone()); assertNull(entry.getValue()); @@ -129,7 +129,7 @@ public class InMemoryNodeEntryTest { ReifiedSkyFunctionException exception = new ReifiedSkyFunctionException( new GenericFunctionException(new SomeErrorException("oops"), Transience.PERSISTENT), key("cause")); - ErrorInfo errorInfo = new ErrorInfo(exception); + ErrorInfo errorInfo = ErrorInfo.fromException(exception); setValue(entry, new SkyValue() {}, errorInfo, /*graphVersion=*/0L); assertTrue(entry.isDone()); assertEquals(errorInfo, entry.getErrorInfo()); @@ -447,7 +447,7 @@ public class InMemoryNodeEntryTest { new GenericFunctionException(new SomeErrorException("oops"), Transience.PERSISTENT), key("cause")); assertThat(entry.markRebuildingAndGetAllRemainingDirtyDirectDeps()).isEmpty(); - setValue(entry, new IntegerValue(5), new ErrorInfo(exception), + setValue(entry, new IntegerValue(5), ErrorInfo.fromException(exception), /*graphVersion=*/1L); assertTrue(entry.isDone()); assertEquals("Version increments when setValue changes", new IntVersion(1), entry.getVersion()); @@ -463,7 +463,7 @@ public class InMemoryNodeEntryTest { ReifiedSkyFunctionException exception = new ReifiedSkyFunctionException( new GenericFunctionException(new SomeErrorException("oops"), Transience.PERSISTENT), key("cause")); - ErrorInfo errorInfo = new ErrorInfo(exception); + ErrorInfo errorInfo = ErrorInfo.fromException(exception); setValue(entry, /*value=*/null, errorInfo, /*graphVersion=*/0L); entry.markDirty(/*isChanged=*/false); entry.addReverseDepAndCheckIfDone(null); // Restart evaluation. @@ -522,7 +522,7 @@ public class InMemoryNodeEntryTest { ReifiedSkyFunctionException exception = new ReifiedSkyFunctionException( new GenericFunctionException(new SomeErrorException("oops"), Transience.PERSISTENT), key("key")); - setValue(entry, null, new ErrorInfo(exception), 0L); + setValue(entry, null, ErrorInfo.fromException(exception), 0L); entry.markDirty(/*isChanged=*/false); entry.addReverseDepAndCheckIfDone(null); // Restart evaluation. assertEquals(NodeEntry.DirtyState.CHECK_DEPENDENCIES, entry.getDirtyState()); |