Bazel server: ensure OutputStreams are closed

Use try-with-resources to ensure OutputStreams that we open via FileSystem.OutputStream(path) are closed. Eagerly closing OutputStreams avoids hanging on to file handles until the garbage collector finalizes the OutputStream, meaning Bazel on Windows (and other processes) can delete or mutate these files. Hopefully this avoids intermittent file deletion errors that sometimes occur on Windows. See #5512 RELNOTES: none PiperOrigin-RevId: 203342889
bazelbuild · Jul 5, 2018 · 09d2031 · 09d2031 · jbduncan · Jul 5, 2018
1 parent 8ff87c1
commit 09d2031
Show file tree

Hide file tree

Showing 10 changed files with 56 additions and 28 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/util/DependencySet.java b/src/main/java/com/google/devtools/build/lib/util/DependencySet.java
@@ -228,15 +228,12 @@ public void write(Path outFile, String suffix) throws IOException {
     Path dotdFile =
         outFile.getRelative(FileSystemUtils.replaceExtension(outFile.asFragment(), suffix));
 
-    PrintStream out = new PrintStream(dotdFile.getOutputStream());
-    try {
+    try (PrintStream out = new PrintStream(dotdFile.getOutputStream())) {
       out.print(outFile.relativeTo(root) + ": ");
       for (Path d : dependencies) {
         out.print(" \\\n  " + d.getPathString());  // should already be root relative
       }
       out.println();
-    } finally {
-      out.close();
     }
   }
 

diff --git a/src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java b/src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java
@@ -698,7 +698,14 @@ public static void writeContentAsLatin1(Path outputFile, String content) throws
    */
   public static void writeContent(Path outputFile, Charset charset, String content)
       throws IOException {
-    asByteSink(outputFile).asCharSink(charset).write(content);
+    try (OutputStream out = outputFile.getOutputStream()) {
+      new ByteSink() {
+        @Override
+        public OutputStream openStream() throws IOException {
+          return out;
+        }
+      }.asCharSink(charset).write(content);
+    }
   }
 
   /**
@@ -729,7 +736,14 @@ public static void appendLinesAs(Path file, Charset charset, String... lines)
   public static void writeLinesAs(Path file, Charset charset, Iterable<String> lines)
       throws IOException {
     createDirectoryAndParents(file.getParentDirectory());
-    asByteSink(file).asCharSink(charset).writeLines(lines);
+    try (OutputStream out = file.getOutputStream()) {
+      new ByteSink() {
+        @Override
+        public OutputStream openStream() throws IOException {
+          return out;
+        }
+      }.asCharSink(charset).writeLines(lines);
+    }
   }
 
   /**
@@ -740,7 +754,14 @@ public static void writeLinesAs(Path file, Charset charset, Iterable<String> lin
   public static void appendLinesAs(Path file, Charset charset, Iterable<String> lines)
       throws IOException {
     createDirectoryAndParents(file.getParentDirectory());
-    asByteSink(file, true).asCharSink(charset).writeLines(lines);
+    try (OutputStream out = file.getOutputStream(true)) {
+      new ByteSink() {
+        @Override
+        public OutputStream openStream() throws IOException {
+          return out;
+        }
+      }.asCharSink(charset).writeLines(lines);
+    }
   }
 
   /**
@@ -749,7 +770,15 @@ public static void appendLinesAs(Path file, Charset charset, Iterable<String> li
    * @throws IOException if there was an error
    */
   public static void writeContent(Path outputFile, byte[] content) throws IOException {
-    asByteSink(outputFile).write(content);
+    try (OutputStream out = outputFile.getOutputStream()) {
+      new ByteSink() {
+        @Override
+        public OutputStream openStream() throws IOException {
+          return out;
+        }
+      }.write(content);
+      ;
+    }
   }
 
   /**

diff --git a/src/test/java/com/google/devtools/build/lib/exec/SingleBuildFileCacheTest.java b/src/test/java/com/google/devtools/build/lib/exec/SingleBuildFileCacheTest.java
@@ -25,6 +25,7 @@
 import com.google.devtools.build.lib.testutil.TestSpec;
 import com.google.devtools.build.lib.vfs.DigestHashFunction;
 import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
 import com.google.protobuf.ByteString;
@@ -73,8 +74,7 @@ protected byte[] getDigest(Path path, DigestHashFunction hf) throws IOException
           }
         };
     underTest = new SingleBuildFileCache("/", fs);
-    Path file = fs.getPath("/empty");
-    file.getOutputStream().close();
+    FileSystemUtils.createEmptyFile(fs.getPath("/empty"));
   }
 
   @Test
@@ -104,7 +104,7 @@ public void testDirectory() throws Exception {
   public void testCache() throws Exception {
     ActionInput empty = ActionInputHelper.fromPath("/empty");
     underTest.getMetadata(empty).getDigest();
-    assert(calls.containsKey("/empty"));
+    assertThat(calls).containsKey("/empty");
     assertThat((int) calls.get("/empty")).isEqualTo(1);
     underTest.getMetadata(empty).getDigest();
     assertThat((int) calls.get("/empty")).isEqualTo(1);
@@ -129,9 +129,9 @@ public void testUnreadableFileWhenFileSystemSupportsDigest() throws Exception {
 
     ActionInput input = ActionInputHelper.fromPath("/unreadable");
     Path file = fs.getPath("/unreadable");
-    file.getOutputStream().close();
+    FileSystemUtils.createEmptyFile(file);
     file.chmod(0);
     ByteString actualDigest = ByteString.copyFrom(underTest.getMetadata(input).getDigest());
-    assertThat(expectedDigest).isEqualTo(actualDigest);
+    assertThat(actualDigest).isEqualTo(expectedDigest);
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CreateIncSymlinkActionTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CreateIncSymlinkActionTest.java
@@ -25,6 +25,7 @@
 import com.google.devtools.build.lib.actions.util.DummyExecutor;
 import com.google.devtools.build.lib.testutil.FoundationTestCase;
 import com.google.devtools.build.lib.util.Fingerprint;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.Symlinks;
@@ -129,7 +130,7 @@ public void testFileRemoved() throws Exception {
     CreateIncSymlinkAction action =
         new CreateIncSymlinkAction(NULL_ACTION_OWNER, ImmutableMap.of(a, b), outputDir);
     Path extra = rootDirectory.getRelative("out/extra");
-    extra.getOutputStream().close();
+    FileSystemUtils.createEmptyFile(extra);
     assertThat(extra.exists()).isTrue();
     action.prepare(fileSystem, rootDirectory);
     assertThat(extra.exists()).isFalse();

diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/BaseSandboxfsProcessTest.java b/src/test/java/com/google/devtools/build/lib/sandbox/BaseSandboxfsProcessTest.java
@@ -95,7 +95,7 @@ public void testReconfigure() throws IOException {
 
       // Create twp mappings: one to be deleted and one to be kept around throughout the test.
       Path keepMeFile = tmpDir.getRelative("one");
-      keepMeFile.getOutputStream().close();
+      FileSystemUtils.createEmptyFile(keepMeFile);
       Path oneFile = tmpDir.getRelative("one");
       FileSystemUtils.writeContent(oneFile, UTF_8, "One test data");
       process.map(

diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/FakeSandboxfsProcessTest.java b/src/test/java/com/google/devtools/build/lib/sandbox/FakeSandboxfsProcessTest.java
@@ -18,6 +18,7 @@
 import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
 
 import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
 import java.io.IOException;
@@ -44,7 +45,7 @@ SandboxfsProcess mount(Path mountPoint) throws IOException {
 
   @Test
   public void testMount_NotADirectory() throws IOException {
-    tmpDir.getRelative("file").getOutputStream().close();
+    FileSystemUtils.createEmptyFile(tmpDir.getRelative("file"));
     IOException expected = assertThrows(
         IOException.class, () -> mount(tmpDir.getRelative("file")));
     assertThat(expected).hasMessageThat().matches(".*/file.*not a directory");

diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java
@@ -1365,11 +1365,8 @@ private Runnable makeWriteFileContentCallback(final Path toChange, final byte[]
     return new Runnable() {
       @Override
       public void run() {
-        OutputStream outputStream;
-        try {
-          outputStream = toChange.getOutputStream();
+        try (OutputStream outputStream = toChange.getOutputStream()) {
           outputStream.write(contents);
-          outputStream.close();
         } catch (IOException e) {
           e.printStackTrace();
           fail(e.getMessage());
@@ -1439,9 +1436,9 @@ private Pair<ImmutableList<String>, Runnable> changeFile(String fileStringToChan
     Path fileToChange = path(fileStringToChange);
     if (fileToChange.exists()) {
       final byte[] oldContents = FileSystemUtils.readContent(fileToChange);
-      OutputStream outputStream = fileToChange.getOutputStream(/*append=*/ true);
-      outputStream.write(new byte[] {(byte) 42}, 0, 1);
-      outputStream.close();
+      try (OutputStream outputStream = fileToChange.getOutputStream(/*append=*/ true)) {
+        outputStream.write(new byte[] {(byte) 42}, 0, 1);
+      }
       return Pair.of(
           ImmutableList.of(fileStringToChange),
           makeWriteFileContentCallback(fileToChange, oldContents));

diff --git a/src/test/java/com/google/devtools/build/lib/vfs/SearchPathTest.java b/src/test/java/com/google/devtools/build/lib/vfs/SearchPathTest.java
@@ -17,6 +17,7 @@
 
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
+import java.io.OutputStream;
 import java.util.List;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -38,7 +39,9 @@ public void testBasic() throws Exception {
     assertThat(SearchPath.parse(fs, "/:/bin")).isEqualTo(searchPath);
     assertThat(SearchPath.parse(fs, ".:/:/bin")).isEqualTo(searchPath);
 
-    fs.getOutputStream(fs.getPath("/bin/exe")).write(new byte[5]);
+    try (OutputStream out = fs.getOutputStream(fs.getPath("/bin/exe"))) {
+      out.write(new byte[5]);
+    }
 
     assertThat(SearchPath.which(searchPath, "exe")).isNull();
 

diff --git a/src/test/java/com/google/devtools/build/lib/vfs/inmemoryfs/InMemoryFileSystemTest.java b/src/test/java/com/google/devtools/build/lib/vfs/inmemoryfs/InMemoryFileSystemTest.java
@@ -59,9 +59,9 @@ public void destroyFileSystem(FileSystem fileSystem) {
    * Writes the given data to the given file.
    */
   private static void writeToFile(Path path, String data) throws IOException {
-    OutputStream out = path.getOutputStream();
-    out.write(data.getBytes(Charset.defaultCharset()));
-    out.close();
+    try (OutputStream out = path.getOutputStream()) {
+      out.write(data.getBytes(Charset.defaultCharset()));
+    }
   }
 
   /**

diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ByteStreamServer.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ByteStreamServer.java
@@ -100,7 +100,7 @@ public StreamObserver<WriteRequest> write(final StreamObserver<WriteResponse> re
     Path temp = workPath.getRelative("upload").getRelative(UUID.randomUUID().toString());
     try {
       FileSystemUtils.createDirectoryAndParents(temp.getParentDirectory());
-      temp.getOutputStream().close();
+      FileSystemUtils.createEmptyFile(temp);
     } catch (IOException e) {
       logger.log(SEVERE, "Failed to create temporary file for upload", e);
       responseObserver.onError(StatusUtils.internalError(e));