Don't bother getting the not-real FileStateValue for a path whose parent doesn't exist.
In addition to saving a filesystem operation, this removes a source of a potential filesystem inconsistency.
--
MOS_MIGRATED_REVID=136355008
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java
index 8404f53..617baad 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java
@@ -18,6 +18,7 @@
import com.google.common.collect.Iterables;
import com.google.common.collect.Sets;
import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
+import com.google.devtools.build.lib.skyframe.FileStateValue.Type;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.lib.vfs.Path;
@@ -80,34 +81,36 @@
}
realRootedPath = resolvedState.getFirst();
realFileStateValue = resolvedState.getSecond();
+ if (realFileStateValue.getType() == Type.NONEXISTENT) {
+ return FileValue.value(
+ rootedPath,
+ FileStateValue.NONEXISTENT_FILE_STATE_NODE,
+ realRootedPath,
+ realFileStateValue);
+ }
}
- FileStateValue fileStateValue = null;
-
- try {
- fileStateValue =
- (FileStateValue)
- env.getValueOrThrow(
- FileStateValue.key(rootedPath), FileOutsidePackageRootsException.class);
- } catch (FileOutsidePackageRootsException e) {
- throw new FileFunctionException(
- new FileOutsidePackageRootsException(rootedPath), Transience.PERSISTENT);
+ FileStateValue fileStateValue;
+ if (rootedPath.equals(realRootedPath)) {
+ fileStateValue = Preconditions.checkNotNull(realFileStateValue, rootedPath);
+ } else {
+ try {
+ fileStateValue =
+ (FileStateValue)
+ env.getValueOrThrow(
+ FileStateValue.key(rootedPath), FileOutsidePackageRootsException.class);
+ } catch (FileOutsidePackageRootsException e) {
+ throw new FileFunctionException(
+ new FileOutsidePackageRootsException(rootedPath), Transience.PERSISTENT);
+ }
+ if (fileStateValue == null) {
+ return null;
+ }
}
- if (fileStateValue == null) {
- return null;
- }
if (realFileStateValue == null) {
realRootedPath = rootedPath;
realFileStateValue = fileStateValue;
- } else if (rootedPath.equals(realRootedPath) && !fileStateValue.equals(realFileStateValue)) {
- String message = String.format(
- "Some filesystem operations implied %s was a %s but others made us think it was a %s",
- rootedPath.asPath().getPathString(),
- fileStateValue.prettyPrint(),
- realFileStateValue.prettyPrint());
- throw new FileFunctionException(new InconsistentFilesystemException(message),
- Transience.TRANSIENT);
}
ArrayList<RootedPath> symlinkChain = new ArrayList<>();
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java
index 5257324..6308953 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java
@@ -888,22 +888,13 @@
}
@Test
- public void testFilesystemInconsistencies_ParentDoesntExistAndChildIsSymlink() throws Exception {
- symlink("a/b", "doesntmatter");
- // Our custom filesystem says "a/b" exists but "a" does not exist.
+ public void testDoesntStatChildIfParentDoesntExist() throws Exception {
+ // Our custom filesystem says "a" does not exist, so FileFunction shouldn't bother trying to
+ // think about "a/b". Test for this by having a stat of "a/b" fail with an io error, and
+ // observing that we don't encounter the error.
fs.stubStat(path("a"), null);
- SequentialBuildDriver driver = makeDriver();
- SkyKey skyKey = skyKey("a/b");
- EvaluationResult<FileValue> result =
- driver.evaluate(
- ImmutableList.of(skyKey), false, DEFAULT_THREAD_COUNT, NullEventHandler.INSTANCE);
- assertTrue(result.hasError());
- ErrorInfo errorInfo = result.getError(skyKey);
- assertThat(errorInfo.getException()).isInstanceOf(InconsistentFilesystemException.class);
- assertThat(errorInfo.getException().getMessage())
- .contains(
- "/root/a/b was a symlink to doesntmatter but others made us think it was a "
- + "nonexistent path");
+ fs.stubStatError(path("a/b"), new IOException("ouch!"));
+ assertThat(valueForPath(path("a/b")).exists()).isFalse();
}
@Test
@@ -1639,8 +1630,9 @@
private class CustomInMemoryFs extends InMemoryFileSystem {
- private Map<Path, FileStatus> stubbedStats = Maps.newHashMap();
- private Map<Path, IOException> stubbedFastDigestErrors = Maps.newHashMap();
+ private final Map<Path, FileStatus> stubbedStats = Maps.newHashMap();
+ private final Map<Path, IOException> stubbedStatErrors = Maps.newHashMap();
+ private final Map<Path, IOException> stubbedFastDigestErrors = Maps.newHashMap();
public CustomInMemoryFs(ManualClock manualClock) {
super(manualClock);
@@ -1667,8 +1659,15 @@
stubbedStats.put(path, stubbedResult);
}
+ public void stubStatError(Path path, IOException error) {
+ stubbedStatErrors.put(path, error);
+ }
+
@Override
public FileStatus stat(Path path, boolean followSymlinks) throws IOException {
+ if (stubbedStatErrors.containsKey(path)) {
+ throw stubbedStatErrors.get(path);
+ }
if (stubbedStats.containsKey(path)) {
return stubbedStats.get(path);
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java
index 9175145..3c960df 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java
@@ -252,7 +252,6 @@
Path bazDir = bazFile.getParentDirectory();
Path barDir = bazDir.getParentDirectory();
- long bazFileNodeId = bazFile.stat().getNodeId();
// Our custom filesystem says "foo/bar/baz" does not exist but it also says that "foo/bar"
// has a child directory "baz".
fs.stubStat(bazDir, null);
@@ -265,9 +264,7 @@
new Dirent("baz", Dirent.Type.DIRECTORY))));
differencer.inject(ImmutableMap.of(DirectoryListingValue.key(barDirRootedPath), barDirListing));
SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//foo"));
- String expectedMessage = "Some filesystem operations implied /workspace/foo/bar/baz/baz.sh was "
- + "a regular file with size of 0 and mtime of 0 and nodeId of " + bazFileNodeId + " and "
- + "mtime of 0 but others made us think it was a nonexistent path";
+ String expectedMessage = "/workspace/foo/bar/baz is no longer an existing directory";
EvaluationResult<PackageValue> result = SkyframeExecutorTestUtils.evaluate(
getSkyframeExecutor(), skyKey, /*keepGoing=*/false, reporter);
assertTrue(result.hasError());
