From ff42504ad4ac4595b2a43feba0796d416f3d7f6e Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 11 Apr 2023 06:47:57 -0700 Subject: [PATCH] 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 #17009. RELNOTES: None. PiperOrigin-RevId: 523388258 Change-Id: Idf50f39f3cfcca83aa0a8ff5b5395f80dfa26b69 --- .../build/lib/actions/AbstractAction.java | 3 +- .../build/lib/actions/ActionCacheChecker.java | 13 +++-- .../lib/actions/ActionExecutionContext.java | 2 +- .../lib/actions/ActionInputPrefetcher.java | 2 +- .../lib/actions/cache/MetadataHandler.java | 5 +- .../lib/analysis/actions/SpawnAction.java | 2 +- .../remote/AbstractActionInputPrefetcher.java | 6 +- .../build/lib/remote/util/RxFutures.java | 4 +- .../build/lib/remote/util/RxUtils.java | 35 +++++++++-- .../lib/rules/genrule/GenRuleAction.java | 3 +- .../lib/skyframe/ActionMetadataHandler.java | 8 ++- .../lib/skyframe/FilesystemValueChecker.java | 58 ++++++++++++------- .../lib/skyframe/SkyframeActionExecutor.java | 5 +- .../build/lib/skyframe/TreeArtifactValue.java | 3 +- .../lib/actions/ActionCacheCheckerTest.java | 6 +- .../lib/actions/util/ActionsTestUtil.java | 6 +- 16 files changed, 110 insertions(+), 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 index 63d9b9517cc689..aa60baf0feafac 100644 --- 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 index aafb5a892b0fa1..f29c96f7bbb7a7 100644 --- 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 actionInputs, MetadataHandler metadataHandler, boolean checkOutput, - @Nullable CachedOutputMetadata cachedOutputMetadata) { + @Nullable CachedOutputMetadata cachedOutputMetadata) + throws InterruptedException { Map 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 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 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 index 28d84ec6d74d1f..31d1722589259d 100644 --- 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 additionalInputs) throws IOException { + Iterable additionalInputs) throws IOException, InterruptedException { ImmutableMap.Builder 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 index d0a70e753292ab..f41a2c26aaea08 100644 --- 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 index f237b4875a199e..87dc68f0592276 100644 --- 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 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 index 6d42148f5e1789..b9332e5e0ff096 100644 --- 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 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 index cb4c59753a3e06..cda5d56cdb6dc1 100644 --- 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 index f04838b3a85500..43e28baed00ef4 100644 --- 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 index bb669396ed2c3f..a12a975f7827fe 100644 --- 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 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 index bf58ad33c023bf..42d7f85546e7c9 100644 --- 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 spawnResults) { + ActionExecutionContext actionExecutionContext, List 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 index 057642e8e9b5cd..d3f3b7c78b7fd5 100644 --- 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 index 8a0b438503c1b5..3c763d80b80b36 100644 --- 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> 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 keyAndValue : shard) { - ActionExecutionValue value = keyAndValue.getSecond(); - if (value == null - || actionValueIsDirtyWithDirectSystemCalls( - value, - knownModifiedOutputFiles, - sortedKnownModifiedOutputFiles, - trustRemoteArtifacts, - modifiedOutputsReceiver, - now)) { - dirtyKeys.add(keyAndValue.getFirst()); + try { + for (Pair 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> sortedKnownModifiedOutputFiles, boolean trustRemoteArtifacts, ModifiedOutputsReceiver modifiedOutputsReceiver, - Instant now) { + Instant now) + throws InterruptedException { boolean isDirty = false; for (Map.Entry 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 index a5f6a0addb7c29..4420756abac3fb 100644 --- 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 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 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 index 9bb0a65af6fee4..484394f749a669 100644 --- 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)); } diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java index 68c3006733c7f1..9567081f12532b 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java @@ -1331,7 +1331,8 @@ public FileArtifactValue getInputMetadata(ActionInput input) throws IOException } @Override - public FileArtifactValue getOutputMetadata(ActionInput input) throws IOException { + public FileArtifactValue getOutputMetadata(ActionInput input) + throws IOException, InterruptedException { if (!(input instanceof Artifact)) { return null; } @@ -1353,7 +1354,8 @@ public FileArtifactValue getOutputMetadata(ActionInput input) throws IOException } @Override - public TreeArtifactValue getTreeArtifactValue(SpecialArtifact output) throws IOException { + public TreeArtifactValue getTreeArtifactValue(SpecialArtifact output) + throws IOException, InterruptedException { if (treeMetadata.containsKey(output)) { return treeMetadata.get(output); } diff --git a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java index ba9adb1c8eea1a..1e141b9b87919a 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java +++ b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java @@ -1062,7 +1062,8 @@ public FileArtifactValue getInputMetadata(ActionInput input) throws IOException } @Override - public FileArtifactValue getOutputMetadata(ActionInput input) throws IOException { + public FileArtifactValue getOutputMetadata(ActionInput input) + throws IOException, InterruptedException { throw new UnsupportedOperationException(); } @@ -1082,7 +1083,8 @@ public ImmutableSet getTreeArtifactChildren(SpecialArtifact tr } @Override - public TreeArtifactValue getTreeArtifactValue(SpecialArtifact treeArtifact) throws IOException { + public TreeArtifactValue getTreeArtifactValue(SpecialArtifact treeArtifact) + throws IOException, InterruptedException { throw new UnsupportedOperationException(); }