From 1a955020ff7a27b9d49c83aef89e0991380daa5e Mon Sep 17 00:00:00 2001 From: Laszlo Csomor Date: Fri, 7 Sep 2018 07:53:51 -0700 Subject: [PATCH] WindowsFileSystem: open files with delete-sharing Open files using `Files.new{Input,Output}Stream` instead of `new File{Input,Output}Stream`. The new method opens files with deletion sharing, the old one does not. This patch will hopefully fix the class of errors where Bazel fails to delete a file that's already open. Change-Id: I1a841ea6ba644ac09dde4838007ba1ecab46defb Closes #6092. Change-Id: I1a841ea6ba644ac09dde4838007ba1ecab46defb PiperOrigin-RevId: 211976021 --- .../build/lib/vfs/AbstractFileSystem.java | 91 +++++++++++++++---- .../build/lib/windows/WindowsFileSystem.java | 20 ++++ .../build/lib/vfs/JavaIoFileSystemTest.java | 44 +++++++++ .../lib/windows/WindowsFileSystemTest.java | 57 ++++++++++++ 4 files changed, 196 insertions(+), 16 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/vfs/AbstractFileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/AbstractFileSystem.java index 771e0735c889c4..16848c203acea6 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/AbstractFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/AbstractFileSystem.java @@ -56,7 +56,7 @@ protected InputStream getInputStream(Path path) throws IOException { } /** Returns either normal or profiled FileInputStream. */ - private InputStream createFileInputStream(Path path) throws FileNotFoundException { + private InputStream createFileInputStream(Path path) throws IOException { final String name = path.toString(); if (profiler.isActive() && (profiler.isProfiling(ProfilerTask.VFS_READ) @@ -64,34 +64,41 @@ private InputStream createFileInputStream(Path path) throws FileNotFoundExceptio long startTime = Profiler.nanoTimeMaybe(); try { // Replace default FileInputStream instance with the custom one that does profiling. - return new ProfiledFileInputStream(name); + return new ProfiledFileInputStream(name, newFileInputStream(name)); } finally { profiler.logSimpleTask(startTime, ProfilerTask.VFS_OPEN, name); } } else { // Use normal FileInputStream instance if profiler is not enabled. - return new FileInputStream(path.toString()); + return newFileInputStream(name); } } + protected InputStream newFileInputStream(String path) throws IOException { + return new FileInputStream(path); + } + + protected OutputStream newFileOutputStream(String path, boolean append) throws IOException { + return new FileOutputStream(path, append); + } + /** * Returns either normal or profiled FileOutputStream. Should be used by subclasses to create * default OutputStream instance. */ - protected OutputStream createFileOutputStream(Path path, boolean append) - throws FileNotFoundException { + protected OutputStream createFileOutputStream(Path path, boolean append) throws IOException { final String name = path.toString(); if (profiler.isActive() && (profiler.isProfiling(ProfilerTask.VFS_WRITE) || profiler.isProfiling(ProfilerTask.VFS_OPEN))) { long startTime = Profiler.nanoTimeMaybe(); try { - return new ProfiledFileOutputStream(name, append); + return new ProfiledFileOutputStream(name, newFileOutputStream(name, append)); } finally { profiler.logSimpleTask(startTime, ProfilerTask.VFS_OPEN, name); } } else { - return new FileOutputStream(name, append); + return newFileOutputStream(name, append); } } @@ -110,12 +117,33 @@ protected OutputStream getOutputStream(Path path, boolean append) throws IOExcep } } - private static final class ProfiledFileInputStream extends FileInputStream { + private static final class ProfiledFileInputStream extends InputStream { private final String name; + private final InputStream stm; - public ProfiledFileInputStream(String name) throws FileNotFoundException { - super(name); + public ProfiledFileInputStream(String name, InputStream stm) { this.name = name; + this.stm = stm; + } + + @Override + public int available() throws IOException { + return stm.available(); + } + + @Override + public void close() throws IOException { + stm.close(); + } + + @Override + public void mark(int readlimit) { + stm.mark(readlimit); + } + + @Override + public boolean markSupported() { + return stm.markSupported(); } @Override @@ -124,7 +152,7 @@ public int read() throws IOException { try { // Note that FileInputStream#read() does *not* call any of our overridden methods, // so there's no concern with double counting here. - return super.read(); + return stm.read(); } finally { profiler.logSimpleTask(startTime, ProfilerTask.VFS_READ, name); } @@ -139,19 +167,40 @@ public int read(byte[] b) throws IOException { public int read(byte[] b, int off, int len) throws IOException { long startTime = Profiler.nanoTimeMaybe(); try { - return super.read(b, off, len); + return stm.read(b, off, len); } finally { profiler.logSimpleTask(startTime, ProfilerTask.VFS_READ, name); } } + + @Override + public void reset() throws IOException { + stm.reset(); + } + + @Override + public long skip(long n) throws IOException { + return stm.skip(n); + } } - private static final class ProfiledFileOutputStream extends FileOutputStream { + private static final class ProfiledFileOutputStream extends OutputStream { private final String name; + private final OutputStream stm; - public ProfiledFileOutputStream(String name, boolean append) throws FileNotFoundException { - super(name, append); + public ProfiledFileOutputStream(String name, OutputStream stm) { this.name = name; + this.stm = stm; + } + + @Override + public void close() throws IOException { + stm.close(); + } + + @Override + public void flush() throws IOException { + stm.flush(); } @Override @@ -163,7 +212,17 @@ public void write(byte[] b) throws IOException { public void write(byte[] b, int off, int len) throws IOException { long startTime = Profiler.nanoTimeMaybe(); try { - super.write(b, off, len); + stm.write(b, off, len); + } finally { + profiler.logSimpleTask(startTime, ProfilerTask.VFS_WRITE, name); + } + } + + @Override + public void write(int b) throws IOException { + long startTime = Profiler.nanoTimeMaybe(); + try { + stm.write(b); } finally { profiler.logSimpleTask(startTime, ProfilerTask.VFS_WRITE, name); } diff --git a/src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java b/src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java index d39173a5dba482..c1249cb627ab3a 100644 --- a/src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java @@ -27,8 +27,12 @@ import java.io.File; import java.io.FileNotFoundException; import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; import java.nio.file.Files; import java.nio.file.LinkOption; +import java.nio.file.Paths; +import java.nio.file.StandardOpenOption; import java.nio.file.attribute.DosFileAttributes; /** File system implementation for Windows. */ @@ -51,6 +55,22 @@ public String getFileSystemType(Path path) { return "ntfs"; } + private static java.nio.file.Path getNioPath(String path) { + return Paths.get(path); + } + + @Override + protected InputStream newFileInputStream(String path) throws IOException { + return Files.newInputStream(getNioPath(path)); + } + + @Override + protected OutputStream newFileOutputStream(String path, boolean append) throws IOException { + return append + ? Files.newOutputStream(getNioPath(path), StandardOpenOption.APPEND) + : Files.newOutputStream(getNioPath(path)); + } + @Override public boolean delete(Path path) throws IOException { long startTime = Profiler.nanoTimeMaybe(); diff --git a/src/test/java/com/google/devtools/build/lib/vfs/JavaIoFileSystemTest.java b/src/test/java/com/google/devtools/build/lib/vfs/JavaIoFileSystemTest.java index cdcb22e4cff721..f34980c8baf67e 100644 --- a/src/test/java/com/google/devtools/build/lib/vfs/JavaIoFileSystemTest.java +++ b/src/test/java/com/google/devtools/build/lib/vfs/JavaIoFileSystemTest.java @@ -23,6 +23,8 @@ import com.google.devtools.build.lib.testutil.ManualClock; import com.google.devtools.build.lib.testutil.TestUtils; import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; import java.nio.file.Files; import java.nio.file.LinkOption; import java.nio.file.Paths; @@ -136,4 +138,46 @@ private static List getSubDirectories(Path base, int loopi, int subDirecto } return subDirs; } + + @Test + public void testDeleteFileOpenForReading() throws Exception { + Path path = absolutize("foo.txt"); + FileSystemUtils.writeIsoLatin1(path, "foo"); + // Sanity check: attempt reading from the file. + try (InputStream is = path.getInputStream()) { + byte[] contents = new byte[3]; + assertThat(is.read(contents)).isEqualTo(3); + assertThat(contents).isEqualTo(new byte[] {'f', 'o', 'o'}); + } + // Actual test: attempt deleting the file while open, then reading from it. + try (InputStream is = path.getInputStream()) { + assertThat(path.delete()).isTrue(); + assertThat(path.exists()).isFalse(); + + byte[] contents = new byte[3]; + assertThat(is.read(contents)).isEqualTo(3); + assertThat(contents).isEqualTo(new byte[] {'f', 'o', 'o'}); + } + } + + @Test + public void testDeleteFileOpenForWriting() throws Exception { + Path path = absolutize("foo.txt"); + // Sanity check: attempt writing to the file. + assertThat(path.exists()).isFalse(); + try (OutputStream os = path.getOutputStream(/* append */ false)) { + os.write(new byte[] {'b', 'a', 'r'}); + } + try (InputStream is = path.getInputStream()) { + byte[] contents = new byte[3]; + assertThat(is.read(contents)).isEqualTo(3); + assertThat(contents).isEqualTo(new byte[] {'b', 'a', 'r'}); + } + // Actual test: attempt deleting the file while open, then writing to it. + try (OutputStream os = path.getOutputStream(/* append */ false)) { + assertThat(path.delete()).isTrue(); + assertThat(path.exists()).isFalse(); + os.write(new byte[] {'b', 'a', 'r'}); + } + } } diff --git a/src/test/java/com/google/devtools/build/lib/windows/WindowsFileSystemTest.java b/src/test/java/com/google/devtools/build/lib/windows/WindowsFileSystemTest.java index 89303fdbd557cc..861e4aff96ead0 100644 --- a/src/test/java/com/google/devtools/build/lib/windows/WindowsFileSystemTest.java +++ b/src/test/java/com/google/devtools/build/lib/windows/WindowsFileSystemTest.java @@ -31,6 +31,9 @@ import com.google.devtools.build.lib.windows.util.WindowsTestUtil; import java.io.File; import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.nio.file.AccessDeniedException; import java.nio.file.Files; import java.util.Arrays; import java.util.HashMap; @@ -332,4 +335,58 @@ public String apply(File input) { assertThat(p.isFile()).isTrue(); } } + + @Test + public void testDeleteFileOpenForReading() throws Exception { + testUtil.scratchFile("foo.txt", "foo"); + Path path = testUtil.createVfsPath(fs, "foo.txt"); + // Sanity check: attempt reading from the file. + try (InputStream is = path.getInputStream()) { + byte[] contents = new byte[3]; + assertThat(is.read(contents)).isEqualTo(3); + assertThat(contents).isEqualTo(new byte[] {'f', 'o', 'o'}); + } + // Actual test: attempt deleting the file while open, then reading from it. + try (InputStream is = path.getInputStream()) { + assertThat(path.delete()).isTrue(); + assertThat(path.exists()).isFalse(); + + byte[] contents = new byte[3]; + assertThat(is.read(contents)).isEqualTo(3); + assertThat(contents).isEqualTo(new byte[] {'f', 'o', 'o'}); + } + } + + @Test + public void testDeleteFileOpenForWriting() throws Exception { + testUtil.scratchFile("bar.txt", "bar"); + Path path = testUtil.createVfsPath(fs, "bar.txt"); + // Sanity check: attempt writing to the file. + try (InputStream is = path.getInputStream()) { + byte[] contents = new byte[3]; + assertThat(is.read(contents)).isEqualTo(3); + assertThat(contents).isEqualTo(new byte[] {'b', 'a', 'r'}); + } + // Actual test: attempt deleting the file while open, then writing to it. + try (OutputStream os = path.getOutputStream(/* append */ false)) { + assertThat(path.delete()).isTrue(); + assertThat(path.exists()).isFalse(); + os.write(new byte[] {'b', 'a', 'r'}); + } + } + + @Test + public void testCannotOpenDirectoryAsFile() throws Exception { + testUtil.scratchFile("foo/bar.txt", "bar"); + Path file = testUtil.createVfsPath(fs, "foo/bar.txt"); + Path dir = file.getParentDirectory(); + // Sanity check: we can open the file. + try (InputStream is = file.getInputStream()) {} + assertThat(dir.isDirectory()).isTrue(); + try (InputStream is = dir.getInputStream()) { + fail("Expected failure"); + } catch (IOException e) { + assertThat(e).isInstanceOf(AccessDeniedException.class); + } + } }