Remove WalkableGraph#exists and allow WalkableGraph#getValue and WalkableGraph#getException to be given non-existent keys without crashing. Add WalkableGraph#isCycle to fill the gap in testing for the difference between non-existence and depending on a cycle.
--
PiperOrigin-RevId: 143205289
MOS_MIGRATED_REVID=143205289
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 ae7aa9d..551b098 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
@@ -43,6 +43,7 @@
import com.google.devtools.build.lib.graph.Digraph;
import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
import com.google.devtools.build.lib.packages.DependencyFilter;
+import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
import com.google.devtools.build.lib.packages.NoSuchThingException;
import com.google.devtools.build.lib.packages.Package;
@@ -728,9 +729,6 @@
public Target getTarget(Label label)
throws TargetNotFoundException, QueryException, InterruptedException {
SkyKey packageKey = PackageValue.key(label.getPackageIdentifier());
- if (!graph.exists(packageKey)) {
- throw new QueryException(packageKey + " does not exist in graph");
- }
try {
PackageValue packageValue = (PackageValue) graph.getValue(packageKey);
if (packageValue != null) {
@@ -740,8 +738,16 @@
}
return packageValue.getPackage().getTarget(label.getName());
} else {
- throw (NoSuchThingException) Preconditions.checkNotNull(
- graph.getException(packageKey), label);
+ NoSuchThingException exception = (NoSuchThingException) graph.getException(packageKey);
+ if (exception != null) {
+ throw exception;
+ }
+ if (graph.isCycle(packageKey)) {
+ throw new NoSuchPackageException(
+ label.getPackageIdentifier(), "Package depends on a cycle");
+ } else {
+ throw new QueryException(packageKey + " does not exist in graph");
+ }
}
} catch (NoSuchThingException e) {
throw new TargetNotFoundException(e);
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 9a94f3f..6fef55c 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
@@ -135,7 +135,7 @@
@Override
@Nullable
public SkyValue get(SkyKey key) throws InterruptedException {
- return walkableGraph.exists(key) ? walkableGraph.getValue(key) : null;
+ return walkableGraph.getValue(key);
}
}
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 5e31b91..337b14d 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
@@ -81,22 +81,21 @@
throws NoSuchPackageException, InterruptedException {
SkyKey pkgKey = PackageValue.key(packageName);
- PackageValue pkgValue;
- if (graph.exists(pkgKey)) {
- pkgValue = (PackageValue) graph.getValue(pkgKey);
- if (pkgValue == null) {
- NoSuchPackageException nspe = (NoSuchPackageException) graph.getException(pkgKey);
- if (nspe != null) {
- throw nspe;
- }
- throw new NoSuchPackageException(packageName, "Package depends on a cycle");
- }
+ PackageValue pkgValue = (PackageValue) graph.getValue(pkgKey);
+ if (pkgValue != null) {
+ return pkgValue.getPackage();
+ }
+ NoSuchPackageException nspe = (NoSuchPackageException) graph.getException(pkgKey);
+ if (nspe != null) {
+ throw nspe;
+ }
+ if (graph.isCycle(pkgKey)) {
+ throw new NoSuchPackageException(packageName, "Package depends on a cycle");
} else {
// If the package key does not exist in the graph, then it must not correspond to any package,
// because the SkyQuery environment has already loaded the universe.
throw new BuildFileNotFoundException(packageName, "BUILD file not found on package path");
}
- return pkgValue.getPackage();
}
@Override
@@ -139,22 +138,25 @@
public boolean isPackage(EventHandler eventHandler, PackageIdentifier packageName)
throws InterruptedException {
SkyKey packageLookupKey = PackageLookupValue.key(packageName);
- if (!graph.exists(packageLookupKey)) {
- // If the package lookup key does not exist in the graph, then it must not correspond to any
- // package, because the SkyQuery environment has already loaded the universe.
- return false;
- }
PackageLookupValue packageLookupValue = (PackageLookupValue) graph.getValue(packageLookupKey);
if (packageLookupValue == null) {
- Exception exception = Preconditions.checkNotNull(graph.getException(packageLookupKey),
- "During package lookup for '%s', got null for exception", packageName);
- if (exception instanceof NoSuchPackageException
- || exception instanceof InconsistentFilesystemException) {
- eventHandler.handle(Event.error(exception.getMessage()));
+ // Package lookups can't depend on Skyframe cycles.
+ Preconditions.checkState(!graph.isCycle(packageLookupKey), packageLookupKey);
+ Exception exception = graph.getException(packageLookupKey);
+ if (exception == null) {
+ // If the package lookup key does not exist in the graph, then it must not correspond to any
+ // package, because the SkyQuery environment has already loaded the universe.
return false;
} else {
- throw new IllegalStateException("During package lookup for '" + packageName
- + "', got unexpected exception type", exception);
+ if (exception instanceof NoSuchPackageException
+ || exception instanceof InconsistentFilesystemException) {
+ eventHandler.handle(Event.error(exception.getMessage()));
+ return false;
+ } else {
+ throw new IllegalStateException(
+ "During package lookup for '" + packageName + "', got unexpected exception type",
+ exception);
+ }
}
}
return packageLookupValue.packageExists();
@@ -190,7 +192,7 @@
roots.addAll(pkgPath.getPathEntries());
} else {
RepositoryDirectoryValue repositoryValue =
- (RepositoryDirectoryValue) graph.getValue(RepositoryDirectoryValue.key(repository));
+ (RepositoryDirectoryValue) graph.getValue(RepositoryDirectoryValue.key(repository));
if (repositoryValue == null) {
// If this key doesn't exist, the repository is outside the universe, so we return
// "nothing".
diff --git a/src/main/java/com/google/devtools/build/skyframe/DelegatingWalkableGraph.java b/src/main/java/com/google/devtools/build/skyframe/DelegatingWalkableGraph.java
index 27475db..8abf5de 100644
--- a/src/main/java/com/google/devtools/build/skyframe/DelegatingWalkableGraph.java
+++ b/src/main/java/com/google/devtools/build/skyframe/DelegatingWalkableGraph.java
@@ -32,26 +32,18 @@
this.graph = graph;
}
+ @Nullable
private NodeEntry getEntryForValue(SkyKey key) throws InterruptedException {
NodeEntry entry =
- Preconditions.checkNotNull(
- graph.getBatch(null, Reason.WALKABLE_GRAPH_VALUE, ImmutableList.of(key)).get(key),
- key);
- Preconditions.checkState(entry.isDone(), "%s %s", key, entry);
- return entry;
- }
-
- @Override
- public boolean exists(SkyKey key) throws InterruptedException {
- NodeEntry entry =
- graph.getBatch(null, Reason.EXISTENCE_CHECKING, ImmutableList.of(key)).get(key);
- return entry != null && entry.isDone();
+ graph.getBatch(null, Reason.WALKABLE_GRAPH_VALUE, ImmutableList.of(key)).get(key);
+ return entry != null && entry.isDone() ? entry : null;
}
@Nullable
@Override
public SkyValue getValue(SkyKey key) throws InterruptedException {
- return getEntryForValue(key).getValue();
+ NodeEntry entry = getEntryForValue(key);
+ return entry == null ? null : entry.getValue();
}
private static SkyValue getValue(NodeEntry entry) throws InterruptedException {
@@ -93,10 +85,24 @@
return result;
}
+ @Override
+ public boolean isCycle(SkyKey key) throws InterruptedException {
+ NodeEntry entry = getEntryForValue(key);
+ if (entry == null) {
+ return false;
+ }
+ ErrorInfo errorInfo = entry.getErrorInfo();
+ return errorInfo != null && errorInfo.getCycleInfo() != null;
+ }
+
@Nullable
@Override
public Exception getException(SkyKey key) throws InterruptedException {
- ErrorInfo errorInfo = getEntryForValue(key).getErrorInfo();
+ NodeEntry entry = getEntryForValue(key);
+ if (entry == null) {
+ return null;
+ }
+ ErrorInfo errorInfo = entry.getErrorInfo();
return errorInfo == null ? null : errorInfo.getException();
}
diff --git a/src/main/java/com/google/devtools/build/skyframe/WalkableGraph.java b/src/main/java/com/google/devtools/build/skyframe/WalkableGraph.java
index 3d7796b..8f3dd04 100644
--- a/src/main/java/com/google/devtools/build/skyframe/WalkableGraph.java
+++ b/src/main/java/com/google/devtools/build/skyframe/WalkableGraph.java
@@ -30,17 +30,17 @@
*/
@ThreadSafe
public interface WalkableGraph {
-
- /**
- * Returns whether the given key exists as a done node in the graph. If there is a chance that the
- * given node does not exist, this method should be called before any others, since the others
- * throw a {@link RuntimeException} on failure to access a node.
- */
- boolean exists(SkyKey key) throws InterruptedException;
-
/**
* Returns the value of the given key, or {@code null} if it has no value due to an error during
- * its computation. A node with this key must exist in the graph.
+ * its computation or it is not done in the graph.
+ *
+ * <p>A node that is done in the graph must have either a non-null getValue, a non-null {@link
+ * #getException}, or a true {@link #isCycle}.
+ *
+ * <p>These three methods should all be reading the same {@link
+ * NodeEntry#getValueMaybeWithMetadata} value internally, so once that value is indirectly
+ * retrieved via one of these methods, the others can read it for free. This is relevant for graph
+ * implementations that may throw an {@link InterruptedException} on retrieving entries and value.
*/
@Nullable
SkyValue getValue(SkyKey key) throws InterruptedException;
@@ -63,21 +63,27 @@
/**
* Returns the exception thrown when computing the node with the given key, if any. If the node
- * was computed successfully, returns null. A node with this key must exist and be done in the
- * graph.
+ * was computed successfully, depends on a cycle without any other error, or is not done in the
+ * graph, returns null.
*/
@Nullable
Exception getException(SkyKey key) throws InterruptedException;
/**
+ * Returns true if the node with the given {@code key} depends on a cycle. Returns false if the
+ * node does not depend on a cycle, or is not done in the graph.
+ */
+ boolean isCycle(SkyKey key) throws InterruptedException;
+
+ /**
* Returns a map giving the direct dependencies of the nodes with the given keys. A node for each
- * given key must exist and be done in the graph.
+ * given key must be done in the graph if it exists.
*/
Map<SkyKey, Iterable<SkyKey>> getDirectDeps(Iterable<SkyKey> keys) throws InterruptedException;
/**
* Returns a map giving the reverse dependencies of the nodes with the given keys. A node for each
- * given key must exist and be done in the graph.
+ * given key must be done in the graph if it exists.
*/
Map<SkyKey, Iterable<SkyKey>> getReverseDeps(Iterable<SkyKey> keys) throws InterruptedException;
