Prohibit constructing `ErrorInfo` that is directly transient but not transitively transient.
PiperOrigin-RevId: 416455536
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 141c47e..c2a754b 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ErrorInfo.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ErrorInfo.java
@@ -16,7 +16,6 @@
import com.google.common.base.MoreObjects;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
-import com.google.common.collect.Iterables;
import com.google.devtools.build.skyframe.SkyFunctionException.ReifiedSkyFunctionException;
import java.util.Collection;
import java.util.Objects;
@@ -48,7 +47,7 @@
ImmutableList.of(cycleInfo),
/*isDirectlyTransient=*/ false,
/*isTransitivelyTransient=*/ false,
- /* isCatastrophic= */ false);
+ /*isCatastrophic=*/ false);
}
/** Create an ErrorInfo from a collection of existing errors. */
@@ -64,11 +63,11 @@
for (ErrorInfo child : childErrors) {
if (firstException == null) {
// Arbitrarily pick the first error.
- firstException = child.getException();
+ firstException = child.exception;
}
cycleBuilder.addAll(CycleInfo.prepareCycles(currentValue, child.cycles));
- isTransitivelyTransient |= child.isTransitivelyTransient();
- isCatastrophic |= child.isCatastrophic();
+ isTransitivelyTransient |= child.isTransitivelyTransient;
+ isCatastrophic |= child.isCatastrophic;
}
return new ErrorInfo(
@@ -80,9 +79,7 @@
}
@Nullable private final Exception exception;
-
private final ImmutableList<CycleInfo> cycles;
-
private final boolean isDirectlyTransient;
private final boolean isTransitivelyTransient;
private final boolean isCatastrophic;
@@ -93,14 +90,19 @@
boolean isDirectlyTransient,
boolean isTransitivelyTransient,
boolean isCatastrophic) {
- Preconditions.checkState(exception != null || !Iterables.isEmpty(cycles),
- "At least one of exception and cycles must be non-null/empty, respectively");
-
this.exception = exception;
this.cycles = cycles;
this.isDirectlyTransient = isDirectlyTransient;
this.isTransitivelyTransient = isTransitivelyTransient;
this.isCatastrophic = isCatastrophic;
+ Preconditions.checkArgument(
+ exception != null || !cycles.isEmpty(),
+ "At least one of exception and cycles must be present",
+ this);
+ Preconditions.checkArgument(
+ !isDirectlyTransient || isTransitivelyTransient,
+ "Cannot be directly transient but not transitively transient",
+ this);
}
@Override
@@ -188,6 +190,9 @@
/**
* Returns true iff the error is directly transient, i.e. if there was a transient error
* encountered during the computation itself.
+ *
+ * <p>A return of {@code true} implies that {@link #isTransitivelyTransient()} is also {@code
+ * true}.
*/
public boolean isDirectlyTransient() {
return isDirectlyTransient;
diff --git a/src/test/java/com/google/devtools/build/skyframe/ErrorInfoTest.java b/src/test/java/com/google/devtools/build/skyframe/ErrorInfoTest.java
index b2e58f5..1537d8b 100644
--- a/src/test/java/com/google/devtools/build/skyframe/ErrorInfoTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/ErrorInfoTest.java
@@ -26,14 +26,13 @@
/** Tests for the non-trivial creation logic of {@link ErrorInfo}. */
@RunWith(JUnit4.class)
-public class ErrorInfoTest {
+public final class ErrorInfoTest {
/** Dummy SkyFunctionException implementation for the sake of testing. */
- private static class DummySkyFunctionException extends SkyFunctionException {
+ private static final class DummySkyFunctionException extends SkyFunctionException {
private final boolean isCatastrophic;
- public DummySkyFunctionException(Exception cause, boolean isTransient,
- boolean isCatastrophic) {
+ DummySkyFunctionException(Exception cause, boolean isTransient, boolean isCatastrophic) {
super(cause, isTransient ? Transience.TRANSIENT : Transience.PERSISTENT);
this.isCatastrophic = isCatastrophic;
}
@@ -138,14 +137,30 @@
}
@Test
- public void testCannotCreateErrorInfoWithoutExceptionOrCycle() {
- // Brittle, but confirms we failed for the right reason.
- IllegalStateException e =
+ public void cannotCreateErrorInfoWithoutExceptionOrCycle() {
+ Exception e =
assertThrows(
- IllegalStateException.class,
- () -> new ErrorInfo(/*exception=*/ null, ImmutableList.of(), false, false, false));
+ RuntimeException.class,
+ () ->
+ new ErrorInfo(
+ /*exception=*/ null, /*cycles=*/ ImmutableList.of(), false, false, false));
+ assertThat(e).hasMessageThat().contains("At least one of exception and cycles must be present");
+ }
+
+ @Test
+ public void cannotCreateErrorInfoWithDirectTransienceButNotTransitiveTransience() {
+ Exception e =
+ assertThrows(
+ RuntimeException.class,
+ () ->
+ new ErrorInfo(
+ new Exception(),
+ /*cycles=*/ ImmutableList.of(),
+ /*isDirectlyTransient=*/ true,
+ /*isTransitivelyTransient=*/ false,
+ /*isCatastrophic=*/ false));
assertThat(e)
.hasMessageThat()
- .isEqualTo("At least one of exception and cycles must be non-null/empty, respectively");
+ .contains("Cannot be directly transient but not transitively transient");
}
}
