From 3f69cd2551a72828ef772a22d31e23e061dd0e79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89amonn=20McManus?= Date: Fri, 8 Sep 2023 11:57:52 -0700 Subject: [PATCH] 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 --- .../auto/value/processor/TemplateVars.java | 91 ++++------- .../value/processor/TemplateVarsTest.java | 146 ------------------ 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 d9e3337ba0..73253629dd 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 @@ private static void addFields( * concrete subclass of TemplateVars) into the template returned by {@link #parsedTemplate()}. */ String toText() { - Map vars = toVars(); + ImmutableMap vars = toVars(); return parsedTemplate().evaluate(vars); } @@ -120,70 +121,43 @@ public String toString() { } 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 @@ private static InputStream inputStreamFromJar(URL resourceUrl) 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 bca7bcab18..621a41220e 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 void testPrimitive() { } 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> { - - private static final Logger logger = Logger.getLogger(ShadowLoader.class.getName()); - - private final Exception exception; - private final Set result = new TreeSet(); - - 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 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 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> myLoader = (Callable>) getClass().getClassLoader(); - try { - Set result = myLoader.call(); - assertThat(result).contains(resourceName); - assertThat(result).contains("threw"); - } catch (Exception e) { - throw new AssertionError(e); - } - } - } }