Make TreeArtifactValue.visitTree() declare InterruptedException.

It does not throw it yet, but this also requires declaring that exception in a lot of places, so it's better to do this separately. Work towards bazelbuild#17009. RELNOTES: None. PiperOrigin-RevId: 523388258 Change-Id: Idf50f39f3cfcca83aa0a8ff5b5395f80dfa26b69
fweikert · May 25, 2023 · ff42504 · ff42504
1 parent 236bfe3
commit ff42504
Show file tree

Hide file tree

Showing 16 changed files with 110 additions and 51 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
@@ -575,7 +575,8 @@ public MiddlemanType getActionType() {
   }
 
   /** If the action might create directories as outputs this method must be called. */
-  protected void checkOutputsForDirectories(ActionExecutionContext actionExecutionContext) {
+  protected void checkOutputsForDirectories(ActionExecutionContext actionExecutionContext)
+      throws InterruptedException {
     FileArtifactValue metadata;
     for (Artifact output : getOutputs()) {
       MetadataHandler metadataHandler = actionExecutionContext.getMetadataHandler();

diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
@@ -192,7 +192,8 @@ private static boolean validateArtifacts(
       NestedSet<Artifact> actionInputs,
       MetadataHandler metadataHandler,
       boolean checkOutput,
-      @Nullable CachedOutputMetadata cachedOutputMetadata) {
+      @Nullable CachedOutputMetadata cachedOutputMetadata)
+      throws InterruptedException {
     Map<String, FileArtifactValue> mdMap = new HashMap<>();
     if (checkOutput) {
       for (Artifact artifact : action.getOutputs()) {
@@ -317,7 +318,8 @@ private CachedOutputMetadata(
   }
 
   private static CachedOutputMetadata loadCachedOutputMetadata(
-      Action action, ActionCache.Entry entry, MetadataHandler metadataHandler) {
+      Action action, ActionCache.Entry entry, MetadataHandler metadataHandler)
+      throws InterruptedException {
     Instant now = Instant.now();
     ImmutableMap.Builder<Artifact, RemoteFileArtifactValue> remoteFileMetadata =
         ImmutableMap.builder();
@@ -580,7 +582,7 @@ private static FileArtifactValue getInputMetadataOrConstant(
   }
 
   private static FileArtifactValue getOutputMetadataOrConstant(
-      MetadataHandler metadataHandler, Artifact artifact) throws IOException {
+      MetadataHandler metadataHandler, Artifact artifact) throws IOException, InterruptedException {
     FileArtifactValue metadata = metadataHandler.getOutputMetadata(artifact);
     return (metadata != null && artifact.isConstantMetadata())
         ? ConstantMetadataValue.INSTANCE
@@ -605,7 +607,7 @@ private static FileArtifactValue getInputMetadataMaybe(
   // should propagate the exception, because it is unexpected (e.g., bad file system state).
   @Nullable
   private static FileArtifactValue getOutputMetadataMaybe(
-      MetadataHandler metadataHandler, Artifact artifact) {
+      MetadataHandler metadataHandler, Artifact artifact) throws InterruptedException {
     try {
       return getOutputMetadataOrConstant(metadataHandler, artifact);
     } catch (IOException e) {
@@ -753,7 +755,8 @@ public List<Artifact> getCachedInputs(Action action, PackageRootResolver resolve
    * actions, it consults with the aggregated middleman digest computed here.
    */
   private void checkMiddlemanAction(
-      Action action, EventHandler handler, MetadataHandler metadataHandler) {
+      Action action, EventHandler handler, MetadataHandler metadataHandler)
+      throws InterruptedException {
     if (!cacheConfig.enabled()) {
       // Action cache is disabled, don't generate digests.
       return;

diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java
@@ -414,7 +414,7 @@ public void close() throws IOException {
    * it must already have been built.
    */
   public ActionExecutionContext withOutputsAsInputs(
-      Iterable<? extends ActionInput> additionalInputs) throws IOException {
+      Iterable<? extends ActionInput> additionalInputs) throws IOException, InterruptedException {
     ImmutableMap.Builder<ActionInput, FileArtifactValue> additionalInputMap =
         ImmutableMap.builder();
 

diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionInputPrefetcher.java b/src/main/java/com/google/devtools/build/lib/actions/ActionInputPrefetcher.java
@@ -27,7 +27,7 @@ public interface ActionInputPrefetcher {
    * either an input or an output artifact.
    */
   public interface MetadataSupplier {
-    FileArtifactValue getMetadata(ActionInput actionInput) throws IOException;
+    FileArtifactValue getMetadata(ActionInput actionInput) throws IOException, InterruptedException;
   }
 
   public static final ActionInputPrefetcher NONE =

diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java
@@ -47,7 +47,7 @@ public interface MetadataHandler extends MetadataProvider, MetadataInjector {
    * @return the artifact's digest or null the artifact is not a known output of the action
    * @throws IOException if the action input cannot be digested
    */
-  FileArtifactValue getOutputMetadata(ActionInput output) throws IOException;
+  FileArtifactValue getOutputMetadata(ActionInput output) throws IOException, InterruptedException;
 
   /** Sets digest for virtual artifacts (e.g. middlemen). {@code digest} must not be null. */
   void setDigestForVirtualArtifact(Artifact artifact, byte[] digest);
@@ -70,7 +70,8 @@ FileArtifactValue constructMetadataForDigest(
   ImmutableSet<TreeFileArtifact> getTreeArtifactChildren(SpecialArtifact treeArtifact);
 
   /** Retrieves the metadata for this tree artifact. Data should already be available. */
-  TreeArtifactValue getTreeArtifactValue(SpecialArtifact treeArtifact) throws IOException;
+  TreeArtifactValue getTreeArtifactValue(SpecialArtifact treeArtifact)
+      throws IOException, InterruptedException;
 
   /**
    * Marks an {@link Artifact} as intentionally omitted.

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java
@@ -315,7 +315,7 @@ protected void beforeExecute(ActionExecutionContext actionExecutionContext)
    */
   protected void afterExecute(
       ActionExecutionContext actionExecutionContext, List<SpawnResult> spawnResults)
-      throws ExecException {}
+      throws ExecException, InterruptedException {}
 
   @Override
   public final ActionResult execute(ActionExecutionContext actionExecutionContext)

diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java
@@ -326,7 +326,7 @@ private Completable prefetchFile(
       MetadataSupplier metadataSupplier,
       ActionInput input,
       Priority priority)
-      throws IOException {
+      throws IOException, InterruptedException {
     if (input instanceof VirtualActionInput) {
       prefetchVirtualActionInput((VirtualActionInput) input);
       return Completable.complete();
@@ -375,7 +375,7 @@ private Completable prefetchFile(
    */
   @Nullable
   private PathFragment maybeGetTreeRoot(ActionInput input, MetadataSupplier metadataSupplier)
-      throws IOException {
+      throws IOException, InterruptedException {
     if (!(input instanceof TreeFileArtifact)) {
       return null;
     }
@@ -400,7 +400,7 @@ private PathFragment maybeGetTreeRoot(ActionInput input, MetadataSupplier metada
   @Nullable
   private Symlink maybeGetSymlink(
       ActionInput input, FileArtifactValue metadata, MetadataSupplier metadataSupplier)
-      throws IOException {
+      throws IOException, InterruptedException {
     if (input instanceof TreeFileArtifact) {
       // Check whether the entire tree artifact should be prefetched into a separate location.
       SpecialArtifact treeArtifact = ((TreeFileArtifact) input).getParent();

diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/RxFutures.java b/src/main/java/com/google/devtools/build/lib/remote/util/RxFutures.java
@@ -213,7 +213,9 @@ public void onComplete() {
 
           @Override
           public void onError(Throwable e) {
-            if (e instanceof CancellationException) {
+            if (e instanceof InterruptedException) {
+              future.cancel(true);
+            } else if (e instanceof CancellationException) {
               future.cancel(true);
             } else {
               future.setException(e);

diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/RxUtils.java b/src/main/java/com/google/devtools/build/lib/remote/util/RxUtils.java
@@ -28,20 +28,29 @@ private RxUtils() {}
 
   /** Result of an I/O operation to remote cache. */
   public static class TransferResult {
-    private static final TransferResult OK = new TransferResult(null);
+    private static final TransferResult OK = new TransferResult(null, false);
+
+    private static final TransferResult INTERRUPTED = new TransferResult(null, true);
 
     public static TransferResult ok() {
       return OK;
     }
 
+    public static TransferResult interrupted() {
+      return INTERRUPTED;
+    }
+
     public static TransferResult error(IOException error) {
-      return new TransferResult(error);
+      return new TransferResult(error, false);
     }
 
     @Nullable private final IOException error;
 
-    TransferResult(@Nullable IOException error) {
+    private final boolean interrupted;
+
+    TransferResult(@Nullable IOException error, boolean interrupted) {
       this.error = error;
+      this.interrupted = interrupted;
     }
 
     /** Returns {@code true} if the operation succeed. */
@@ -54,6 +63,10 @@ public boolean isError() {
       return error != null;
     }
 
+    public boolean isInterrupted() {
+      return interrupted;
+    }
+
     /** Returns the IO error if the operation failed. */
     @Nullable
     public IOException getError() {
@@ -72,20 +85,28 @@ public static Single<TransferResult> toTransferResult(Completable completable) {
             error -> {
               if (error instanceof IOException) {
                 return Single.just(TransferResult.error((IOException) error));
+              } else if (error instanceof InterruptedException) {
+                return Single.just(TransferResult.interrupted());
+              } else {
+                return Single.error(error);
               }
-
-              return Single.error(error);
             });
   }
 
   private static class BulkTransferExceptionCollector {
     private BulkTransferException bulkTransferException;
+    private boolean interrupted = false;
 
     void onResult(TransferResult result) {
       if (result.isOk()) {
         return;
       }
 
+      if (result.isInterrupted()) {
+        interrupted = true;
+        return;
+      }
+
       IOException error = checkNotNull(result.getError());
       if (bulkTransferException == null) {
         bulkTransferException = new BulkTransferException();
@@ -99,6 +120,10 @@ Completable toCompletable() {
         return Completable.complete();
       }
 
+      if (interrupted) {
+        return Completable.error(new InterruptedException());
+      }
+
       return Completable.error(bulkTransferException);
     }
   }

diff --git a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleAction.java b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleAction.java
@@ -79,7 +79,8 @@ protected void beforeExecute(ActionExecutionContext actionExecutionContext) thro
 
   @Override
   protected void afterExecute(
-      ActionExecutionContext actionExecutionContext, List<SpawnResult> spawnResults) {
+      ActionExecutionContext actionExecutionContext, List<SpawnResult> spawnResults)
+      throws InterruptedException {
     checkOutputsForDirectories(actionExecutionContext);
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
@@ -254,7 +254,8 @@ public FileArtifactValue getInputMetadata(ActionInput actionInput) throws IOExce
 
   @Nullable
   @Override
-  public FileArtifactValue getOutputMetadata(ActionInput actionInput) throws IOException {
+  public FileArtifactValue getOutputMetadata(ActionInput actionInput)
+      throws IOException, InterruptedException {
     Artifact artifact = (Artifact) actionInput;
     FileArtifactValue value;
 
@@ -318,7 +319,8 @@ public void setDigestForVirtualArtifact(Artifact artifact, byte[] digest) {
   }
 
   @Override
-  public TreeArtifactValue getTreeArtifactValue(SpecialArtifact artifact) throws IOException {
+  public TreeArtifactValue getTreeArtifactValue(SpecialArtifact artifact)
+      throws IOException, InterruptedException {
     checkState(artifact.isTreeArtifact(), "%s is not a tree artifact", artifact);
 
     TreeArtifactValue value = store.getTreeArtifactData(artifact);
@@ -332,7 +334,7 @@ public TreeArtifactValue getTreeArtifactValue(SpecialArtifact artifact) throws I
   }
 
   private TreeArtifactValue constructTreeArtifactValueFromFilesystem(SpecialArtifact parent)
-      throws IOException {
+      throws IOException, InterruptedException {
     Path treeDir = artifactPathResolver.toPath(parent);
     boolean chmod = executionMode.get();
 

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java
@@ -344,6 +344,7 @@ && shouldCheckFile(knownModifiedOutputFiles, artifact)) {
         return;
       } catch (InterruptedException e) {
         logger.atInfo().log("Interrupted doing batch stat");
+        Thread.currentThread().interrupt();
         // We handle interrupt in the main thread.
         return;
       }
@@ -383,13 +384,20 @@ && shouldCheckFile(knownModifiedOutputFiles, artifact)) {
       for (Map.Entry<Artifact, Pair<SkyKey, ActionExecutionValue>> entry :
           treeArtifactsToKeyAndValue.entrySet()) {
         Artifact artifact = entry.getKey();
-        if (treeArtifactIsDirty(
-            entry.getKey(), entry.getValue().getSecond().getTreeArtifactValue(artifact))) {
-          // Count the changed directory as one "file".
-          // TODO(bazel-team): There are no tests for this codepath.
-          modifiedOutputsReceiver.reportModifiedOutputFile(
-              getBestEffortModifiedTime(artifact.getPath()), artifact);
-          dirtyKeys.add(entry.getValue().getFirst());
+        try {
+          if (treeArtifactIsDirty(
+              entry.getKey(), entry.getValue().getSecond().getTreeArtifactValue(artifact))) {
+            // Count the changed directory as one "file".
+            // TODO(bazel-team): There are no tests for this codepath.
+            modifiedOutputsReceiver.reportModifiedOutputFile(
+                getBestEffortModifiedTime(artifact.getPath()), artifact);
+            dirtyKeys.add(entry.getValue().getFirst());
+          }
+        } catch (InterruptedException e) {
+          logger.atInfo().log("Interrupted doing batch stat");
+          Thread.currentThread().interrupt();
+          // We handle interrupt in the main thread.
+          return;
         }
       }
     };
@@ -406,24 +414,33 @@ private Runnable outputStatJob(
     return new Runnable() {
       @Override
       public void run() {
-        for (Pair<SkyKey, ActionExecutionValue> keyAndValue : shard) {
-          ActionExecutionValue value = keyAndValue.getSecond();
-          if (value == null
-              || actionValueIsDirtyWithDirectSystemCalls(
-                  value,
-                  knownModifiedOutputFiles,
-                  sortedKnownModifiedOutputFiles,
-                  trustRemoteArtifacts,
-                  modifiedOutputsReceiver,
-                  now)) {
-            dirtyKeys.add(keyAndValue.getFirst());
+        try {
+          for (Pair<SkyKey, ActionExecutionValue> keyAndValue : shard) {
+            ActionExecutionValue value = keyAndValue.getSecond();
+            if (value == null
+                || actionValueIsDirtyWithDirectSystemCalls(
+                    value,
+                    knownModifiedOutputFiles,
+                    sortedKnownModifiedOutputFiles,
+                    trustRemoteArtifacts,
+                    modifiedOutputsReceiver,
+                    now)) {
+              dirtyKeys.add(keyAndValue.getFirst());
+            }
           }
+        } catch (InterruptedException e) {
+          // This code is called from getDirtyActionValues() and is running under an Executor. This
+          // means that getDirtyActionValues() will take care of house-keeping in case of an
+          // interrupt; all that matters is that we exit as quickly as possible.
+          logger.atInfo().log("Interrupted doing non-batch stat");
+          Thread.currentThread().interrupt();
         }
       }
     };
   }
 
-  private boolean treeArtifactIsDirty(Artifact artifact, TreeArtifactValue value) {
+  private boolean treeArtifactIsDirty(Artifact artifact, TreeArtifactValue value)
+      throws InterruptedException {
     Path path = artifact.getPath();
     if (path.isSymbolicLink()) {
       return true; // TreeArtifacts may not be symbolic links.
@@ -488,7 +505,8 @@ private boolean actionValueIsDirtyWithDirectSystemCalls(
       Supplier<NavigableSet<PathFragment>> sortedKnownModifiedOutputFiles,
       boolean trustRemoteArtifacts,
       ModifiedOutputsReceiver modifiedOutputsReceiver,
-      Instant now) {
+      Instant now)
+      throws InterruptedException {
     boolean isDirty = false;
     for (Map.Entry<Artifact, FileArtifactValue> entry : actionValue.getAllFileValues().entrySet()) {
       if (artifactIsDirtyWithDirectSystemCalls(

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
@@ -1473,7 +1473,8 @@ private boolean checkOutputs(
       Action action,
       MetadataHandler metadataHandler,
       @Nullable ImmutableList<FilesetOutputSymlink> filesetOutputSymlinksForMetrics,
-      boolean isActionCacheHitForMetrics) {
+      boolean isActionCacheHitForMetrics)
+      throws InterruptedException {
     boolean success = true;
     try (SilentCloseable c = profiler.profile(ProfilerTask.INFO, "checkOutputs")) {
       for (Artifact output : action.getOutputs()) {
@@ -1520,7 +1521,7 @@ private void addOutputToMetrics(
       @Nullable ImmutableList<FilesetOutputSymlink> filesetOutputSymlinks,
       boolean isActionCacheHit,
       Action actionForDebugging)
-      throws IOException {
+      throws IOException, InterruptedException {
     if (metadata == null) {
       BugReport.sendBugReport(
           new IllegalStateException(

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java
@@ -504,7 +504,8 @@ public interface TreeArtifactVisitor {
    * @throws IOException if there is any problem reading or validating outputs under the given tree
    *     artifact directory, or if {@link TreeArtifactVisitor#visit} throws {@link IOException}
    */
-  public static void visitTree(Path parentDir, TreeArtifactVisitor visitor) throws IOException {
+  public static void visitTree(Path parentDir, TreeArtifactVisitor visitor)
+      throws IOException, InterruptedException {
     visitTree(parentDir, PathFragment.EMPTY_FRAGMENT, checkNotNull(visitor));
   }