Skip to content

Commit

Permalink
WindowsFileSystem: open files with delete-sharing
Browse files Browse the repository at this point in the history
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
  • Loading branch information
laszlocsomor authored and Copybara-Service committed Sep 7, 2018
1 parent e6709cc commit 1a95502
Show file tree
Hide file tree
Showing 4 changed files with 196 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand All @@ -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
Expand All @@ -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);
}
Expand All @@ -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
Expand All @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand All @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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'});
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}
}

0 comments on commit 1a95502

Please sign in to comment.