Make InconsistentFilesystemException less pervasive in bzl skyframe code
ASTFileLookupFunction:
- Delete handling of this exception entirely, as it's dead code. Its use for package lookup was removed by https://github.com/bazelbuild/bazel/commit/c5d9e2b2034ee7b3c7bc26db095ff0a14e7785f1, and its special casing in a try statement was refactored in https://github.com/bazelbuild/bazel/commit/dc1d9dcf7f6149a37b71a150b3f194937e9682a7.
BzlLoadFunction:
- Delete it from many method signatures.
- In package lookup, re-raise it as BzlLoadFailedException, just like for BuildFileNotFoundException.
- Narrow the type for deferredException, replace the one use of MoreObjects.firstNonNull().
PackageFunction:
- Same simplification of deferredException.
RELNOTES: None
PiperOrigin-RevId: 328162322
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java
index 799b892..fa151b2 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java
@@ -15,7 +15,6 @@
package com.google.devtools.build.lib.skyframe;
import com.google.devtools.build.lib.actions.FileValue;
-import com.google.devtools.build.lib.actions.InconsistentFilesystemException;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.packages.PackageFactory;
@@ -66,8 +65,6 @@
(ASTFileLookupValue.Key) skyKey.argument(), env, packageFactory, digestHashFunction);
} catch (ErrorReadingStarlarkExtensionException e) {
throw new ASTLookupFunctionException(e, e.getTransience());
- } catch (InconsistentFilesystemException e) {
- throw new ASTLookupFunctionException(e, Transience.PERSISTENT);
}
}
@@ -76,8 +73,7 @@
Environment env,
PackageFactory packageFactory,
DigestHashFunction digestHashFunction)
- throws ErrorReadingStarlarkExtensionException, InconsistentFilesystemException,
- InterruptedException {
+ throws ErrorReadingStarlarkExtensionException, InterruptedException {
byte[] bytes;
byte[] digest;
String inputName;
@@ -182,9 +178,5 @@
ErrorReadingStarlarkExtensionException e, Transience transience) {
super(e, transience);
}
-
- private ASTLookupFunctionException(InconsistentFilesystemException e, Transience transience) {
- super(e, transience);
- }
}
}
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 dcc532d..d52beb5 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -1346,6 +1346,7 @@
name = "error_reading_skylark_extension_exception",
srcs = ["ErrorReadingStarlarkExtensionException.java"],
deps = [
+ "//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/skyframe",
],
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java
index 52b7472..c11a0b2 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java
@@ -13,10 +13,8 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;
-import com.google.common.base.MoreObjects;
import com.google.common.base.Preconditions;
import com.google.common.base.Predicates;
-import com.google.common.base.Throwables;
import com.google.common.cache.Cache;
import com.google.common.cache.CacheBuilder;
import com.google.common.collect.ImmutableList;
@@ -167,8 +165,6 @@
BzlLoadValue.Key key = (BzlLoadValue.Key) skyKey.argument();
try {
return computeInternal(key, env, /*inliningState=*/ null);
- } catch (InconsistentFilesystemException e) {
- throw new BzlLoadFunctionException(e, Transience.PERSISTENT);
} catch (BzlLoadFailedException e) {
throw new BzlLoadFunctionException(e);
}
@@ -237,7 +233,7 @@
// TODO(brandjon): Pick one of the nouns "load" and "bzl" and use that term consistently.
@Nullable
BzlLoadValue computeInline(BzlLoadValue.Key key, Environment env, InliningState inliningState)
- throws InconsistentFilesystemException, BzlLoadFailedException, InterruptedException {
+ throws BzlLoadFailedException, InterruptedException {
// Note to refactorors: No Skyframe calls may be made before the RecordingSkyFunctionEnvironment
// is set up below in computeInlineForCacheMiss.
Preconditions.checkNotNull(cachedBzlLoadDataManager);
@@ -261,7 +257,7 @@
@Nullable
private CachedBzlLoadData computeInlineCachedData(
BzlLoadValue.Key key, Environment env, InliningState inliningState)
- throws InconsistentFilesystemException, BzlLoadFailedException, InterruptedException {
+ throws BzlLoadFailedException, InterruptedException {
// Note to refactorors: No Skyframe calls may be made before the RecordingSkyFunctionEnvironment
// is set up below in computeInlineForCacheMiss.
@@ -311,7 +307,7 @@
@Nullable
private CachedBzlLoadData computeInlineForCacheMiss(
BzlLoadValue.Key key, Environment env, InliningState inliningState)
- throws InconsistentFilesystemException, BzlLoadFailedException, InterruptedException {
+ throws BzlLoadFailedException, InterruptedException {
// We use an instrumented Skyframe env to capture Skyframe deps in the CachedBzlLoadData. This
// generally includes transitive Skyframe deps, but specifically excludes deps underneath
// recursively loaded .bzls. We unwrap the instrumented env right before recursively calling
@@ -459,7 +455,7 @@
@Nullable
private BzlLoadValue computeInternal(
BzlLoadValue.Key key, Environment env, @Nullable InliningState inliningState)
- throws InconsistentFilesystemException, BzlLoadFailedException, InterruptedException {
+ throws BzlLoadFailedException, InterruptedException {
Label label = key.getLabel();
PathFragment filePath = label.toPathFragment();
@@ -499,8 +495,7 @@
*/
@Nullable
private static ASTFileLookupValue.Key validatePackageAndGetASTKey(
- BzlLoadValue.Key key, Environment env)
- throws BzlLoadFailedException, InconsistentFilesystemException, InterruptedException {
+ BzlLoadValue.Key key, Environment env) throws BzlLoadFailedException, InterruptedException {
Label label = key.getLabel();
// Do package lookup.
@@ -518,6 +513,9 @@
} catch (BuildFileNotFoundException e) {
throw BzlLoadFailedException.errorReadingFile(
label.toPathFragment(), new ErrorReadingStarlarkExtensionException(e));
+ } catch (InconsistentFilesystemException e) {
+ throw BzlLoadFailedException.errorReadingFile(
+ label.toPathFragment(), new ErrorReadingStarlarkExtensionException(e));
}
if (packageLookup == null) {
return null;
@@ -556,7 +554,7 @@
ASTFileLookupValue astLookup,
Environment env,
@Nullable InliningState inliningState)
- throws InconsistentFilesystemException, BzlLoadFailedException, InterruptedException {
+ throws BzlLoadFailedException, InterruptedException {
Label label = key.getLabel();
PathFragment filePath = label.toPathFragment();
@@ -591,9 +589,7 @@
// loads.
// TODO(bazel-team): In case of a failed load(), we should report the location of the load()
// statement in the requesting file, e.g. using
- // file.getLoadStatements().get(...).getStartLocation(). We should also probably catch and
- // rethrow InconsistentFilesystemException with location info in the non-bzl-inlining code path
- // so the error message is the same in both code paths.
+ // file.getLoadStatements().get(...).getStartLocation().
List<BzlLoadValue> loadValues =
inliningState == null
? computeBzlLoadsWithSkyframe(env, loadKeys, file)
@@ -772,7 +768,7 @@
List<BzlLoadValue.Key> keys,
StarlarkFile requestingFile,
InliningState inliningState)
- throws InterruptedException, BzlLoadFailedException, InconsistentFilesystemException {
+ throws BzlLoadFailedException, InterruptedException {
String filePathForErrors = requestingFile.getStartLocation().file();
Preconditions.checkState(
env instanceof RecordingSkyFunctionEnvironment,
@@ -793,7 +789,7 @@
//
// This approach assumes --keep_going; determinism is not guaranteed otherwise. It also assumes
// InterruptedException does not occur, since we don't catch and defer it.
- Exception deferredException = null;
+ BzlLoadFailedException deferredException = null;
boolean valuesMissing = false;
// NOTE: Iterating over loads in the order listed in the file.
for (BzlLoadValue.Key key : keys) {
@@ -801,11 +797,9 @@
try {
cachedData = computeInlineCachedData(key, strippedEnv, inliningState);
} catch (BzlLoadFailedException e) {
- e = BzlLoadFailedException.whileLoadingDep(filePathForErrors, e);
- deferredException = MoreObjects.firstNonNull(deferredException, e);
- continue;
- } catch (InconsistentFilesystemException e) {
- deferredException = MoreObjects.firstNonNull(deferredException, e);
+ if (deferredException == null) {
+ deferredException = BzlLoadFailedException.whileLoadingDep(filePathForErrors, e);
+ }
continue;
}
if (cachedData == null) {
@@ -822,10 +816,7 @@
}
}
if (deferredException != null) {
- Throwables.throwIfInstanceOf(deferredException, BzlLoadFailedException.class);
- Throwables.throwIfInstanceOf(deferredException, InconsistentFilesystemException.class);
- throw new IllegalStateException(
- "caught a checked exception of unexpected type", deferredException);
+ throw deferredException;
}
return valuesMissing ? null : bzlLoads;
}
@@ -1035,8 +1026,7 @@
private interface ASTManager {
@Nullable
ASTFileLookupValue getASTFileLookupValue(ASTFileLookupValue.Key key, Environment env)
- throws InconsistentFilesystemException, InterruptedException,
- ErrorReadingStarlarkExtensionException;
+ throws ErrorReadingStarlarkExtensionException, InterruptedException;
void doneWithASTFileLookupValue(ASTFileLookupValue.Key key);
}
@@ -1048,13 +1038,9 @@
@Nullable
@Override
public ASTFileLookupValue getASTFileLookupValue(ASTFileLookupValue.Key key, Environment env)
- throws InconsistentFilesystemException, InterruptedException,
- ErrorReadingStarlarkExtensionException {
+ throws ErrorReadingStarlarkExtensionException, InterruptedException {
return (ASTFileLookupValue)
- env.getValueOrThrow(
- key,
- ErrorReadingStarlarkExtensionException.class,
- InconsistentFilesystemException.class);
+ env.getValueOrThrow(key, ErrorReadingStarlarkExtensionException.class);
}
@Override
@@ -1086,8 +1072,7 @@
@Nullable
@Override
public ASTFileLookupValue getASTFileLookupValue(ASTFileLookupValue.Key key, Environment env)
- throws InconsistentFilesystemException, InterruptedException,
- ErrorReadingStarlarkExtensionException {
+ throws ErrorReadingStarlarkExtensionException, InterruptedException {
ASTFileLookupValue value = astFileLookupValueCache.getIfPresent(key);
if (value == null) {
value = ASTFileLookupFunction.computeInline(key, env, packageFactory, digestHashFunction);
@@ -1142,9 +1127,5 @@
private BzlLoadFunctionException(BzlLoadFailedException cause) {
super(cause, cause.transience);
}
-
- private BzlLoadFunctionException(InconsistentFilesystemException e, Transience transience) {
- super(e, transience);
- }
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ErrorReadingStarlarkExtensionException.java b/src/main/java/com/google/devtools/build/lib/skyframe/ErrorReadingStarlarkExtensionException.java
index b26b2e4..441b8e8 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ErrorReadingStarlarkExtensionException.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ErrorReadingStarlarkExtensionException.java
@@ -13,11 +13,13 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;
+import com.google.devtools.build.lib.actions.InconsistentFilesystemException;
import com.google.devtools.build.lib.packages.BuildFileNotFoundException;
import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
import java.io.IOException;
/** Indicates some sort of IO error while dealing with a Starlark extension. */
+// TODO(brandjon): Delete this class.
public class ErrorReadingStarlarkExtensionException extends Exception {
private final Transience transience;
@@ -26,6 +28,11 @@
this.transience = Transience.PERSISTENT;
}
+ public ErrorReadingStarlarkExtensionException(InconsistentFilesystemException e) {
+ super(e.getMessage(), e);
+ this.transience = Transience.PERSISTENT;
+ }
+
public ErrorReadingStarlarkExtensionException(IOException e, Transience transience) {
super(e.getMessage(), e);
this.transience = transience;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
index ec4567d..9db3318 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
@@ -18,7 +18,6 @@
import static com.google.common.base.Preconditions.checkState;
import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.MoreObjects;
import com.google.common.base.Throwables;
import com.google.common.cache.Cache;
import com.google.common.collect.ImmutableList;
@@ -81,6 +80,7 @@
import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
+import com.google.devtools.build.skyframe.ValueOrException;
import com.google.devtools.build.skyframe.ValueOrException2;
import java.io.IOException;
import java.util.ArrayList;
@@ -661,14 +661,6 @@
.setMessage(e.getMessage())
.setPackageLoadingCode(PackageLoading.Code.IMPORT_STARLARK_FILE_ERROR)
.buildCause();
- } catch (InconsistentFilesystemException e) {
- throw PackageFunctionException.builder()
- .setType(PackageFunctionException.Type.NO_SUCH_PACKAGE)
- .setPackageIdentifier(packageId)
- .setException(e)
- .setMessage(e.getMessage())
- .setPackageLoadingCode(PackageLoading.Code.IMPORT_STARLARK_FILE_ERROR)
- .buildCause();
}
if (bzlLoads == null) {
return null; // Skyframe deps unavailable
@@ -693,12 +685,10 @@
@Nullable
private static List<BzlLoadValue> computeBzlLoadsNoInlining(
Environment env, List<BzlLoadValue.Key> keys)
- throws InterruptedException, BzlLoadFailedException, InconsistentFilesystemException {
+ throws InterruptedException, BzlLoadFailedException {
List<BzlLoadValue> bzlLoads = Lists.newArrayListWithExpectedSize(keys.size());
- Map<SkyKey, ValueOrException2<BzlLoadFailedException, InconsistentFilesystemException>>
- starlarkLookupResults =
- env.getValuesOrThrow(
- keys, BzlLoadFailedException.class, InconsistentFilesystemException.class);
+ Map<SkyKey, ValueOrException<BzlLoadFailedException>> starlarkLookupResults =
+ env.getValuesOrThrow(keys, BzlLoadFailedException.class);
for (BzlLoadValue.Key key : keys) {
bzlLoads.add((BzlLoadValue) starlarkLookupResults.get(key).get());
}
@@ -713,11 +703,11 @@
@Nullable
private static List<BzlLoadValue> computeBzlLoadsWithInlining(
Environment env, List<BzlLoadValue.Key> keys, BzlLoadFunction bzlLoadFunctionForInlining)
- throws InterruptedException, BzlLoadFailedException, InconsistentFilesystemException {
+ throws InterruptedException, BzlLoadFailedException {
List<BzlLoadValue> bzlLoads = Lists.newArrayListWithExpectedSize(keys.size());
// See the comment about the desire for deterministic graph structure in BzlLoadFunction for the
// motivation of this approach to exception handling.
- Exception deferredException = null;
+ BzlLoadFailedException deferredException = null;
// Compute BzlLoadValue for each key, sharing the same inlining state, i.e. cache of loaded
// modules. This ensures that each .bzl is loaded only once, regardless of diamond dependencies
// or cache eviction. (Multiple loads of the same .bzl would screw up identity equality of some
@@ -729,8 +719,10 @@
// Will complete right away if this key has been seen before in inliningState -- regardless
// of whether it was evaluated successfully, had missing deps, or was found to be in error.
skyValue = bzlLoadFunctionForInlining.computeInline(key, env, inliningState);
- } catch (BzlLoadFailedException | InconsistentFilesystemException e) {
- deferredException = MoreObjects.firstNonNull(deferredException, e);
+ } catch (BzlLoadFailedException e) {
+ if (deferredException == null) {
+ deferredException = e;
+ }
continue;
}
if (skyValue != null) {
@@ -743,10 +735,7 @@
// SkyFunctions that are requested and avoid a quadratic number of restarts.
}
if (deferredException != null) {
- Throwables.throwIfInstanceOf(deferredException, BzlLoadFailedException.class);
- Throwables.throwIfInstanceOf(deferredException, InconsistentFilesystemException.class);
- throw new IllegalStateException(
- "caught a checked exception of unexpected type", deferredException);
+ throw deferredException;
}
return env.valuesMissing() ? null : bzlLoads;
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunction.java
index 3227d60..41f4bf9 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunction.java
@@ -16,7 +16,6 @@
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
-import com.google.devtools.build.lib.actions.InconsistentFilesystemException;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.packages.PackageFactory;
import com.google.devtools.build.lib.skyframe.BzlLoadFunction.BzlLoadFailedException;
@@ -168,8 +167,6 @@
}
} catch (BzlLoadFailedException ex) {
throw BuiltinsFailedException.errorEvaluatingBuiltinsBzls(ex);
- } catch (InconsistentFilesystemException ex) {
- throw BuiltinsFailedException.errorEvaluatingBuiltinsBzls(ex);
}
if (exportsValue == null) {
return null;
@@ -236,11 +233,6 @@
}
static BuiltinsFailedException errorEvaluatingBuiltinsBzls(
- InconsistentFilesystemException cause) {
- return errorEvaluatingBuiltinsBzls(cause, Transience.PERSISTENT);
- }
-
- static BuiltinsFailedException errorEvaluatingBuiltinsBzls(
Exception cause, Transience transience) {
return new BuiltinsFailedException(
String.format("Failed to load builtins sources: %s", cause.getMessage()),
