Skip to content

Commit

Permalink
Apply a workaround for a JDK bug unconditionally.
Browse files Browse the repository at this point in the history 
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 #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
  • Loading branch information
@eamonnmcmanus
eamonnmcmanus authored and Google Java Core Libraries committed Sep 8, 2023
1 parent 389b6e7 commit 3f69cd2
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 209 deletions.
91 changes: 28 additions & 63 deletions value/src/main/java/com/google/auto/value/processor/TemplateVars.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -95,7 +96,7 @@ private static void addFields(
* 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);
}

Expand All @@ -120,99 +121,63 @@ 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);
if (entryName.startsWith("/")) {
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) {
Expand Down
146 changes: 0 additions & 146 deletions value/src/test/java/com/google/auto/value/processor/TemplateVarsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<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);
}
}
}
}

0 comments on commit 3f69cd2

Please sign in to comment.