Apply a workaround for a JDK bug unconditionally.

We now always read template resources directly from the jar file containing them, rather than initially trying to use `getResourceAsStream`. That can trigger [JDK-6947916](https://bugs.openjdk.org/browse/JDK-6947916) and our existing fallback-to-workaround logic was ineffective with recent versions of EscapeVelocity. Even though the JDK bug was supposedly fixed in JDK 9 and later JDK 8 updates, people are still reporting issues with AutoValue that look exactly like it.

Reading directly from the jar file should not be _too_ inefficient, and each read should only happen once per compilation, no matter how many `@AutoValue` classes there are in the compilation.

Fixes https://github.com/google/auto/issues/1572.

RELNOTES=A workaround for a JDK bug with reading jar resources has been extended so it always applies, rather than just as a fallback. See #1572.
PiperOrigin-RevId: 563814239

2 files changed, 28 insertions, 209 deletions

diff --git a/value/src/main/java/com/google/auto/value/processor/TemplateVars.java b/value/src/main/java/com/google/auto/value/processor/TemplateVars.java
index d9e3337b..73253629 100644
--- a/value/src/main/java/com/google/auto/value/processor/TemplateVars.java
+++ b/value/src/main/java/com/google/auto/value/processor/TemplateVars.java
@@ -15,27 +15,28 @@
  */
 package com.google.auto.value.processor;
 
-import com.google.common.base.Throwables;
+import static java.nio.charset.StandardCharsets.UTF_8;
+
+import com.google.common.base.Ascii;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.io.ByteStreams;
 import com.google.escapevelocity.Template;
 import java.io.File;
 import java.io.FileInputStream;
-import java.io.FilterInputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.io.Reader;
-import java.io.UnsupportedEncodingException;
+import java.io.StringReader;
+import java.io.UncheckedIOException;
 import java.lang.reflect.Field;
 import java.lang.reflect.Modifier;
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.net.URL;
-import java.nio.charset.StandardCharsets;
 import java.util.Map;
 import java.util.TreeMap;
-import java.util.jar.JarEntry;
 import java.util.jar.JarFile;
 
 /**
@@ -95,7 +96,7 @@ abstract class TemplateVars {
    * concrete subclass of TemplateVars) into the template returned by {@link #parsedTemplate()}.
    */
   String toText() {
-    Map<String, Object> vars = toVars();
+    ImmutableMap<String, Object> vars = toVars();
     return parsedTemplate().evaluate(vars);
   }
 
@@ -121,69 +122,42 @@ abstract class TemplateVars {
 
   static Template parsedTemplateForResource(String resourceName) {
     try {
-      return Template.parseFrom(resourceName, TemplateVars::readerFromResource);
-    } catch (UnsupportedEncodingException e) {
-      throw new AssertionError(e);
-    } catch (IOException | NullPointerException | IllegalStateException e) {
-      // https://github.com/google/auto/pull/439 says that we can get NullPointerException.
-      // https://github.com/google/auto/issues/715 says that we can get IllegalStateException
-      return retryParseAfterException(resourceName, e);
-    }
-  }
-
-  private static Template retryParseAfterException(String resourceName, Exception exception) {
-    try {
       return Template.parseFrom(resourceName, TemplateVars::readerFromUrl);
-    } catch (IOException t) {
-      // Chain the original exception so we can see both problems.
-      Throwables.getRootCause(exception).initCause(t);
-      throw new AssertionError(exception);
+    } catch (IOException e) {
+      throw new UncheckedIOException(e);
     }
   }
 
-  private static Reader readerFromResource(String resourceName) {
-    InputStream in = TemplateVars.class.getResourceAsStream(resourceName);
-    if (in == null) {
-      throw new IllegalArgumentException("Could not find resource: " + resourceName);
-    }
-    return new InputStreamReader(in, StandardCharsets.UTF_8);
-  }
-
   // This is an ugly workaround for https://bugs.openjdk.java.net/browse/JDK-6947916, as
   // reported in https://github.com/google/auto/issues/365.
   // The issue is that sometimes the InputStream returned by JarURLCollection.getInputStream()
   // can be closed prematurely, which leads to an IOException saying "Stream closed".
-  // We catch all IOExceptions, and fall back on logic that opens the jar file directly and
-  // loads the resource from it. Since that doesn't use JarURLConnection, it shouldn't be
-  // susceptible to the same bug. We only use this as fallback logic rather than doing it always,
-  // because jars are memory-mapped by URLClassLoader, so loading a resource in the usual way
-  // through the getResourceAsStream should be a lot more efficient than reopening the jar.
+  // To avoid that issue, we open the jar file directly and load the resource from it. Since that
+  // doesn't use JarURLConnection, it shouldn't be susceptible to the same bug.
   private static Reader readerFromUrl(String resourceName) throws IOException {
     URL resourceUrl = TemplateVars.class.getResource(resourceName);
     if (resourceUrl == null) {
-      // This is unlikely, since getResourceAsStream has already succeeded for the same resource.
       throw new IllegalArgumentException("Could not find resource: " + resourceName);
     }
-    InputStream in;
     try {
-      if (resourceUrl.getProtocol().equalsIgnoreCase("file")) {
-        in = inputStreamFromFile(resourceUrl);
-      } else if (resourceUrl.getProtocol().equalsIgnoreCase("jar")) {
-        in = inputStreamFromJar(resourceUrl);
+      if (Ascii.equalsIgnoreCase(resourceUrl.getProtocol(), "file")) {
+        return readerFromFile(resourceUrl);
+      } else if (Ascii.equalsIgnoreCase(resourceUrl.getProtocol(), "jar")) {
+        return readerFromJar(resourceUrl);
       } else {
-        throw new AssertionError("Template fallback logic fails for: " + resourceUrl);
+        throw new AssertionError("Template search logic fails for: " + resourceUrl);
       }
     } catch (URISyntaxException e) {
       throw new IOException(e);
     }
-    return new InputStreamReader(in, StandardCharsets.UTF_8);
   }
 
-  private static InputStream inputStreamFromJar(URL resourceUrl)
-      throws URISyntaxException, IOException {
+  private static Reader readerFromJar(URL resourceUrl) throws URISyntaxException, IOException {
     // Jar URLs look like this: jar:file:/path/to/file.jar!/entry/within/jar
     // So take apart the URL to open the jar /path/to/file.jar and read the entry
     // entry/within/jar from it.
+    // We could use the methods from JarURLConnection here, but that would risk provoking the same
+    // problem that prompted this workaround in the first place.
     String resourceUrlString = resourceUrl.toString().substring("jar:".length());
     int bang = resourceUrlString.lastIndexOf('!');
     String entryName = resourceUrlString.substring(bang + 1);
@@ -191,28 +165,19 @@ abstract class TemplateVars {
       entryName = entryName.substring(1);
     }
     URI jarUri = new URI(resourceUrlString.substring(0, bang));
-    JarFile jar = new JarFile(new File(jarUri));
-    JarEntry entry = jar.getJarEntry(entryName);
-    InputStream in = jar.getInputStream(entry);
-    // We have to be careful not to close the JarFile before the stream has been read, because
-    // that would also close the stream. So we defer closing the JarFile until the stream is closed.
-    return new FilterInputStream(in) {
-      @Override
-      public void close() throws IOException {
-        super.close();
-        jar.close();
-      }
-    };
+    try (JarFile jar = new JarFile(new File(jarUri));
+        InputStream in = jar.getInputStream(jar.getJarEntry(entryName))) {
+      String contents = new String(ByteStreams.toByteArray(in), UTF_8);
+      return new StringReader(contents);
+    }
   }
 
-  // We don't really expect this case to arise, since the bug we're working around concerns jars
-  // not individual files. However, when running the test for this workaround from Maven, we do
-  // have files. That does mean the test is basically useless there, but Google's internal build
-  // system does run it using a jar, so we do have coverage.
-  private static InputStream inputStreamFromFile(URL resourceUrl)
+  // In most execution environments, we'll be dealing with a jar, but we handle individual files
+  // just for cases like running our tests with Maven.
+  private static Reader readerFromFile(URL resourceUrl)
       throws IOException, URISyntaxException {
     File resourceFile = new File(resourceUrl.toURI());
-    return new FileInputStream(resourceFile);
+    return new InputStreamReader(new FileInputStream(resourceFile), UTF_8);
   }
 
   private static Object fieldValue(Field field, Object container) {
diff --git a/value/src/test/java/com/google/auto/value/processor/TemplateVarsTest.java b/value/src/test/java/com/google/auto/value/processor/TemplateVarsTest.java
index bca7bcab..621a4122 100644
--- a/value/src/test/java/com/google/auto/value/processor/TemplateVarsTest.java
+++ b/value/src/test/java/com/google/auto/value/processor/TemplateVarsTest.java
@@ -15,29 +15,15 @@
  */
 package com.google.auto.value.processor;
 
-import static com.google.common.base.StandardSystemProperty.JAVA_CLASS_PATH;
-import static com.google.common.base.StandardSystemProperty.PATH_SEPARATOR;
 import static com.google.common.truth.Truth.assertThat;
-import static java.util.logging.Level.WARNING;
 import static org.junit.Assert.fail;
 
-import com.google.common.base.Splitter;
 import com.google.common.collect.ImmutableList;
-import com.google.common.reflect.Reflection;
 import com.google.escapevelocity.Template;
-import java.io.File;
 import java.io.IOException;
-import java.io.InputStream;
 import java.io.Reader;
 import java.io.StringReader;
-import java.net.MalformedURLException;
-import java.net.URL;
-import java.net.URLClassLoader;
 import java.util.List;
-import java.util.Set;
-import java.util.TreeSet;
-import java.util.concurrent.Callable;
-import java.util.logging.Logger;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -172,136 +158,4 @@ public class TemplateVarsTest {
     } catch (IllegalArgumentException expected) {
     }
   }
-
-  @Test
-  public void testBrokenInputStream_IOException() throws Exception {
-    doTestBrokenInputStream(new IOException("BrokenInputStream"));
-  }
-
-  @Test
-  public void testBrokenInputStream_NullPointerException() throws Exception {
-    doTestBrokenInputStream(new NullPointerException("BrokenInputStream"));
-  }
-
-  @Test
-  public void testBrokenInputStream_IllegalStateException() throws Exception {
-    doTestBrokenInputStream(new IllegalStateException("BrokenInputStream"));
-  }
-
-  // This is a complicated test that tries to simulates the failures that are worked around in
-  // Template.parsedTemplateForResource. Those failures means that the InputStream returned by
-  // ClassLoader.getResourceAsStream sometimes throws IOException or NullPointerException or
-  // IllegalStateException while it is being read. To simulate that, we make a second ClassLoader
-  // with the same configuration as the one that runs this test, and we override getResourceAsStream
-  // so that it wraps the returned InputStream in a BrokenInputStream, which throws an exception
-  // after a certain number of characters. We check that that exception was indeed seen, and that
-  // we did indeed try to read the resource we're interested in, and that we succeeded in loading a
-  // Template nevertheless.
-  private void doTestBrokenInputStream(Exception exception) throws Exception {
-    URLClassLoader shadowLoader = new ShadowLoader(getClass().getClassLoader(), exception);
-    Runnable brokenInputStreamTest =
-        (Runnable)
-            shadowLoader
-                .loadClass(BrokenInputStreamTest.class.getName())
-                .getConstructor()
-                .newInstance();
-    brokenInputStreamTest.run();
-  }
-
-  private static class ShadowLoader extends URLClassLoader implements Callable<Set<String>> {
-
-    private static final Logger logger = Logger.getLogger(ShadowLoader.class.getName());
-
-    private final Exception exception;
-    private final Set<String> result = new TreeSet<String>();
-
-    ShadowLoader(ClassLoader original, Exception exception) {
-      super(getClassPathUrls(original), original.getParent());
-      this.exception = exception;
-    }
-
-    private static URL[] getClassPathUrls(ClassLoader original) {
-      return original instanceof URLClassLoader
-          ? ((URLClassLoader) original).getURLs()
-          : parseJavaClassPath();
-    }
-
-    /**
-     * Returns the URLs in the class path specified by the {@code java.class.path} {@linkplain
-     * System#getProperty system property}.
-     */
-    // TODO(b/65488446): Use a new public API.
-    private static URL[] parseJavaClassPath() {
-      ImmutableList.Builder<URL> urls = ImmutableList.builder();
-      for (String entry : Splitter.on(PATH_SEPARATOR.value()).split(JAVA_CLASS_PATH.value())) {
-        try {
-          try {
-            urls.add(new File(entry).toURI().toURL());
-          } catch (SecurityException e) { // File.toURI checks to see if the file is a directory
-            urls.add(new URL("file", null, new File(entry).getAbsolutePath()));
-          }
-        } catch (MalformedURLException e) {
-          logger.log(WARNING, "malformed classpath entry: " + entry, e);
-        }
-      }
-      return urls.build().toArray(new URL[0]);
-    }
-
-    @Override
-    public Set<String> call() throws Exception {
-      return result;
-    }
-
-    @Override
-    public InputStream getResourceAsStream(String resource) {
-      // Make sure this is actually the resource we are expecting. If we're using JaCoCo or the
-      // like, we might end up reading some other resource, and we don't want to break that.
-      if (resource.startsWith("com/google/auto")) {
-        result.add(resource);
-        return new BrokenInputStream(super.getResourceAsStream(resource));
-      } else {
-        return super.getResourceAsStream(resource);
-      }
-    }
-
-    private class BrokenInputStream extends InputStream {
-      private final InputStream original;
-      private int count = 0;
-
-      BrokenInputStream(InputStream original) {
-        this.original = original;
-      }
-
-      @Override
-      public int read() throws IOException {
-        if (++count > 10) {
-          result.add("threw");
-          if (exception instanceof IOException) {
-            throw (IOException) exception;
-          }
-          throw (RuntimeException) exception;
-        }
-        return original.read();
-      }
-    }
-  }
-
-  public static class BrokenInputStreamTest implements Runnable {
-    @Override
-    public void run() {
-      Template template = TemplateVars.parsedTemplateForResource("autovalue.vm");
-      assertThat(template).isNotNull();
-      String resourceName =
-          Reflection.getPackageName(getClass()).replace('.', '/') + "/autovalue.vm";
-      @SuppressWarnings("unchecked")
-      Callable<Set<String>> myLoader = (Callable<Set<String>>) getClass().getClassLoader();
-      try {
-        Set<String> result = myLoader.call();
-        assertThat(result).contains(resourceName);
-        assertThat(result).contains("threw");
-      } catch (Exception e) {
-        throw new AssertionError(e);
-      }
-    }
-  }
 }