From be52530370a52d42b796f1e3192de03cf9e539fb Mon Sep 17 00:00:00 2001 From: janakr Date: Tue, 16 Nov 2021 14:45:58 -0800 Subject: [PATCH] Restrict the type of exception that can be thrown during target pattern parsing (and general target-oriented graph visitation) to QueryException. In practice, this is the only exception that can be thrown, but that fact is extremely non-obvious. We make it obvious by restricting the type of Exception that can be thrown to QueryExceptionMarkerInterface, an interface that is only non-trivially implemented by QueryException. PiperOrigin-RevId: 410355114 --- .../google/devtools/build/lib/cmdline/BUILD | 27 +++++++++ .../BatchCallback.java | 18 +++--- .../ParallelVisitor.java | 15 ++--- .../QueryExceptionMarkerInterface.java | 39 +++++++++++++ .../build/lib/cmdline/TargetPattern.java | 41 +++++++------- .../lib/cmdline/TargetPatternResolver.java | 56 ++++++++++--------- .../devtools/build/lib/concurrent/BUILD | 5 ++ .../google/devtools/build/lib/pkgcache/BUILD | 1 + .../pkgcache/RecursivePackageProvider.java | 7 +-- .../query2/AbstractSkyKeyParallelVisitor.java | 2 +- .../google/devtools/build/lib/query2/BUILD | 1 + .../lib/query2/ParallelVisitorUtils.java | 4 +- .../build/lib/query2/SkyQueryEnvironment.java | 2 +- .../devtools/build/lib/query2/engine/BUILD | 2 + .../build/lib/query2/engine/Callback.java | 2 +- .../lib/query2/engine/QueryException.java | 3 +- .../google/devtools/build/lib/skyframe/BUILD | 14 ++++- ...ronmentBackedRecursivePackageProvider.java | 5 +- .../GraphBackedRecursivePackageProvider.java | 9 ++- .../PackageIdentifierBatchingCallback.java | 7 +-- .../PrepareDepsOfPatternFunction.java | 9 +-- ...geProviderBackedTargetPatternResolver.java | 52 +++++++++-------- ...RecursivePkgValueRootPackageExtractor.java | 5 +- .../lib/skyframe/RootPackageExtractor.java | 5 +- ...mplePackageIdentifierBatchingCallback.java | 6 +- .../SkyframeTargetPatternEvaluator.java | 3 +- .../lib/skyframe/TargetPatternFunction.java | 44 +++++++-------- .../TraversalInfoRootPackageExtractor.java | 16 +++--- .../google/devtools/build/lib/cmdline/BUILD | 6 +- .../ParallelVisitorTest.java | 40 ++++++------- 30 files changed, 267 insertions(+), 179 deletions(-) rename src/main/java/com/google/devtools/build/lib/{concurrent => cmdline}/BatchCallback.java (72%) rename src/main/java/com/google/devtools/build/lib/{concurrent => cmdline}/ParallelVisitor.java (97%) create mode 100644 src/main/java/com/google/devtools/build/lib/cmdline/QueryExceptionMarkerInterface.java rename src/test/java/com/google/devtools/build/lib/{concurrent => cmdline}/ParallelVisitorTest.java (89%) diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/BUILD b/src/main/java/com/google/devtools/build/lib/cmdline/BUILD index e414ea004108c1..9861a47c415223 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/BUILD +++ b/src/main/java/com/google/devtools/build/lib/cmdline/BUILD @@ -23,7 +23,9 @@ java_library( exports = [":cmdline-primitives"], deps = [ ":LabelValidator", + ":batch_callback", ":cmdline-primitives", + ":query_exception_marker_interface", "//src/main/java/com/google/devtools/build/docgen/annot", "//src/main/java/com/google/devtools/build/lib/actions:commandline_item", "//src/main/java/com/google/devtools/build/lib/concurrent", @@ -72,6 +74,31 @@ java_library( ], ) +java_library( + name = "parallel_visitor", + srcs = ["ParallelVisitor.java"], + deps = [ + ":batch_callback", + ":query_exception_marker_interface", + "//src/main/java/com/google/devtools/build/lib/concurrent", + "//third_party:guava", + ], +) + +java_library( + name = "batch_callback", + srcs = ["BatchCallback.java"], + deps = [ + ":query_exception_marker_interface", + "//src/main/java/com/google/devtools/build/lib/concurrent:thread_safety", + ], +) + +java_library( + name = "query_exception_marker_interface", + srcs = ["QueryExceptionMarkerInterface.java"], +) + # LabelValidator provides validation of Bazel build labels. # This is a public API. # TODO(adonovan): unsplit the lib.cmdline Java package by moving this logic to a subpackage. diff --git a/src/main/java/com/google/devtools/build/lib/concurrent/BatchCallback.java b/src/main/java/com/google/devtools/build/lib/cmdline/BatchCallback.java similarity index 72% rename from src/main/java/com/google/devtools/build/lib/concurrent/BatchCallback.java rename to src/main/java/com/google/devtools/build/lib/cmdline/BatchCallback.java index 7fdf0438d670af..1144b02e5f6172 100644 --- a/src/main/java/com/google/devtools/build/lib/concurrent/BatchCallback.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/BatchCallback.java @@ -11,17 +11,17 @@ // 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.lib.concurrent; +package com.google.devtools.build.lib.cmdline; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; /** - * Callback to be invoked when part of a result has been computed. Allows a client interested in - * the result to process it as it is computed, for instance by streaming it, if it is too big to - * fit in memory. + * Callback to be invoked when part of a result has been computed. Allows a client interested in the + * result to process it as it is computed, for instance by streaming it, if it is too big to fit in + * memory. */ @ThreadSafe -public interface BatchCallback { +public interface BatchCallback { /** * Called when part of a result has been computed. * @@ -33,8 +33,12 @@ public interface BatchCallback { */ void process(Iterable partialResult) throws E, InterruptedException; - /** {@link BatchCallback} that does precisely nothing. */ - class NullCallback implements BatchCallback { + /** {@link BatchCallback} that doesn't throw. */ + interface SafeBatchCallback + extends BatchCallback {} + + /** {@link SafeBatchCallback} that does precisely nothing. */ + class NullCallback implements SafeBatchCallback { private static final NullCallback INSTANCE = new NullCallback<>(); @Override diff --git a/src/main/java/com/google/devtools/build/lib/concurrent/ParallelVisitor.java b/src/main/java/com/google/devtools/build/lib/cmdline/ParallelVisitor.java similarity index 97% rename from src/main/java/com/google/devtools/build/lib/concurrent/ParallelVisitor.java rename to src/main/java/com/google/devtools/build/lib/cmdline/ParallelVisitor.java index 64d25891543283..78ac1517c5d564 100644 --- a/src/main/java/com/google/devtools/build/lib/concurrent/ParallelVisitor.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/ParallelVisitor.java @@ -11,12 +11,15 @@ // 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.lib.concurrent; +package com.google.devtools.build.lib.cmdline; import com.google.common.base.Preconditions; import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.concurrent.AbstractQueueVisitor; +import com.google.devtools.build.lib.concurrent.ErrorClassifier; +import com.google.devtools.build.lib.concurrent.QuiescingExecutor; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import java.util.ArrayList; import java.util.Collection; @@ -47,7 +50,7 @@ public abstract class ParallelVisitor< VisitKeyT, OutputKeyT, OutputResultT, - ExceptionT extends Exception, + ExceptionT extends Exception & QueryExceptionMarkerInterface, CallbackT extends BatchCallback> { protected final CallbackT callback; protected final Class exceptionClass; @@ -142,7 +145,7 @@ public interface Factory< VisitKeyT, OutputKeyT, OutputResultT, - ExceptionT extends Exception, + ExceptionT extends Exception & QueryExceptionMarkerInterface, CallbackT extends BatchCallback> { ParallelVisitor create(); } @@ -166,12 +169,6 @@ public void onVisitTaskCompleted() {} protected abstract Iterable outputKeysToOutputValues( Iterable targetKeys) throws ExceptionT, InterruptedException; - /** - * Suitable exception type to use with {@link ParallelVisitor} when no checked exception is - * appropriate. - */ - public static final class UnusedException extends RuntimeException {} - /** An object to hold keys to visit and keys ready for processing. */ protected final class Visit { private final Iterable keysToUseForResult; diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/QueryExceptionMarkerInterface.java b/src/main/java/com/google/devtools/build/lib/cmdline/QueryExceptionMarkerInterface.java new file mode 100644 index 00000000000000..c988f14ad6578a --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/cmdline/QueryExceptionMarkerInterface.java @@ -0,0 +1,39 @@ +// Copyright 2021 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.lib.cmdline; + +/** + * Marker interface indicating either a {@link + * com.google.devtools.build.lib.query2.engine.QueryException} or a {@link MarkerRuntimeException}. + * Used with a generic type to indicate that a method can optionally throw {@code QueryException} if + * the caller passes {@code QueryException.class} as a parameter to the method. + * + *

The only outside implementation of this interface is {@link + * com.google.devtools.build.lib.query2.engine.QueryException}. Do not implement or extend! + * + *

Used to narrow a generic type like {@code E extends Exception} to {@code E extends Exception & + * QueryExceptionMarkerInterface}, guaranteeing that the method will only throw {@link + * com.google.devtools.build.lib.query2.engine.QueryException} if any exception of type E is thrown. + * Because {@code E} will appear in the "throws" clause of a method, it must extend {@link + * Exception}. + */ +@SuppressWarnings("InterfaceWithOnlyStatics") +public interface QueryExceptionMarkerInterface { + /** + * Marker class indicating that a given method does not throw QueryException. Pass {@code + * MarkerRuntimeException.class} as a parameter. + */ + class MarkerRuntimeException extends RuntimeException implements QueryExceptionMarkerInterface {} +} diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java b/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java index 492ebad3ea4666..ba41c108933ae1 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java @@ -29,7 +29,6 @@ import com.google.common.util.concurrent.ListeningExecutorService; import com.google.devtools.build.lib.cmdline.LabelValidator.BadLabelException; import com.google.devtools.build.lib.cmdline.LabelValidator.PackageAndTarget; -import com.google.devtools.build.lib.concurrent.BatchCallback; import com.google.devtools.build.lib.server.FailureDetails.TargetPatterns; import com.google.devtools.build.lib.server.FailureDetails.TargetPatterns.Code; import com.google.devtools.build.lib.supplier.InterruptibleSupplier; @@ -146,7 +145,7 @@ public String getOriginalPattern() { * excludedSubdirectories} is nonempty and this pattern does not have type {@code * Type.TARGETS_BELOW_DIRECTORY}. */ - public abstract void eval( + public abstract void eval( TargetPatternResolver resolver, InterruptibleSupplier> ignoredSubdirectories, ImmutableSet excludedSubdirectories, @@ -162,12 +161,13 @@ public abstract void eval( * ExecutionException}, the cause will be an instance of either {@link TargetParsingException} or * the given {@code exceptionClass}. */ - public final ListenableFuture evalAdaptedForAsync( - TargetPatternResolver resolver, - InterruptibleSupplier> ignoredSubdirectories, - ImmutableSet excludedSubdirectories, - BatchCallback callback, - Class exceptionClass) { + public final + ListenableFuture evalAdaptedForAsync( + TargetPatternResolver resolver, + InterruptibleSupplier> ignoredSubdirectories, + ImmutableSet excludedSubdirectories, + BatchCallback callback, + Class exceptionClass) { try { eval(resolver, ignoredSubdirectories, excludedSubdirectories, callback, exceptionClass); return Futures.immediateFuture(null); @@ -191,7 +191,7 @@ public final ListenableFuture evalAdaptedForAsync * ExecutionException}, the cause will be an instance of either {@link TargetParsingException} or * the given {@code exceptionClass}. */ - public ListenableFuture evalAsync( + public ListenableFuture evalAsync( TargetPatternResolver resolver, InterruptibleSupplier> ignoredSubdirectories, ImmutableSet excludedSubdirectories, @@ -258,7 +258,7 @@ private SingleTarget(String targetName, PackageIdentifier directory, String orig } @Override - public void eval( + public void eval( TargetPatternResolver resolver, InterruptibleSupplier> ignoredSubdirectories, ImmutableSet excludedSubdirectories, @@ -320,7 +320,7 @@ private InterpretPathAsTarget(String path, String originalPattern) { } @Override - public void eval( + public void eval( TargetPatternResolver resolver, InterruptibleSupplier> ignoredSubdirectories, ImmutableSet excludedSubdirectories, @@ -418,7 +418,7 @@ private TargetsInPackage( } @Override - public void eval( + public void eval( TargetPatternResolver resolver, InterruptibleSupplier> ignoredSubdirectories, ImmutableSet excludedSubdirectories, @@ -544,7 +544,7 @@ private TargetsBelowDirectory( } @Override - public void eval( + public void eval( TargetPatternResolver resolver, InterruptibleSupplier> ignoredSubdirectories, ImmutableSet excludedSubdirectories, @@ -573,13 +573,14 @@ public void eval( } @Override - public ListenableFuture evalAsync( - TargetPatternResolver resolver, - InterruptibleSupplier> ignoredSubdirectories, - ImmutableSet excludedSubdirectories, - BatchCallback callback, - Class exceptionClass, - ListeningExecutorService executor) { + public + ListenableFuture evalAsync( + TargetPatternResolver resolver, + InterruptibleSupplier> ignoredSubdirectories, + ImmutableSet excludedSubdirectories, + BatchCallback callback, + Class exceptionClass, + ListeningExecutorService executor) { Preconditions.checkState( !excludedSubdirectories.contains(directory.getPackageFragment()), "Fully excluded target pattern %s should have already been filtered out (%s)", diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/TargetPatternResolver.java b/src/main/java/com/google/devtools/build/lib/cmdline/TargetPatternResolver.java index 639f6b3a87eeeb..f1c81df33f9765 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/TargetPatternResolver.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/TargetPatternResolver.java @@ -14,11 +14,13 @@ package com.google.devtools.build.lib.cmdline; +import static com.google.common.util.concurrent.Futures.immediateCancelledFuture; +import static com.google.common.util.concurrent.Futures.immediateFailedFuture; +import static com.google.common.util.concurrent.Futures.immediateVoidFuture; + import com.google.common.collect.ImmutableSet; -import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.ListeningExecutorService; -import com.google.devtools.build.lib.concurrent.BatchCallback; import com.google.devtools.build.lib.vfs.PathFragment; import java.util.Collection; @@ -88,32 +90,34 @@ public abstract Collection getTargetsInPackage( * @param exceptionClass The class type of the parameterized exception. * @throws TargetParsingException under implementation-specific failure conditions */ - public abstract void findTargetsBeneathDirectory( - RepositoryName repository, - String originalPattern, - String directory, - boolean rulesOnly, - ImmutableSet forbiddenSubdirectories, - ImmutableSet excludedSubdirectories, - BatchCallback callback, - Class exceptionClass) - throws TargetParsingException, E, InterruptedException; + public abstract + void findTargetsBeneathDirectory( + RepositoryName repository, + String originalPattern, + String directory, + boolean rulesOnly, + ImmutableSet forbiddenSubdirectories, + ImmutableSet excludedSubdirectories, + BatchCallback callback, + Class exceptionClass) + throws TargetParsingException, E, InterruptedException; /** * Async version of {@link #findTargetsBeneathDirectory} * *

Default implementation is synchronous. */ - public ListenableFuture findTargetsBeneathDirectoryAsync( - RepositoryName repository, - String originalPattern, - String directory, - boolean rulesOnly, - ImmutableSet forbiddenSubdirectories, - ImmutableSet excludedSubdirectories, - BatchCallback callback, - Class exceptionClass, - ListeningExecutorService executor) { + public + ListenableFuture findTargetsBeneathDirectoryAsync( + RepositoryName repository, + String originalPattern, + String directory, + boolean rulesOnly, + ImmutableSet forbiddenSubdirectories, + ImmutableSet excludedSubdirectories, + BatchCallback callback, + Class exceptionClass, + ListeningExecutorService executor) { try { findTargetsBeneathDirectory( repository, @@ -124,14 +128,14 @@ public ListenableFuture findTargetsBeneathDirectoryA excludedSubdirectories, callback, exceptionClass); - return Futures.immediateFuture(null); + return immediateVoidFuture(); } catch (TargetParsingException e) { - return Futures.immediateFailedFuture(e); + return immediateFailedFuture(e); } catch (InterruptedException e) { - return Futures.immediateCancelledFuture(); + return immediateCancelledFuture(); } catch (Exception e) { if (exceptionClass.isInstance(e)) { - return Futures.immediateFailedFuture(e); + return immediateFailedFuture(e); } throw new IllegalStateException(e); } diff --git a/src/main/java/com/google/devtools/build/lib/concurrent/BUILD b/src/main/java/com/google/devtools/build/lib/concurrent/BUILD index b3700690437ec7..e5a8e3a0094fb6 100644 --- a/src/main/java/com/google/devtools/build/lib/concurrent/BUILD +++ b/src/main/java/com/google/devtools/build/lib/concurrent/BUILD @@ -18,3 +18,8 @@ java_library( "//third_party:jsr305", ], ) + +java_library( + name = "thread_safety", + srcs = ["ThreadSafety.java"], +) diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/BUILD b/src/main/java/com/google/devtools/build/lib/pkgcache/BUILD index cf2dee9d992b6a..37f7e7ebc69aa2 100644 --- a/src/main/java/com/google/devtools/build/lib/pkgcache/BUILD +++ b/src/main/java/com/google/devtools/build/lib/pkgcache/BUILD @@ -27,6 +27,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/cmdline:LabelValidator", + "//src/main/java/com/google/devtools/build/lib/cmdline:batch_callback", "//src/main/java/com/google/devtools/build/lib/collect/compacthashset", "//src/main/java/com/google/devtools/build/lib/concurrent", "//src/main/java/com/google/devtools/build/lib/events", diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/RecursivePackageProvider.java b/src/main/java/com/google/devtools/build/lib/pkgcache/RecursivePackageProvider.java index 4df9df50adff63..94de4cda1326c9 100644 --- a/src/main/java/com/google/devtools/build/lib/pkgcache/RecursivePackageProvider.java +++ b/src/main/java/com/google/devtools/build/lib/pkgcache/RecursivePackageProvider.java @@ -14,11 +14,10 @@ package com.google.devtools.build.lib.pkgcache; import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.cmdline.BatchCallback.SafeBatchCallback; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.cmdline.RepositoryName; -import com.google.devtools.build.lib.concurrent.BatchCallback; -import com.google.devtools.build.lib.concurrent.ParallelVisitor.UnusedException; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.packages.NoSuchPackageException; import com.google.devtools.build.lib.packages.NoSuchTargetException; @@ -55,7 +54,7 @@ public interface RecursivePackageProvider extends PackageProvider { * SkyKey}s that are created during the traversal, instead filtered out later */ void streamPackagesUnderDirectory( - BatchCallback results, + SafeBatchCallback results, ExtendedEventHandler eventHandler, RepositoryName repository, PathFragment directory, @@ -121,7 +120,7 @@ public Target getTarget(ExtendedEventHandler eventHandler, Label label) @Override public void streamPackagesUnderDirectory( - BatchCallback results, + SafeBatchCallback results, ExtendedEventHandler eventHandler, RepositoryName repository, PathFragment directory, diff --git a/src/main/java/com/google/devtools/build/lib/query2/AbstractSkyKeyParallelVisitor.java b/src/main/java/com/google/devtools/build/lib/query2/AbstractSkyKeyParallelVisitor.java index 5138a82de10e64..8fba9b1f874bfe 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/AbstractSkyKeyParallelVisitor.java +++ b/src/main/java/com/google/devtools/build/lib/query2/AbstractSkyKeyParallelVisitor.java @@ -13,7 +13,7 @@ // limitations under the License. package com.google.devtools.build.lib.query2; -import com.google.devtools.build.lib.concurrent.ParallelVisitor; +import com.google.devtools.build.lib.cmdline.ParallelVisitor; import com.google.devtools.build.lib.query2.ParallelVisitorUtils.ParallelQueryVisitor; import com.google.devtools.build.lib.query2.engine.Callback; import com.google.devtools.build.lib.query2.engine.QueryException; diff --git a/src/main/java/com/google/devtools/build/lib/query2/BUILD b/src/main/java/com/google/devtools/build/lib/query2/BUILD index b6aa812b07bf56..ceb4b3958ec0f4 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/BUILD +++ b/src/main/java/com/google/devtools/build/lib/query2/BUILD @@ -55,6 +55,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto", "//src/main/java/com/google/devtools/build/lib/causes", "//src/main/java/com/google/devtools/build/lib/cmdline", + "//src/main/java/com/google/devtools/build/lib/cmdline:parallel_visitor", "//src/main/java/com/google/devtools/build/lib/collect/compacthashset", "//src/main/java/com/google/devtools/build/lib/collect/nestedset", "//src/main/java/com/google/devtools/build/lib/concurrent", diff --git a/src/main/java/com/google/devtools/build/lib/query2/ParallelVisitorUtils.java b/src/main/java/com/google/devtools/build/lib/query2/ParallelVisitorUtils.java index c0077234b6a966..038232be686471 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/ParallelVisitorUtils.java +++ b/src/main/java/com/google/devtools/build/lib/query2/ParallelVisitorUtils.java @@ -14,9 +14,9 @@ package com.google.devtools.build.lib.query2; import com.google.common.util.concurrent.ThreadFactoryBuilder; +import com.google.devtools.build.lib.cmdline.ParallelVisitor; +import com.google.devtools.build.lib.cmdline.ParallelVisitor.Factory; import com.google.devtools.build.lib.concurrent.BlockingStack; -import com.google.devtools.build.lib.concurrent.ParallelVisitor; -import com.google.devtools.build.lib.concurrent.ParallelVisitor.Factory; import com.google.devtools.build.lib.packages.Target; import com.google.devtools.build.lib.query2.engine.Callback; import com.google.devtools.build.lib.query2.engine.QueryException; diff --git a/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java index c9247a84becf7f..b706e2ab7d0b08 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java @@ -40,6 +40,7 @@ import com.google.common.util.concurrent.ThreadFactoryBuilder; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.PackageIdentifier; +import com.google.devtools.build.lib.cmdline.ParallelVisitor.VisitTaskStatusCallback; import com.google.devtools.build.lib.cmdline.SignedTargetPattern; import com.google.devtools.build.lib.cmdline.TargetParsingException; import com.google.devtools.build.lib.cmdline.TargetPattern; @@ -47,7 +48,6 @@ import com.google.devtools.build.lib.collect.compacthashset.CompactHashSet; import com.google.devtools.build.lib.concurrent.BlockingStack; import com.google.devtools.build.lib.concurrent.MultisetSemaphore; -import com.google.devtools.build.lib.concurrent.ParallelVisitor.VisitTaskStatusCallback; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.events.DelegatingEventHandler; import com.google.devtools.build.lib.events.Event; diff --git a/src/main/java/com/google/devtools/build/lib/query2/engine/BUILD b/src/main/java/com/google/devtools/build/lib/query2/engine/BUILD index ce3b371d4323f8..c70b8e020b76e9 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/engine/BUILD +++ b/src/main/java/com/google/devtools/build/lib/query2/engine/BUILD @@ -13,6 +13,8 @@ java_library( srcs = glob(["*.java"]), deps = [ "//src/main/java/com/google/devtools/build/lib/cmdline", + "//src/main/java/com/google/devtools/build/lib/cmdline:batch_callback", + "//src/main/java/com/google/devtools/build/lib/cmdline:query_exception_marker_interface", "//src/main/java/com/google/devtools/build/lib/collect/compacthashset", "//src/main/java/com/google/devtools/build/lib/concurrent", "//src/main/java/com/google/devtools/build/lib/graph", diff --git a/src/main/java/com/google/devtools/build/lib/query2/engine/Callback.java b/src/main/java/com/google/devtools/build/lib/query2/engine/Callback.java index cf4524f61a9127..0beb457e7f1b33 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/engine/Callback.java +++ b/src/main/java/com/google/devtools/build/lib/query2/engine/Callback.java @@ -13,7 +13,7 @@ // limitations under the License. package com.google.devtools.build.lib.query2.engine; -import com.google.devtools.build.lib.concurrent.BatchCallback; +import com.google.devtools.build.lib.cmdline.BatchCallback; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; /** diff --git a/src/main/java/com/google/devtools/build/lib/query2/engine/QueryException.java b/src/main/java/com/google/devtools/build/lib/query2/engine/QueryException.java index 162c5f1510fe34..68ffbf000ef36a 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/engine/QueryException.java +++ b/src/main/java/com/google/devtools/build/lib/query2/engine/QueryException.java @@ -14,13 +14,14 @@ package com.google.devtools.build.lib.query2.engine; import com.google.common.base.Preconditions; +import com.google.devtools.build.lib.cmdline.QueryExceptionMarkerInterface; import com.google.devtools.build.lib.server.FailureDetails.ActionQuery; import com.google.devtools.build.lib.server.FailureDetails.ConfigurableQuery; import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; import com.google.devtools.build.lib.server.FailureDetails.Query; /** Exception indicating a failure in Blaze query, aquery, or cquery. */ -public class QueryException extends Exception { +public class QueryException extends Exception implements QueryExceptionMarkerInterface { /** Returns a better error message for the query. */ static String describeFailedQuery(QueryException e, QueryExpression toplevel) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index 30cc36f7791695..325bf5300ea8c3 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -286,6 +286,8 @@ java_library( "//src/main/java/com/google/devtools/build/lib/causes", "//src/main/java/com/google/devtools/build/lib/clock", "//src/main/java/com/google/devtools/build/lib/cmdline", + "//src/main/java/com/google/devtools/build/lib/cmdline:batch_callback", + "//src/main/java/com/google/devtools/build/lib/cmdline:query_exception_marker_interface", "//src/main/java/com/google/devtools/build/lib/collect/compacthashset", "//src/main/java/com/google/devtools/build/lib/collect/nestedset", "//src/main/java/com/google/devtools/build/lib/concurrent", @@ -1502,6 +1504,7 @@ java_library( ":package_value", ":root_package_extractor", "//src/main/java/com/google/devtools/build/lib/cmdline", + "//src/main/java/com/google/devtools/build/lib/cmdline:batch_callback", "//src/main/java/com/google/devtools/build/lib/concurrent", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/io:inconsistent_filesystem_exception", @@ -1685,6 +1688,7 @@ java_library( ], deps = [ "//src/main/java/com/google/devtools/build/lib/cmdline", + "//src/main/java/com/google/devtools/build/lib/cmdline:batch_callback", "//src/main/java/com/google/devtools/build/lib/concurrent", "//third_party:guava", "//third_party:jsr305", @@ -2096,6 +2100,8 @@ java_library( deps = [ ":package_identifier_batching_callback", "//src/main/java/com/google/devtools/build/lib/cmdline", + "//src/main/java/com/google/devtools/build/lib/cmdline:batch_callback", + "//src/main/java/com/google/devtools/build/lib/cmdline:query_exception_marker_interface", "//src/main/java/com/google/devtools/build/lib/concurrent", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/packages", @@ -2177,7 +2183,7 @@ java_library( ":recursive_pkg_value", ":root_package_extractor", "//src/main/java/com/google/devtools/build/lib/cmdline", - "//src/main/java/com/google/devtools/build/lib/concurrent", + "//src/main/java/com/google/devtools/build/lib/cmdline:batch_callback", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/query2/engine", "//src/main/java/com/google/devtools/build/lib/vfs", @@ -2225,7 +2231,7 @@ java_library( srcs = ["RootPackageExtractor.java"], deps = [ "//src/main/java/com/google/devtools/build/lib/cmdline", - "//src/main/java/com/google/devtools/build/lib/concurrent", + "//src/main/java/com/google/devtools/build/lib/cmdline:batch_callback", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/query2/engine", "//src/main/java/com/google/devtools/build/lib/vfs", @@ -2656,7 +2662,9 @@ java_library( ":recursive_package_provider_backed_target_pattern_resolver", ":root_package_extractor", "//src/main/java/com/google/devtools/build/lib/cmdline", - "//src/main/java/com/google/devtools/build/lib/concurrent", + "//src/main/java/com/google/devtools/build/lib/cmdline:batch_callback", + "//src/main/java/com/google/devtools/build/lib/cmdline:parallel_visitor", + "//src/main/java/com/google/devtools/build/lib/cmdline:query_exception_marker_interface", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java b/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java index 9c24dfbe743dc2..cd3dcd30555523 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java @@ -18,10 +18,9 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.cmdline.BatchCallback.SafeBatchCallback; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.cmdline.RepositoryName; -import com.google.devtools.build.lib.concurrent.BatchCallback; -import com.google.devtools.build.lib.concurrent.ParallelVisitor.UnusedException; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.io.InconsistentFilesystemException; @@ -142,7 +141,7 @@ public boolean isPackage(ExtendedEventHandler eventHandler, PackageIdentifier pa @Override public void streamPackagesUnderDirectory( - BatchCallback results, + SafeBatchCallback results, ExtendedEventHandler eventHandler, RepositoryName repository, PathFragment directory, diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java b/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java index fec12cec8b2e32..0f9fdc1ee2f74a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java @@ -22,12 +22,11 @@ import com.google.common.collect.Sets; import com.google.common.collect.Sets.SetView; import com.google.common.flogger.GoogleLogger; +import com.google.devtools.build.lib.cmdline.BatchCallback.SafeBatchCallback; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.cmdline.TargetPattern; import com.google.devtools.build.lib.cmdline.TargetPattern.TargetsBelowDirectory; -import com.google.devtools.build.lib.concurrent.BatchCallback; -import com.google.devtools.build.lib.concurrent.ParallelVisitor.UnusedException; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.ExtendedEventHandler; @@ -59,8 +58,8 @@ public final class GraphBackedRecursivePackageProvider extends AbstractRecursive * Helper interface for clients of GraphBackedRecursivePackageProvider to indicate what universe * packages should be resolved in. * - *

Client can either specify a fixed set of target patterns (using {@link #of()}), or specify - * that all targets are valid (using {@link #all()}). + *

Client can either specify a fixed set of target patterns (using {@link #of}), or specify + * that all targets are valid (using {@link #all}). */ public interface UniverseTargetPattern { ImmutableList patterns(); @@ -240,7 +239,7 @@ private ImmutableList checkValidDirectoryAndGetRoots( @Override public void streamPackagesUnderDirectory( - BatchCallback results, + SafeBatchCallback results, ExtendedEventHandler eventHandler, RepositoryName repository, PathFragment directory, diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageIdentifierBatchingCallback.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageIdentifierBatchingCallback.java index daaac76f48e758..2bf7e9f17638d7 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageIdentifierBatchingCallback.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageIdentifierBatchingCallback.java @@ -13,9 +13,8 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe; +import com.google.devtools.build.lib.cmdline.BatchCallback.SafeBatchCallback; import com.google.devtools.build.lib.cmdline.PackageIdentifier; -import com.google.devtools.build.lib.concurrent.BatchCallback; -import com.google.devtools.build.lib.concurrent.ParallelVisitor.UnusedException; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; /** @@ -28,12 +27,12 @@ */ @ThreadSafe public interface PackageIdentifierBatchingCallback - extends BatchCallback, AutoCloseable { + extends SafeBatchCallback, AutoCloseable { void close() throws InterruptedException; /** Factory for {@link PackageIdentifierBatchingCallback}. */ interface Factory { PackageIdentifierBatchingCallback create( - BatchCallback batchResults, int maxBatchSize); + SafeBatchCallback batchResults, int maxBatchSize); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java index b72f3832fe009d..be7986dc2aeff2 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java @@ -16,15 +16,16 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.cmdline.BatchCallback; +import com.google.devtools.build.lib.cmdline.BatchCallback.NullCallback; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.PackageIdentifier; +import com.google.devtools.build.lib.cmdline.QueryExceptionMarkerInterface; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.cmdline.ResolvedTargets; import com.google.devtools.build.lib.cmdline.TargetParsingException; import com.google.devtools.build.lib.cmdline.TargetPattern; import com.google.devtools.build.lib.cmdline.TargetPatternResolver; -import com.google.devtools.build.lib.concurrent.BatchCallback; -import com.google.devtools.build.lib.concurrent.BatchCallback.NullCallback; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.packages.NoSuchPackageException; import com.google.devtools.build.lib.packages.NoSuchTargetException; @@ -116,7 +117,7 @@ public SkyValue compute(SkyKey key, Environment env) () -> repositoryIgnoredPatterns, ImmutableSet.of(), NullCallback.instance(), - RuntimeException.class); + QueryExceptionMarkerInterface.MarkerRuntimeException.class); } catch (TargetParsingException e) { throw new PrepareDepsOfPatternFunctionException(e); } catch (MissingDepException e) { @@ -249,7 +250,7 @@ public String getTargetKind(Void target) { } @Override - public void findTargetsBeneathDirectory( + public void findTargetsBeneathDirectory( RepositoryName repository, String originalPattern, String directory, diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java index 9e52de427ca9b3..e9abf1b721d6f9 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java @@ -24,15 +24,16 @@ import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.ListeningExecutorService; import com.google.common.util.concurrent.MoreExecutors; +import com.google.devtools.build.lib.cmdline.BatchCallback; +import com.google.devtools.build.lib.cmdline.BatchCallback.SafeBatchCallback; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.PackageIdentifier; +import com.google.devtools.build.lib.cmdline.QueryExceptionMarkerInterface; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.cmdline.ResolvedTargets; import com.google.devtools.build.lib.cmdline.TargetParsingException; import com.google.devtools.build.lib.cmdline.TargetPatternResolver; -import com.google.devtools.build.lib.concurrent.BatchCallback; import com.google.devtools.build.lib.concurrent.MultisetSemaphore; -import com.google.devtools.build.lib.concurrent.ParallelVisitor.UnusedException; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.ExtendedEventHandler; @@ -178,7 +179,7 @@ public String getTargetKind(Target target) { } @Override - public void findTargetsBeneathDirectory( + public void findTargetsBeneathDirectory( final RepositoryName repository, final String originalPattern, String directory, @@ -206,16 +207,17 @@ public void findTargetsBeneathDirectory( } @Override - public ListenableFuture findTargetsBeneathDirectoryAsync( - RepositoryName repository, - String originalPattern, - String directory, - boolean rulesOnly, - ImmutableSet forbiddenSubdirectories, - ImmutableSet excludedSubdirectories, - BatchCallback callback, - Class exceptionClass, - ListeningExecutorService executor) { + public + ListenableFuture findTargetsBeneathDirectoryAsync( + RepositoryName repository, + String originalPattern, + String directory, + boolean rulesOnly, + ImmutableSet forbiddenSubdirectories, + ImmutableSet excludedSubdirectories, + BatchCallback callback, + Class exceptionClass, + ListeningExecutorService executor) { return findTargetsBeneathDirectoryAsyncImpl( repository, originalPattern, @@ -227,20 +229,21 @@ public ListenableFuture findTargetsBeneathDirectoryA executor); } - private ListenableFuture findTargetsBeneathDirectoryAsyncImpl( - RepositoryName repository, - String pattern, - String directory, - boolean rulesOnly, - ImmutableSet forbiddenSubdirectories, - ImmutableSet excludedSubdirectories, - BatchCallback callback, - ListeningExecutorService executor) { + private + ListenableFuture findTargetsBeneathDirectoryAsyncImpl( + RepositoryName repository, + String pattern, + String directory, + boolean rulesOnly, + ImmutableSet forbiddenSubdirectories, + ImmutableSet excludedSubdirectories, + BatchCallback callback, + ListeningExecutorService executor) { FilteringPolicy actualPolicy = rulesOnly ? FilteringPolicies.and(FilteringPolicies.RULES_ONLY, policy) : policy; ArrayList> futures = new ArrayList<>(); - BatchCallback getPackageTargetsCallback = + SafeBatchCallback getPackageTargetsCallback = (pkgIdBatch) -> futures.add( executor.submit( @@ -278,7 +281,8 @@ private ListenableFuture findTargetsBeneathDirectory * Task to get all matching targets in the given packages, filter them, and pass them to the * target batch callback. */ - private class GetTargetsInPackagesTask implements Callable { + private class GetTargetsInPackagesTask + implements Callable { private final Iterable packageIdentifiers; private final String originalPattern; diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgValueRootPackageExtractor.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgValueRootPackageExtractor.java index bc0e7b5f21a136..47b27ea1960c50 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgValueRootPackageExtractor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgValueRootPackageExtractor.java @@ -16,10 +16,9 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.cmdline.BatchCallback.SafeBatchCallback; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.cmdline.RepositoryName; -import com.google.devtools.build.lib.concurrent.BatchCallback; -import com.google.devtools.build.lib.concurrent.ParallelVisitor.UnusedException; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.query2.engine.QueryException; import com.google.devtools.build.lib.server.FailureDetails.Query.Code; @@ -34,7 +33,7 @@ public class RecursivePkgValueRootPackageExtractor implements RootPackageExtract @Override public void streamPackagesFromRoots( - BatchCallback results, + SafeBatchCallback results, WalkableGraph graph, List roots, ExtendedEventHandler eventHandler, diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RootPackageExtractor.java b/src/main/java/com/google/devtools/build/lib/skyframe/RootPackageExtractor.java index d77e938c6f42d2..758546a0d14590 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/RootPackageExtractor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RootPackageExtractor.java @@ -14,10 +14,9 @@ package com.google.devtools.build.lib.skyframe; import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.cmdline.BatchCallback.SafeBatchCallback; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.cmdline.RepositoryName; -import com.google.devtools.build.lib.concurrent.BatchCallback; -import com.google.devtools.build.lib.concurrent.ParallelVisitor.UnusedException; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.query2.engine.QueryException; import com.google.devtools.build.lib.vfs.PathFragment; @@ -46,7 +45,7 @@ public interface RootPackageExtractor { * searched exhaustively */ void streamPackagesFromRoots( - BatchCallback results, + SafeBatchCallback results, WalkableGraph graph, List roots, ExtendedEventHandler eventHandler, diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SimplePackageIdentifierBatchingCallback.java b/src/main/java/com/google/devtools/build/lib/skyframe/SimplePackageIdentifierBatchingCallback.java index 80ce1162ac66b3..fe93e680865dd8 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SimplePackageIdentifierBatchingCallback.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SimplePackageIdentifierBatchingCallback.java @@ -15,8 +15,6 @@ import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.cmdline.PackageIdentifier; -import com.google.devtools.build.lib.concurrent.BatchCallback; -import com.google.devtools.build.lib.concurrent.ParallelVisitor.UnusedException; import javax.annotation.concurrent.GuardedBy; /** @@ -25,7 +23,7 @@ * smaller than the others. */ public class SimplePackageIdentifierBatchingCallback implements PackageIdentifierBatchingCallback { - private final BatchCallback batchResults; + private final SafeBatchCallback batchResults; private final int batchSize; @GuardedBy("this") @@ -35,7 +33,7 @@ public class SimplePackageIdentifierBatchingCallback implements PackageIdentifie private int bufferedPackageIds; public SimplePackageIdentifierBatchingCallback( - BatchCallback batchResults, int batchSize) { + SafeBatchCallback batchResults, int batchSize) { this.batchResults = batchResults; this.batchSize = batchSize; reset(); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java index 081dfd9217d24b..e6437fd6c9c2fe 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java @@ -17,6 +17,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.cmdline.QueryExceptionMarkerInterface; import com.google.devtools.build.lib.cmdline.ResolvedTargets; import com.google.devtools.build.lib.cmdline.SignedTargetPattern; import com.google.devtools.build.lib.cmdline.TargetParsingException; @@ -265,7 +266,7 @@ public Collection process( partialResult instanceof Collection ? (Collection) partialResult : ImmutableSet.copyOf(partialResult)), - TargetParsingException.class); + QueryExceptionMarkerInterface.MarkerRuntimeException.class); return result.get(); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java index 4a3d93ec3cf6fe..01bd4626e81e1a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java @@ -15,12 +15,12 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.cmdline.BatchCallback.SafeBatchCallback; import com.google.devtools.build.lib.cmdline.Label; -import com.google.devtools.build.lib.cmdline.PackageIdentifier; +import com.google.devtools.build.lib.cmdline.QueryExceptionMarkerInterface; import com.google.devtools.build.lib.cmdline.ResolvedTargets; import com.google.devtools.build.lib.cmdline.TargetParsingException; import com.google.devtools.build.lib.cmdline.TargetPattern; -import com.google.devtools.build.lib.concurrent.BatchCallback; import com.google.devtools.build.lib.concurrent.MultisetSemaphore; import com.google.devtools.build.lib.packages.OutputFile; import com.google.devtools.build.lib.packages.Target; @@ -66,30 +66,26 @@ public SkyValue compute(SkyKey key, Environment env) throws TargetPatternFunctio provider, env.getListener(), patternKey.getPolicy(), - MultisetSemaphore.unbounded(), + MultisetSemaphore.unbounded(), SimplePackageIdentifierBatchingCallback::new); ImmutableSet excludedSubdirectories = patternKey.getExcludedSubdirectories(); ResolvedTargets.Builder resolvedTargetsBuilder = ResolvedTargets.builder(); - BatchCallback callback = - new BatchCallback() { - @Override - public void process(Iterable partialResult) { - for (Target target : partialResult) { - // TODO(b/156899726): This will go away as soon as we remove implicit outputs from - // cc_library completely. The only - // downside to doing this is that implicit outputs won't be listed when doing - // somepackage:* for the handful of cases still on the allowlist. This is only a - // google internal problem and the scale of it is acceptable in the short term - // while cleaning up the allowlist. - if (target instanceof OutputFile - && ((OutputFile) target) - .getGeneratingRule() - .getRuleClass() - .equals("cc_library")) { - continue; - } - resolvedTargetsBuilder.add(target); + SafeBatchCallback callback = + partialResult -> { + for (Target target : partialResult) { + // TODO(b/156899726): This will go away as soon as we remove implicit outputs from + // cc_library completely. The only downside to doing this is that implicit outputs + // won't be listed when doing somepackage:* for the handful of cases still on the + // allowlist. This is only a Google-internal problem and the scale of it is + // acceptable in the short term while cleaning up the allowlist. + if (target instanceof OutputFile + && ((OutputFile) target) + .getGeneratingRule() + .getRuleClass() + .equals("cc_library")) { + continue; } + resolvedTargetsBuilder.add(target); } }; parsedPattern.eval( @@ -97,9 +93,7 @@ public void process(Iterable partialResult) { () -> ignoredPatterns, excludedSubdirectories, callback, - // The exception type here has to match the one on the BatchCallback. Since the callback - // defined above never throws, the exact type here is not really relevant. - RuntimeException.class); + QueryExceptionMarkerInterface.MarkerRuntimeException.class); if (provider.encounteredPackageErrors()) { resolvedTargetsBuilder.setError(); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TraversalInfoRootPackageExtractor.java b/src/main/java/com/google/devtools/build/lib/skyframe/TraversalInfoRootPackageExtractor.java index 85f5d05f464054..978a3820ef0da6 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TraversalInfoRootPackageExtractor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TraversalInfoRootPackageExtractor.java @@ -21,11 +21,11 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.util.concurrent.ThreadFactoryBuilder; +import com.google.devtools.build.lib.cmdline.BatchCallback.SafeBatchCallback; import com.google.devtools.build.lib.cmdline.PackageIdentifier; +import com.google.devtools.build.lib.cmdline.ParallelVisitor; +import com.google.devtools.build.lib.cmdline.QueryExceptionMarkerInterface; import com.google.devtools.build.lib.cmdline.RepositoryName; -import com.google.devtools.build.lib.concurrent.BatchCallback; -import com.google.devtools.build.lib.concurrent.ParallelVisitor; -import com.google.devtools.build.lib.concurrent.ParallelVisitor.UnusedException; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.vfs.PathFragment; @@ -53,7 +53,7 @@ public class TraversalInfoRootPackageExtractor implements RootPackageExtractor { @Override public void streamPackagesFromRoots( - BatchCallback results, + SafeBatchCallback results, WalkableGraph graph, List roots, ExtendedEventHandler eventHandler, @@ -96,15 +96,15 @@ static class PackageCollectingParallelVisitor TraversalInfo, TraversalInfo, PackageIdentifier, - UnusedException, - BatchCallback> { + QueryExceptionMarkerInterface.MarkerRuntimeException, + SafeBatchCallback> { private final ExtendedEventHandler eventHandler; private final RepositoryName repository; private final WalkableGraph graph; PackageCollectingParallelVisitor( - BatchCallback callback, + SafeBatchCallback callback, int visitBatchSize, int processResultsBatchSize, int minPendingTasks, @@ -114,7 +114,7 @@ static class PackageCollectingParallelVisitor WalkableGraph graph) { super( callback, - UnusedException.class, + QueryExceptionMarkerInterface.MarkerRuntimeException.class, visitBatchSize, processResultsBatchSize, minPendingTasks, diff --git a/src/test/java/com/google/devtools/build/lib/cmdline/BUILD b/src/test/java/com/google/devtools/build/lib/cmdline/BUILD index cfca030825823c..ef53ba5bf69f7e 100644 --- a/src/test/java/com/google/devtools/build/lib/cmdline/BUILD +++ b/src/test/java/com/google/devtools/build/lib/cmdline/BUILD @@ -18,9 +18,13 @@ java_library( deps = [ "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/cmdline:LabelValidator", + "//src/main/java/com/google/devtools/build/lib/cmdline:batch_callback", + "//src/main/java/com/google/devtools/build/lib/cmdline:parallel_visitor", + "//src/main/java/com/google/devtools/build/lib/cmdline:query_exception_marker_interface", + "//src/main/java/com/google/devtools/build/lib/concurrent", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//src/main/java/net/starlark/java/eval", - "//src/test/java/com/google/devtools/build/lib/testutil:TestUtils", + "//src/test/java/com/google/devtools/build/lib/testutil:TestThread", "//third_party:guava", "//third_party:guava-testlib", "//third_party:junit4", diff --git a/src/test/java/com/google/devtools/build/lib/concurrent/ParallelVisitorTest.java b/src/test/java/com/google/devtools/build/lib/cmdline/ParallelVisitorTest.java similarity index 89% rename from src/test/java/com/google/devtools/build/lib/concurrent/ParallelVisitorTest.java rename to src/test/java/com/google/devtools/build/lib/cmdline/ParallelVisitorTest.java index da5486832a7cf8..f14724a3ad3c60 100644 --- a/src/test/java/com/google/devtools/build/lib/concurrent/ParallelVisitorTest.java +++ b/src/test/java/com/google/devtools/build/lib/cmdline/ParallelVisitorTest.java @@ -11,7 +11,7 @@ // 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.lib.concurrent; +package com.google.devtools.build.lib.cmdline; import static com.google.common.truth.Truth.assertThat; @@ -19,6 +19,7 @@ import com.google.common.collect.ImmutableMultimap; import com.google.common.collect.Iterables; import com.google.common.collect.Sets; +import com.google.devtools.build.lib.cmdline.BatchCallback.SafeBatchCallback; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.testutil.TestThread; import java.util.ArrayList; @@ -37,17 +38,10 @@ public class ParallelVisitorTest { private static final int BATCH_CALLBACK_SIZE = 10_000; private static final long MIN_PENDING_TASKS = 3L; - private static class SampleException extends Exception { - SampleException() { - super("sample exception"); - } - } - @ThreadSafe - private interface TestCallback extends BatchCallback { - + private interface TestCallback extends SafeBatchCallback { @Override - void process(Iterable partialResult) throws SampleException, InterruptedException; + void process(Iterable partialResult); } /** @@ -56,7 +50,12 @@ private interface TestCallback extends BatchCallback { */ private static class DelayGettingVisitResultParallelVisitor extends ParallelVisitor< - String, String, String, String, SampleException, TestCallback> { + String, + String, + String, + String, + QueryExceptionMarkerInterface.MarkerRuntimeException, + TestCallback> { private final CountDownLatch invocationLatch; private final CountDownLatch delayLatch; @@ -64,7 +63,7 @@ private DelayGettingVisitResultParallelVisitor( CountDownLatch invocationLatch, CountDownLatch delayLatch) { super( targets -> {}, - SampleException.class, + QueryExceptionMarkerInterface.MarkerRuntimeException.class, /*visitBatchSize=*/ 1, /*processResultsBatchSize=*/ 1, /*minPendingTasks=*/ MIN_PENDING_TASKS, @@ -76,8 +75,7 @@ private DelayGettingVisitResultParallelVisitor( } @Override - protected Visit getVisitResult(Iterable values) - throws SampleException, InterruptedException { + protected Visit getVisitResult(Iterable values) throws InterruptedException { invocationLatch.countDown(); delayLatch.await(); return new Visit(ImmutableList.of(), values); @@ -89,14 +87,13 @@ protected Iterable preprocessInitialVisit(Iterable visitationKey } @Override - protected Iterable outputKeysToOutputValues(Iterable targetKeys) - throws SampleException, InterruptedException { + protected Iterable outputKeysToOutputValues(Iterable targetKeys) { return ImmutableList.of(); } @Override protected Iterable noteAndReturnUniqueVisitationKeys( - Iterable prospectiveVisitationKeys) throws SampleException { + Iterable prospectiveVisitationKeys) { return ImmutableList.copyOf(prospectiveVisitationKeys); } } @@ -124,7 +121,12 @@ public void testInterrupt() throws Exception { private static class RecordingParallelVisitor extends ParallelVisitor< - InputKey, String, String, String, SampleException, TestCallback> { + InputKey, + String, + String, + String, + QueryExceptionMarkerInterface.MarkerRuntimeException, + TestCallback> { private final ArrayList> visits = new ArrayList<>(); private final ImmutableMultimap successorMap; private final Set visited = Sets.newConcurrentHashSet(); @@ -136,7 +138,7 @@ private RecordingParallelVisitor( int processResultsBatchSize) { super( recordingCallback, - SampleException.class, + QueryExceptionMarkerInterface.MarkerRuntimeException.class, visitBatchSize, processResultsBatchSize, MIN_PENDING_TASKS,