Migrate UnixGlob to Path#statIfFound() instead of #statNullable(). Th…

…e latter swallows all filesystem failures, and does not disambiguate missing files from filesystem problems. The syscall cache now tracks IOExceptions if they are present, just as it does with readdir(). RELNOTES: None PiperOrigin-RevId: 153185433
rabadin · Apr 18, 2017 · 8fd7f75 · 8fd7f75
1 parent a45939b
commit 8fd7f75
Show file tree

Hide file tree

Showing 4 changed files with 72 additions and 25 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/PathPackageLocator.java b/src/main/java/com/google/devtools/build/lib/pkgcache/PathPackageLocator.java
@@ -28,6 +28,7 @@
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.Symlinks;
 import com.google.devtools.build.lib.vfs.UnixGlob;
+import java.io.IOException;
 import java.io.Serializable;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -111,10 +112,14 @@ public Path getPackageBuildFileNullable(PackageIdentifier packageIdentifier,
       // invocation in Parser#include().
       Path buildFile = outputBase.getRelative(
           packageIdentifier.getSourceRoot()).getRelative("BUILD");
-      FileStatus stat = cache.get().statNullable(buildFile, Symlinks.FOLLOW);
-      if (stat != null && stat.isFile()) {
-        return buildFile;
-      } else {
+      try {
+        FileStatus stat = cache.get().statIfFound(buildFile, Symlinks.FOLLOW);
+        if (stat != null && stat.isFile()) {
+          return buildFile;
+        } else {
+          return null;
+        }
+      } catch (IOException e) {
         return null;
       }
     }
@@ -225,9 +230,13 @@ private Path getFilePath(PathFragment suffix,
       AtomicReference<? extends UnixGlob.FilesystemCalls> cache) {
     for (Path pathEntry : pathEntries) {
       Path buildFile = pathEntry.getRelative(suffix);
-      FileStatus stat = cache.get().statNullable(buildFile, Symlinks.FOLLOW);
-      if (stat != null && stat.isFile()) {
-        return buildFile;
+      try {
+        FileStatus stat = cache.get().statIfFound(buildFile, Symlinks.FOLLOW);
+        if (stat != null && stat.isFile()) {
+          return buildFile;
+        }
+      } catch (IOException ignored) {
+        // Treat IOException as a missing file.
       }
     }
     return null;

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PerBuildSyscallCache.java b/src/main/java/com/google/devtools/build/lib/skyframe/PerBuildSyscallCache.java
@@ -30,13 +30,14 @@
  */
 public class PerBuildSyscallCache implements UnixGlob.FilesystemCalls {
 
-  private final LoadingCache<Pair<Path, Symlinks>, FileStatus> statCache;
+  private final LoadingCache<Pair<Path, Symlinks>, Pair<FileStatus, IOException>> statCache;
   private final LoadingCache<Pair<Path, Symlinks>, Pair<Collection<Dirent>, IOException>>
       readdirCache;
 
   private static final FileStatus NO_STATUS = new FakeFileStatus();
 
-  private PerBuildSyscallCache(LoadingCache<Pair<Path, Symlinks>, FileStatus> statCache,
+  private PerBuildSyscallCache(
+      LoadingCache<Pair<Path, Symlinks>, Pair<FileStatus, IOException>> statCache,
       LoadingCache<Pair<Path, Symlinks>, Pair<Collection<Dirent>, IOException>> readdirCache) {
     this.statCache = statCache;
     this.readdirCache = readdirCache;
@@ -104,9 +105,12 @@ public Collection<Dirent> readdir(Path path, Symlinks symlinks) throws IOExcepti
   }
 
   @Override
-  public FileStatus statNullable(Path path, Symlinks symlinks) {
-    FileStatus status = statCache.getUnchecked(Pair.of(path, symlinks));
-    return (status == NO_STATUS) ? null : status;
+  public FileStatus statIfFound(Path path, Symlinks symlinks) throws IOException {
+    Pair<FileStatus, IOException> status = statCache.getUnchecked(Pair.of(path, symlinks));
+    if (status.getFirst() != null) {
+      return (status.getFirst() == NO_STATUS) ? null : status.getFirst();
+    }
+    throw status.getSecond();
   }
 
   public void clear() {
@@ -162,12 +166,16 @@ public boolean isSymbolicLink() {
    * Input: (path, following_symlinks)
    * Output: FileStatus
    */
-  private static CacheLoader<Pair<Path, Symlinks>, FileStatus> newStatLoader() {
-    return new CacheLoader<Pair<Path, Symlinks>, FileStatus>() {
+  private static CacheLoader<Pair<Path, Symlinks>, Pair<FileStatus, IOException>> newStatLoader() {
+    return new CacheLoader<Pair<Path, Symlinks>, Pair<FileStatus, IOException>>() {
         @Override
-        public FileStatus load(Pair<Path, Symlinks> p) {
-          FileStatus f = p.first.statNullable(p.second);
-          return (f == null) ? NO_STATUS : f;
+        public Pair<FileStatus, IOException> load(Pair<Path, Symlinks> p) {
+          try {
+            FileStatus f = p.first.statIfFound(p.second);
+            return Pair.of((f == null) ? NO_STATUS : f, null);
+          } catch (IOException e) {
+            return Pair.of(null, e);
+          }
         }
       };
   }

diff --git a/src/main/java/com/google/devtools/build/lib/vfs/UnixGlob.java b/src/main/java/com/google/devtools/build/lib/vfs/UnixGlob.java
@@ -271,7 +271,7 @@ public interface FilesystemCalls {
     /**
      * Return the stat() for the given path, or null.
      */
-    FileStatus statNullable(Path path, Symlinks symlinks);
+    FileStatus statIfFound(Path path, Symlinks symlinks) throws IOException;
   }
 
   public static FilesystemCalls DEFAULT_SYSCALLS = new FilesystemCalls() {
@@ -281,8 +281,8 @@ public Collection<Dirent> readdir(Path path, Symlinks symlinks) throws IOExcepti
     }
 
     @Override
-    public FileStatus statNullable(Path path, Symlinks symlinks) {
-      return path.statNullable(symlinks);
+    public FileStatus statIfFound(Path path, Symlinks symlinks) throws IOException {
+      return path.statIfFound(symlinks);
     }
   };
 
@@ -547,14 +547,19 @@ private static boolean isRecursivePattern(String pattern) {
      * Same as {@link #glob}, except does so asynchronously and returns a {@link Future} for the
      * result.
      */
-    public Future<List<Path>> globAsync(
+    Future<List<Path>> globAsync(
         Path base,
         Collection<String> patterns,
         boolean excludeDirectories,
         Predicate<Path> dirPred,
         FilesystemCalls syscalls) {
 
-      FileStatus baseStat = syscalls.statNullable(base, Symlinks.FOLLOW);
+      FileStatus baseStat;
+      try {
+        baseStat = syscalls.statIfFound(base, Symlinks.FOLLOW);
+      } catch (IOException e) {
+        return Futures.immediateFailedFuture(e);
+      }
       if (baseStat == null || patterns.isEmpty()) {
         return Futures.immediateFuture(Collections.<Path>emptyList());
       }
@@ -782,7 +787,7 @@ private void reallyGlob(
       if (!pattern.contains("*") && !pattern.contains("?")) {
         // We do not need to do a readdir in this case, just a stat.
         Path child = base.getChild(pattern);
-        FileStatus status = context.syscalls.statNullable(child, Symlinks.FOLLOW);
+        FileStatus status = context.syscalls.statIfFound(child, Symlinks.FOLLOW);
         if (status == null || (!status.isDirectory() && !status.isFile())) {
           // The file is a dangling symlink, fifo, does not exist, etc.
           return;

diff --git a/src/test/java/com/google/devtools/build/lib/vfs/GlobTest.java b/src/test/java/com/google/devtools/build/lib/vfs/GlobTest.java
@@ -184,12 +184,37 @@ private Set<Path> resolvePaths(String... relativePaths) {
     return expectedFiles;
   }
 
+  @Test
+  public void testIOFailureOnStat() throws Exception {
+    UnixGlob.FilesystemCalls syscalls = new UnixGlob.FilesystemCalls() {
+      @Override
+      public FileStatus statIfFound(Path path, Symlinks symlinks) throws IOException {
+        throw new IOException("EIO");
+      }
+
+      @Override
+      public Collection<Dirent> readdir(Path path, Symlinks symlinks) {
+        throw new IllegalStateException();
+      }
+    };
+
+    try {
+      new UnixGlob.Builder(tmpPath)
+          .addPattern("foo/bar/wiz/file")
+          .setFilesystemCalls(new AtomicReference<>(syscalls))
+          .glob();
+      fail("Expected failure");
+    } catch (IOException e) {
+      assertThat(e).hasMessageThat().isEqualTo("EIO");
+    }
+  }
+
   @Test
   public void testGlobWithoutWildcardsDoesNotCallReaddir() throws Exception {
     UnixGlob.FilesystemCalls syscalls = new UnixGlob.FilesystemCalls() {
       @Override
-      public FileStatus statNullable(Path path, Symlinks symlinks) {
-        return UnixGlob.DEFAULT_SYSCALLS.statNullable(path, symlinks);
+      public FileStatus statIfFound(Path path, Symlinks symlinks) throws IOException {
+        return UnixGlob.DEFAULT_SYSCALLS.statIfFound(path, symlinks);
       }
 
       @Override