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 bazelbuild#6092.

Change-Id: I1a841ea6ba644ac09dde4838007ba1ecab46defb
PiperOrigin-RevId: 211976021

src/main/java/com/google/devtools/build/lib/vfs/AbstractFileSystem.java

@@ -56,42 +56,49 @@ 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)
             || profiler.isProfiling(ProfilerTask.VFS_OPEN))) {
       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);
       }

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();

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<Path> 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'});
+    }
+  }
 }

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);
+    }
+  }
 }