Migrate UnixGlob to Path#statIfFound() instead of #statNullable(). The 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
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 index 3060916..f828538 100644 --- 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 @@ // 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 @@ 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 index 646d609..d9eb450 100644 --- 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 @@ } @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 @@ * 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 index d05677f..261072d 100644 --- 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 @@ /** * 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 @@ } @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 @@ * 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 @@ 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 index 430078a..c74e61d 100644 --- a/src/test/java/com/google/devtools/build/lib/vfs/GlobTest.java +++ b/src/test/java/com/google/devtools/build/lib/vfs/GlobTest.java
@@ -185,11 +185,36 @@ } @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