Bazel server, tools: ensure Readers are closed

Follow-up to commit 59f17d6e0550bf63a0b6ef182e2d63474e058ede.

Use try-with-resources to ensure Reader objects
are closed eagerly.

Eagerly closing Readers avoids hanging on to
file handles until the garbage collector finalizes
the object, 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 https://github.com/bazelbuild/bazel/issues/5512

RELNOTES: none
PiperOrigin-RevId: 203771262

9 files changed, 99 insertions, 111 deletions

diff --git a/src/java_tools/singlejar/java/com/google/devtools/build/singlejar/OptionFileExpander.java b/src/java_tools/singlejar/java/com/google/devtools/build/singlejar/OptionFileExpander.java
index 66d4a72579..aa396b5973 100644
--- a/src/java_tools/singlejar/java/com/google/devtools/build/singlejar/OptionFileExpander.java
+++ b/src/java_tools/singlejar/java/com/google/devtools/build/singlejar/OptionFileExpander.java
@@ -18,7 +18,6 @@ import static java.nio.charset.StandardCharsets.ISO_8859_1;
 
 import com.google.devtools.build.lib.shell.ShellUtils;
 import com.google.devtools.build.lib.shell.ShellUtils.TokenizationException;
-
 import java.io.BufferedReader;
 import java.io.IOException;
 import java.io.InputStream;
@@ -26,7 +25,6 @@ import java.io.InputStreamReader;
 import java.io.Reader;
 import java.util.ArrayList;
 import java.util.List;
