Properly handle missing packages and I/O exceptions when parsing target patterns in no-keep-going mode. Additionally, eagerly terminate evaluation on "inconsistent filesystem exceptions" even in keep-going mode, by analogy with a source file changing mid-build, which terminates the build even in keep-going mode. Note that we still don't terminate an evaluation eagerly on all inconsistent filesystem exceptions, but this is incrementally better than before, IMO (I think the remaining holes are bzl files and reading source artifacts for the first time).
TargetPatternFunction can recover from missing packages/other errors in keep-going mode, and so before this change it very rarely threw an exception. However, in a no-keep-going evaluation (or a keep-going evaluation that failed catastrophically), the failure to throw an exception meant that it missed a chance to transform a lower-level exception into a more intelligible one. To allow TargetPatternFunction to distinguish these cases, we add a new method on SkyFunction.Environment.
PiperOrigin-RevId: 415073167
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 e9abf1b..56a5a59 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
@@ -13,6 +13,8 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;
+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.MoreExecutors.directExecutor;
import com.google.common.base.Throwables;
@@ -24,6 +26,7 @@
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.ListeningExecutorService;
import com.google.common.util.concurrent.MoreExecutors;
+import com.google.common.util.concurrent.Uninterruptibles;
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;
@@ -37,6 +40,8 @@
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;
+import com.google.devtools.build.lib.io.InconsistentFilesystemException;
+import com.google.devtools.build.lib.io.ProcessPackageDirectoryException;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.NoSuchThingException;
import com.google.devtools.build.lib.packages.Package;
@@ -54,12 +59,11 @@
import java.util.Map;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Future;
-/**
- * A {@link TargetPatternResolver} backed by a {@link RecursivePackageProvider}.
- */
+/** A {@link TargetPatternResolver} backed by a {@link RecursivePackageProvider}. */
@ThreadCompatible
-public class RecursivePackageProviderBackedTargetPatternResolver
+public final class RecursivePackageProviderBackedTargetPatternResolver
extends TargetPatternResolver<Target> {
// TODO(janakr): Move this to a more generic place and unify with SkyQueryEnvironment's value?
@@ -99,12 +103,13 @@
}
private Map<PackageIdentifier, Package> bulkGetPackages(Iterable<PackageIdentifier> pkgIds)
- throws NoSuchPackageException, InterruptedException {
+ throws NoSuchPackageException, InterruptedException {
return recursivePackageProvider.bulkGetPackages(pkgIds);
}
@Override
- public Target getTargetOrNull(Label label) throws InterruptedException {
+ public Target getTargetOrNull(Label label)
+ throws InterruptedException, InconsistentFilesystemException {
try {
if (!isPackage(label.getPackageIdentifier())) {
return null;
@@ -132,15 +137,14 @@
public Collection<Target> getTargetsInPackage(
String originalPattern, PackageIdentifier packageIdentifier, boolean rulesOnly)
throws TargetParsingException, InterruptedException {
- FilteringPolicy actualPolicy = rulesOnly
- ? FilteringPolicies.and(FilteringPolicies.RULES_ONLY, policy)
- : policy;
+ FilteringPolicy actualPolicy =
+ rulesOnly ? FilteringPolicies.and(FilteringPolicies.RULES_ONLY, policy) : policy;
try {
Package pkg = getPackage(packageIdentifier);
return TargetPatternResolverUtil.resolvePackageTargets(pkg, actualPolicy);
} catch (NoSuchThingException e) {
- String message = TargetPatternResolverUtil.getParsingErrorMessage(
- e.getMessage(), originalPattern);
+ String message =
+ TargetPatternResolverUtil.getParsingErrorMessage(e.getMessage(), originalPattern);
throw new TargetParsingException(message, e, e.getDetailedExitCode());
}
}
@@ -151,25 +155,27 @@
try {
Map<PackageIdentifier, Package> pkgs = bulkGetPackages(pkgIds);
if (pkgs.size() != Iterables.size(pkgIds)) {
- throw new IllegalStateException("Bulk package retrieval missing results: "
- + Sets.difference(ImmutableSet.copyOf(pkgIds), pkgs.keySet()));
+ throw new IllegalStateException(
+ "Bulk package retrieval missing results: "
+ + Sets.difference(ImmutableSet.copyOf(pkgIds), pkgs.keySet()));
}
ImmutableMap.Builder<PackageIdentifier, Collection<Target>> result = ImmutableMap.builder();
for (PackageIdentifier pkgId : pkgIds) {
Package pkg = pkgs.get(pkgId);
- result.put(pkgId, TargetPatternResolverUtil.resolvePackageTargets(pkg, policy));
+ result.put(pkgId, TargetPatternResolverUtil.resolvePackageTargets(pkg, policy));
}
return result.build();
} catch (NoSuchThingException e) {
- String message = TargetPatternResolverUtil.getParsingErrorMessage(
- e.getMessage(), originalPattern);
+ String message =
+ TargetPatternResolverUtil.getParsingErrorMessage(e.getMessage(), originalPattern);
throw new IllegalStateException(
"Mismatch: Expected given pkgIds to correspond to valid Packages. " + message, e);
}
}
@Override
- public boolean isPackage(PackageIdentifier packageIdentifier) throws InterruptedException {
+ public boolean isPackage(PackageIdentifier packageIdentifier)
+ throws InterruptedException, InconsistentFilesystemException {
return recursivePackageProvider.isPackage(eventHandler, packageIdentifier);
}
@@ -188,9 +194,11 @@
ImmutableSet<PathFragment> excludedSubdirectories,
BatchCallback<Target, E> callback,
Class<E> exceptionClass)
- throws TargetParsingException, E, InterruptedException {
+ throws TargetParsingException, E, InterruptedException, ProcessPackageDirectoryException {
+ ListenableFuture<Void> future;
try {
- findTargetsBeneathDirectoryAsyncImpl(
+ future =
+ findTargetsBeneathDirectoryAsyncImpl(
repository,
originalPattern,
directory,
@@ -198,11 +206,27 @@
forbiddenSubdirectories,
excludedSubdirectories,
callback,
- MoreExecutors.newDirectExecutorService())
- .get();
- } catch (ExecutionException e) {
- Throwables.propagateIfPossible(e.getCause(), TargetParsingException.class, exceptionClass);
- throw new IllegalStateException(e.getCause());
+ MoreExecutors.newDirectExecutorService());
+ } catch (QueryException e) {
+ Throwables.propagateIfPossible(e, exceptionClass);
+ throw new IllegalStateException(e);
+ } catch (NoSuchPackageException e) {
+ // Can happen during a Skyframe no-keep-going evaluation.
+ throw new TargetParsingException(
+ "error loading package under directory '" + directory + "': " + e.getMessage(),
+ e,
+ e.getDetailedExitCode());
+ }
+ if (!isSuccessful(future)) {
+ // Don't get the future if it finished successfully: all that will do is throw an
+ // interrupted exception if this thread was interrupted, but that's not helpful for a done
+ // future.
+ try {
+ future.get();
+ } catch (ExecutionException e) {
+ Throwables.propagateIfPossible(e.getCause(), InterruptedException.class, exceptionClass);
+ throw new IllegalStateException(e.getCause());
+ }
}
}
@@ -218,17 +242,39 @@
BatchCallback<Target, E> callback,
Class<E> exceptionClass,
ListeningExecutorService executor) {
- return findTargetsBeneathDirectoryAsyncImpl(
- repository,
- originalPattern,
- directory,
- rulesOnly,
- forbiddenSubdirectories,
- excludedSubdirectories,
- callback,
- executor);
+ try {
+ return findTargetsBeneathDirectoryAsyncImpl(
+ repository,
+ originalPattern,
+ directory,
+ rulesOnly,
+ forbiddenSubdirectories,
+ excludedSubdirectories,
+ callback,
+ executor);
+ } catch (TargetParsingException e) {
+ return immediateFailedFuture(e);
+ } catch (InterruptedException e) {
+ return immediateCancelledFuture();
+ } catch (ProcessPackageDirectoryException | NoSuchPackageException e) {
+ throw new IllegalStateException(
+ "Async find targets beneath directory isn't called from within Skyframe: traversing "
+ + directory
+ + " for "
+ + originalPattern,
+ e);
+ } catch (QueryException e) {
+ if (exceptionClass.isInstance(e)) {
+ return immediateFailedFuture(e);
+ }
+ throw new IllegalStateException(e);
+ }
}
+ /**
+ * The returned future may throw {@link QueryException} (if {@code E} is {@link QueryException})
+ * or {@link InterruptedException} on retrieval, but no other exceptions.
+ */
private <E extends Exception & QueryExceptionMarkerInterface>
ListenableFuture<Void> findTargetsBeneathDirectoryAsyncImpl(
RepositoryName repository,
@@ -238,7 +284,9 @@
ImmutableSet<PathFragment> forbiddenSubdirectories,
ImmutableSet<PathFragment> excludedSubdirectories,
BatchCallback<Target, E> callback,
- ListeningExecutorService executor) {
+ ListeningExecutorService executor)
+ throws TargetParsingException, QueryException, InterruptedException,
+ ProcessPackageDirectoryException, NoSuchPackageException {
FilteringPolicy actualPolicy =
rulesOnly ? FilteringPolicies.and(FilteringPolicies.RULES_ONLY, policy) : policy;
@@ -249,11 +297,10 @@
executor.submit(
new GetTargetsInPackagesTask<>(pkgIdBatch, pattern, actualPolicy, callback)));
- PathFragment pathFragment;
+ PathFragment pathFragment = TargetPatternResolverUtil.getPathFragment(directory);
try (PackageIdentifierBatchingCallback batchingCallback =
packageIdentifierBatchingCallbackFactory.create(
getPackageTargetsCallback, MAX_PACKAGES_BULK_GET)) {
- pathFragment = TargetPatternResolverUtil.getPathFragment(directory);
recursivePackageProvider.streamPackagesUnderDirectory(
batchingCallback,
eventHandler,
@@ -261,19 +308,11 @@
pathFragment,
forbiddenSubdirectories,
excludedSubdirectories);
- } catch (TargetParsingException | QueryException e) {
- return Futures.immediateFailedFuture(e);
- } catch (InterruptedException e) {
- return Futures.immediateCancelledFuture();
}
-
if (futures.isEmpty()) {
- return Futures.immediateFailedFuture(
- new TargetParsingException(
- "no targets found beneath '" + pathFragment + "'",
- TargetPatterns.Code.TARGETS_MISSING));
+ throw new TargetParsingException(
+ "no targets found beneath '" + pathFragment + "'", TargetPatterns.Code.TARGETS_MISSING);
}
-
return Futures.whenAllSucceed(futures).call(() -> null, directExecutor());
}
@@ -301,7 +340,7 @@
}
@Override
- public Void call() throws Exception {
+ public Void call() throws E, InterruptedException {
ImmutableSet<PackageIdentifier> pkgIdBatchSet = ImmutableSet.copyOf(packageIdentifiers);
packageSemaphore.acquireAll(pkgIdBatchSet);
try {
@@ -336,5 +375,17 @@
}
return size;
}
-}
+ /** Inspired by not-yet-open-source futures code. */
+ private static boolean isSuccessful(Future<?> future) {
+ if (future.isDone() && !future.isCancelled()) {
+ try {
+ Uninterruptibles.getUninterruptibly(future);
+ return true;
+ } catch (ExecutionException | RuntimeException e) {
+ // Fall through.
+ }
+ }
+ return false;
+ }
+}