When loading a package, if an interrupted exception is thrown but an error was already encountered, suppress the interrupted exception in favor of the package-specific error: the error may be the one needed for error bubbling, while the interruption could just mean that we were interrupted during error bubbling.
In the non-error-bubbling case, it's ~neutral to propagate the SkyFunction-specific exception instead of the InterruptedException: likely neither will be listened to, since the interrupt of a Skyframe thread indicates the threadpool is already shutting down.
PiperOrigin-RevId: 361889324
diff --git a/src/main/java/com/google/devtools/build/lib/packages/BUILD b/src/main/java/com/google/devtools/build/lib/packages/BUILD
index 10e79dd..61bfbcd 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/packages/BUILD
@@ -58,6 +58,7 @@
"//src/main/protobuf:build_java_proto",
"//src/main/protobuf:failure_details_java_proto",
"//third_party:auto_value",
+ "//third_party:flogger",
"//third_party:guava",
"//third_party:jsr305",
"//third_party/protobuf:protobuf_java",
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
index efcd7bf..a118989 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -20,6 +20,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
+import com.google.common.flogger.GoogleLogger;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.cmdline.RepositoryName;
@@ -89,6 +90,7 @@
* per client application.
*/
public final class PackageFactory {
+ private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
/** An extension to the global namespace of the BUILD language. */
// TODO(bazel-team): this should probably be renamed PackageFactory.RuntimeExtension
@@ -647,6 +649,16 @@
pkgContext.eventHandler.handle(
Package.error(null, ex.getMessageWithStack(), Code.STARLARK_EVAL_ERROR));
pkgBuilder.setContainsErrors();
+ } catch (InterruptedException ex) {
+ if (pkgContext.pkgBuilder.containsErrors()) {
+ // Suppress the interrupted exception: we have an error of our own to return.
+ Thread.currentThread().interrupt();
+ logger.atInfo().withCause(ex).log(
+ "Suppressing InterruptedException for Package %s because an error was also found",
+ pkgBuilder.getPackageIdentifier().getCanonicalForm());
+ } else {
+ throw ex;
+ }
}
pkgBuilder.setComputationSteps(thread.getExecutedSteps());
}
diff --git a/src/main/java/com/google/devtools/build/skyframe/SkyFunction.java b/src/main/java/com/google/devtools/build/skyframe/SkyFunction.java
index 0ba2590..bdbde01 100644
--- a/src/main/java/com/google/devtools/build/skyframe/SkyFunction.java
+++ b/src/main/java/com/google/devtools/build/skyframe/SkyFunction.java
@@ -260,7 +260,10 @@
* <p>Callers should prioritize their responsibility to detect and handle errors in the returned
* map over their responsibility to return {@code null} if values are missing. This is because
* in nokeep_going evaluations, an error from a low level dependency is given a chance to be
- * enriched by its reverse-dependencies, if possible.
+ * enriched by its reverse-dependencies, if possible. Callers should also prioritize throwing
+ * exceptions over checking for {@link InterruptedException}, since during the error-bubbling
+ * enrichment process, the SkyFunction is interrupted after it has received the exception to
+ * prevent it from doing too much unnecessary work.
*
* <p>Returns a map, {@code m}. For all {@code k} in {@code depKeys}, {@code m.get(k) != null}.
* For all {@code v} such that there is some {@code k} such that {@code m.get(k) == v}, the
@@ -368,7 +371,8 @@
* ks.contains(k)
* </ul>
*
- * <p>If this returns true, the {@link SkyFunction} must return {@code null}.
+ * <p>If this returns true, the {@link SkyFunction} must return {@code null} or throw a {@link
+ * SkyFunctionException} if it detected an error even with values missing.
*/
boolean valuesMissing();
@@ -381,7 +385,8 @@
/**
* A live view of deps known to have already been requested either through an earlier call to
* {@link SkyFunction#compute} or inferred during change pruning. Should return {@code null} if
- * unknown.
+ * unknown. Only for special use cases: do not use in general unless you know exactly what
+ * you're doing!
*/
@Nullable
default GroupedList<SkyKey> getTemporaryDirectDeps() {
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AnalysisWithIOExceptionsTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AnalysisWithIOExceptionsTest.java
index 03115e3..87ce6b1 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/AnalysisWithIOExceptionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/AnalysisWithIOExceptionsTest.java
@@ -20,8 +20,10 @@
import com.google.devtools.build.lib.vfs.DigestHashFunction;
import com.google.devtools.build.lib.vfs.FileStatus;
import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.ModifiedFileSet;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
import java.io.IOException;
import java.util.function.Function;
@@ -58,6 +60,24 @@
}
@Test
+ public void testIncrementalGlobIOException() throws Exception {
+ scratch.file("b/BUILD", "sh_library(name = 'b', deps= ['//a:a'])");
+ scratch.file(
+ "a/BUILD",
+ "sh_library(name = 'a', srcs = glob(['a.sh']))",
+ "sh_library(name = 'expensive', srcs = ['expensive.sh'])");
+ Path aShFile = scratch.file("a/a.sh");
+ update("//b:b");
+ skyframeExecutor.invalidateFilesUnderPathForTesting(
+ reporter,
+ ModifiedFileSet.builder().modify(aShFile.relativeTo(rootDirectory)).build(),
+ Root.fromPath(rootDirectory));
+ crashMessage = path -> path.toString().contains("a.sh") ? "bork" : null;
+ reporter.removeHandler(failFastHandler);
+ assertThrows(ViewCreationFailedException.class, () -> update("//b:b"));
+ }
+
+ @Test
public void testWorkspaceError() throws IOException {
scratch.file("a/BUILD");
crashMessage = path -> path.toString().contains("WORKSPACE") ? "bork" : null;