-
 import javax.annotation.concurrent.Immutable;
 
 /**
@@ -82,11 +80,11 @@ final class OptionFileExpander {
    */
   private void expandArgument(String arg, List<String> expanded) throws IOException {
     if (arg.startsWith("@")) {
-      InputStream in = fileSystem.getInputStream(arg.substring(1));
-      try {
+      try (InputStreamReader reader =
+          new InputStreamReader(fileSystem.getInputStream(arg.substring(1)), ISO_8859_1)) {
         // TODO(bazel-team): This code doesn't handle escaped newlines correctly.
         // ShellUtils doesn't support them either.
-        for (String line : readAllLines(new InputStreamReader(in, ISO_8859_1))) {
+        for (String line : readAllLines(reader)) {
           List<String> parsedTokens = new ArrayList<>();
           try {
             ShellUtils.tokenize(parsedTokens, line);
@@ -97,18 +95,6 @@ final class OptionFileExpander {
             expandArgument(token, expanded);
           }
         }
-        InputStream inToClose = in;
-        in = null;
-        inToClose.close();
-      } finally {
-        if (in != null) {
-          try {
-            in.close();
-          } catch (IOException e) {
-            // Ignore the exception. It can only occur if an exception already
-            // happened and in that case, we want to preserve the original one.
-          }
-        }
       }
     } else {
       expanded.add(arg);
diff --git a/src/java_tools/singlejar/javatests/com/google/devtools/build/singlejar/ZipCombinerTest.java b/src/java_tools/singlejar/javatests/com/google/devtools/build/singlejar/ZipCombinerTest.java
index bb287637ea..2ecf40e1d0 100644
--- a/src/java_tools/singlejar/javatests/com/google/devtools/build/singlejar/ZipCombinerTest.java
+++ b/src/java_tools/singlejar/javatests/com/google/devtools/build/singlejar/ZipCombinerTest.java
@@ -745,17 +745,20 @@ public class ZipCombinerTest {
     byte[] zipCombinerRaw = out.toByteArray();
 
     new ZipTester(zipCombinerRaw).validate();
-    assertZipFilesEquivalent(new ZipReader(zipCombinerFile), new ZipReader(javaFile));
-  }
-
-  void assertZipFilesEquivalent(ZipReader x, ZipReader y) {
-    Collection<ZipFileEntry> xEntries = x.entries();
-    Collection<ZipFileEntry> yEntries = y.entries();
-    assertThat(xEntries).hasSize(yEntries.size());
-    Iterator<ZipFileEntry> xIter = xEntries.iterator();
-    Iterator<ZipFileEntry> yIter = yEntries.iterator();
-    for (int i = 0; i < xEntries.size(); i++) {
-      assertZipEntryEquivalent(xIter.next(), yIter.next());
+    assertZipFilesEquivalent(zipCombinerFile, javaFile);
+  }
+
+  void assertZipFilesEquivalent(File a, File b) throws IOException {
+    try (ZipReader x = new ZipReader(a);
+        ZipReader y = new ZipReader(b)) {
+      Collection<ZipFileEntry> xEntries = x.entries();
+      Collection<ZipFileEntry> yEntries = y.entries();
+      assertThat(xEntries).hasSize(yEntries.size());
+      Iterator<ZipFileEntry> xIter = xEntries.iterator();
+      Iterator<ZipFileEntry> yIter = yEntries.iterator();
+      for (int i = 0; i < xEntries.size(); i++) {
+        assertZipEntryEquivalent(xIter.next(), yIter.next());
+      }
     }
   }
 
diff --git a/src/main/java/com/google/devtools/build/docgen/SourceFileReader.java b/src/main/java/com/google/devtools/build/docgen/SourceFileReader.java
index 17c641e6ea..a6e3e2c62e 100644
--- a/src/main/java/com/google/devtools/build/docgen/SourceFileReader.java
+++ b/src/main/java/com/google/devtools/build/docgen/SourceFileReader.java
@@ -20,10 +20,11 @@ import com.google.common.collect.ListMultimap;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
 import java.io.BufferedReader;
 import java.io.File;
-import java.io.FileReader;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.LinkedList;
@@ -296,19 +297,23 @@ public class SourceFileReader {
     return line;
   }
 
-  public static void readTextFile(String filePath, ReadAction action)
-      throws BuildEncyclopediaDocException, IOException {
-    BufferedReader br = null;
-    try {
-      File file = new File(filePath);
-      if (file.exists()) {
-        br = new BufferedReader(new FileReader(file));
+  private static BufferedReader createReader(String filePath) throws IOException {
+    File file = new File(filePath);
+    if (file.exists()) {
+      return Files.newBufferedReader(file.toPath(), StandardCharsets.UTF_8);
+    } else {
+      InputStream is = SourceFileReader.class.getResourceAsStream(filePath);
+      if (is != null) {
+        return new BufferedReader(new InputStreamReader(is, StandardCharsets.UTF_8));
       } else {
-        InputStream is = SourceFileReader.class.getResourceAsStream(filePath);
-        if (is != null) {
-          br = new BufferedReader(new InputStreamReader(is));
-        }
+        return null;
       }
+    }
+  }
+
+  public static void readTextFile(String filePath, ReadAction action)
+      throws BuildEncyclopediaDocException, IOException {
+    try (BufferedReader br = createReader(filePath)) {
       if (br != null) {
         String line = null;
         while ((line = br.readLine()) != null) {
@@ -317,10 +322,6 @@ public class SourceFileReader {
       } else {
         System.out.println("Couldn't find file or resource: " + filePath);
       }
-    } finally {
-      if (br != null) {
-        br.close();
-      }
     }
   }
 }
diff --git a/src/test/java/com/google/devtools/build/android/AarGeneratorActionTest.java b/src/test/java/com/google/devtools/build/android/AarGeneratorActionTest.java
index ae40dd3592..c5f5323376 100644
--- a/src/test/java/com/google/devtools/build/android/AarGeneratorActionTest.java
+++ b/src/test/java/com/google/devtools/build/android/AarGeneratorActionTest.java
@@ -730,14 +730,16 @@ public class AarGeneratorActionTest {
         aarData.proguardSpecs);
     Set<String> zipEntries = getZipEntries(aar);
     assertThat(zipEntries).contains("proguard.txt");
-    ZipReader aarReader = new ZipReader(aar.toFile());
-    List<String> proguardTxtContents =
-        new BufferedReader(
-                new InputStreamReader(
-                    aarReader.getInputStream(aarReader.getEntry("proguard.txt")),
-                    StandardCharsets.UTF_8))
-            .lines()
-            .collect(Collectors.toList());
+    List<String> proguardTxtContents = null;
+    try (ZipReader aarReader = new ZipReader(aar.toFile())) {
+      proguardTxtContents =
+          new BufferedReader(
+                  new InputStreamReader(
+                      aarReader.getInputStream(aarReader.getEntry("proguard.txt")),
+                      StandardCharsets.UTF_8))
+              .lines()
+              .collect(Collectors.toList());
+    }
     assertThat(proguardTxtContents).containsExactly("foo", "bar", "baz").inOrder();
   }
 }
diff --git a/src/test/java/com/google/devtools/build/android/ziputils/SplitZipTest.java b/src/test/java/com/google/devtools/build/android/ziputils/SplitZipTest.java
index 1794252a22..15f1461196 100644
--- a/src/test/java/com/google/devtools/build/android/ziputils/SplitZipTest.java
+++ b/src/test/java/com/google/devtools/build/android/ziputils/SplitZipTest.java
@@ -21,6 +21,7 @@ import com.google.common.base.Predicates;
 import com.google.common.collect.ImmutableSet;
 import java.io.FileNotFoundException;
 import java.io.IOException;
+import java.io.InputStream;
 import java.util.Arrays;
 import java.util.Date;
 import org.junit.Before;
@@ -423,15 +424,17 @@ public class SplitZipTest {
       String classFileList = "pkg1/test1.class\npkg2/test2.class\n";
       fileSystem.addFile("main_dex_list.txt", classFileList);
 
-      new SplitZip()
-          .addOutput(new ZipOut(fileSystem.getOutputChannel("out/shard1.jar", false),
-              "out/shard1.jar"))
-          .addOutput(new ZipOut(fileSystem.getOutputChannel("out/shard2.jar", false),
-              "out/shard2.jar"))
-          .setMainClassListFile(fileSystem.getInputStream("main_dex_list.txt"))
-          .addInput(new ZipIn(fileSystem.getInputChannel("input.jar"), "input.jar"))
-          .run()
-          .close();
+      try (InputStream mainDex = fileSystem.getInputStream("main_dex_list.txt")) {
+        new SplitZip()
+            .addOutput(new ZipOut(fileSystem.getOutputChannel("out/shard1.jar", false),
+                "out/shard1.jar"))
+            .addOutput(new ZipOut(fileSystem.getOutputChannel("out/shard2.jar", false),
+                "out/shard2.jar"))
+            .setMainClassListStreamForTesting(mainDex)
+            .addInput(new ZipIn(fileSystem.getInputChannel("input.jar"), "input.jar"))
+            .run()
+            .close();
+      }
 
       new ZipFileBuilder()
           .add("pkg1/test1.class", "hello world")
@@ -531,20 +534,22 @@ public class SplitZipTest {
     String classFileList = "pkg1/test1.class\npkg2/test2.class\n";
     fileSystem.addFile("main_dex_list.txt", classFileList);
 
-    new SplitZip()
-        .addOutput(new ZipOut(fileSystem.getOutputChannel("out/shard1.jar", false),
-            "out/shard1.jar"))
-        .addOutput(new ZipOut(fileSystem.getOutputChannel("out/shard2.jar", false),
-            "out/shard2.jar"))
-        .setVerbose(true)
-        .setMainClassListFile(fileSystem.getInputStream("main_dex_list.txt"))
-        .addInput(new ZipIn(fileSystem.getInputChannel("input.jar"), "input.jar"))
-        .setInputFilter(
-            Predicates.in(
-                ImmutableSet.of("pkg1/test1.class", "pkg2/test1.class", "pkg3/test1.class")))
-        .setSplitDexedClasses(true)
-        .run()
-        .close();
+    try (InputStream mainDex = fileSystem.getInputStream("main_dex_list.txt")) {
+      new SplitZip()
+          .addOutput(new ZipOut(fileSystem.getOutputChannel("out/shard1.jar", false),
+              "out/shard1.jar"))
+          .addOutput(new ZipOut(fileSystem.getOutputChannel("out/shard2.jar", false),
+              "out/shard2.jar"))
+          .setVerbose(true)
+          .setMainClassListStreamForTesting(mainDex)
+          .addInput(new ZipIn(fileSystem.getInputChannel("input.jar"), "input.jar"))
+          .setInputFilter(
+              Predicates.in(
+                  ImmutableSet.of("pkg1/test1.class", "pkg2/test1.class", "pkg3/test1.class")))
+          .setSplitDexedClasses(true)
+          .run()
+          .close();
+    }
 
     // 1st shard contains only main dex list classes also in the filter
     new ZipFileBuilder()
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HashInputStreamTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HashInputStreamTest.java
index c1e14df907..3c97cf30f7 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HashInputStreamTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HashInputStreamTest.java
@@ -39,29 +39,30 @@ public class HashInputStreamTest {
 
   @Test
   public void validChecksum_readsOk() throws Exception {
-    assertThat(
-            CharStreams.toString(
-                new InputStreamReader(
-                    new HashInputStream(
-                        new ByteArrayInputStream("hello".getBytes(UTF_8)),
-                        Hashing.sha1(),
-                        HashCode.fromString("aaf4c61ddcc5e8a2dabede0f3b482cd9aea9434d")),
-                    UTF_8)))
-        .isEqualTo("hello");
+    try (InputStreamReader reader =
+        new InputStreamReader(
+            new HashInputStream(
+                new ByteArrayInputStream("hello".getBytes(UTF_8)),
+                Hashing.sha1(),
+                HashCode.fromString("aaf4c61ddcc5e8a2dabede0f3b482cd9aea9434d")),
+            UTF_8)) {
+      assertThat(CharStreams.toString(reader)).isEqualTo("hello");
+    }
   }
 
   @Test
   public void badChecksum_throwsIOException() throws Exception {
     thrown.expect(IOException.class);
     thrown.expectMessage("Checksum");
-    assertThat(
-            CharStreams.toString(
-                new InputStreamReader(
-                    new HashInputStream(
-                        new ByteArrayInputStream("hello".getBytes(UTF_8)),
-                        Hashing.sha1(),
-                        HashCode.fromString("0000000000000000000000000000000000000000")),
-                    UTF_8)))
-        .isNull();  // Only here to make @CheckReturnValue happy.
+    try (InputStreamReader reader =
+        new InputStreamReader(
+            new HashInputStream(
+                new ByteArrayInputStream("hello".getBytes(UTF_8)),
+                Hashing.sha1(),
+                HashCode.fromString("0000000000000000000000000000000000000000")),
+            UTF_8)) {
+      assertThat(CharStreams.toString(reader))
+          .isNull(); // Only here to make @CheckReturnValue happy.
+    }
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/JsonFormatFileTransportTest.java b/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/JsonFormatFileTransportTest.java
index 656f87c289..83ce626c22 100644
--- a/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/JsonFormatFileTransportTest.java
+++ b/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/JsonFormatFileTransportTest.java
@@ -29,7 +29,6 @@ import com.google.devtools.common.options.Options;
 import com.google.protobuf.util.JsonFormat;
 import java.io.File;
 import java.io.FileInputStream;
-import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.io.Reader;
 import org.junit.After;
@@ -82,8 +81,7 @@ public class JsonFormatFileTransportTest {
     transport.sendBuildEvent(buildEvent, artifactGroupNamer);
 
     transport.close().get();
-    try (InputStream in = new FileInputStream(output)) {
-      Reader reader = new InputStreamReader(in);
+    try (Reader reader = new InputStreamReader(new FileInputStream(output))) {
       JsonFormat.Parser parser = JsonFormat.parser();
       BuildEventStreamProtos.BuildEvent.Builder builder =
           BuildEventStreamProtos.BuildEvent.newBuilder();
diff --git a/src/tools/android/java/com/google/devtools/build/android/incrementaldeployment/StubApplication.java b/src/tools/android/java/com/google/devtools/build/android/incrementaldeployment/StubApplication.java
index 0b91d03207..5cf15fad73 100644
--- a/src/tools/android/java/com/google/devtools/build/android/incrementaldeployment/StubApplication.java
+++ b/src/tools/android/java/com/google/devtools/build/android/incrementaldeployment/StubApplication.java
@@ -434,8 +434,7 @@ public class StubApplication extends Application {
 
   private static Map<String, String> parseManifest(File file) throws IOException {
     Map<String, String> result = new HashMap<>();
-    BufferedReader reader = new BufferedReader(new FileReader(file));
-    try {
+    try (BufferedReader reader = new BufferedReader(new FileReader(file))) {
       while (true) {
         String line = reader.readLine();
         if (line == null) {
@@ -445,8 +444,6 @@ public class StubApplication extends Application {
         String[] items = line.split(" ");
         result.put(items[0], items[1]);
       }
-    } finally {
-      reader.close();
     }
 
     return result;
diff --git a/src/tools/android/java/com/google/devtools/build/android/ziputils/SplitZip.java b/src/tools/android/java/com/google/devtools/build/android/ziputils/SplitZip.java
index 7a1f81a7e9..25a32a05cc 100644
--- a/src/tools/android/java/com/google/devtools/build/android/ziputils/SplitZip.java
+++ b/src/tools/android/java/com/google/devtools/build/android/ziputils/SplitZip.java
@@ -118,7 +118,7 @@ public class SplitZip implements EntryHandler {
   }
 
   // Package private for testing with mock file
-  SplitZip setMainClassListFile(InputStream clInputStream) {
+  SplitZip setMainClassListStreamForTesting(InputStream clInputStream) {
     filterInputStream = clInputStream;
     return this;
   }
@@ -446,21 +446,16 @@ public class SplitZip implements EntryHandler {
    */
   private Set<String> readPaths(String fileName) throws IOException {
     Set<String> paths = new HashSet<>();
-    BufferedReader reader = null;
-    try {
-      if (filterInputStream == null) {
-        filterInputStream = new FileInputStream(fileName);
-      }
-      reader = new BufferedReader(new InputStreamReader(filterInputStream, UTF_8));
+    if (filterInputStream == null) {
+      filterInputStream = new FileInputStream(fileName);
+    }
+    try (BufferedReader reader =
+        new BufferedReader(new InputStreamReader(filterInputStream, UTF_8))) {
       String line;
       while (null != (line = reader.readLine())) {
         paths.add(fixPath(line));
       }
       return paths;
-    } finally {
-      if (reader != null) {
-        reader.close();
-      }
     }
   }