Do not eagerly load classfiles at startup (DataDog/dd-trace-java#1606)
MODIFY
agent-bootstrap/src/main/java/io/opentelemetry/auto/bootstrap/InternalJarURLHandler.java
agent-bootstrap/src/main/java/io/opentelemetry/auto/bootstrap/InternalJarURLHandler.java
@@ -27,9 +27,6 @@ import java.net.URL;
import java.net.URLConnection;
import java.net.URLStreamHandler;
import java.security.Permission;
-import java.util.Enumeration;
-import java.util.HashMap;
-import java.util.Map;
import java.util.jar.JarEntry;
import java.util.jar.JarFile;
import lombok.extern.slf4j.Slf4j;
@@ -41,7 +38,6 @@ public class InternalJarURLHandler extends URLStreamHandler {
private final String name;
private final FileNotInInternalJar notFound;
- private final Map<String, JarEntry> filenameToEntry = new HashMap<>();
private final JarFile bootstrapJarFile;
private WeakReference<Pair<String, JarEntry>> cache = NULL;
@@ -49,30 +45,15 @@ public class InternalJarURLHandler extends URLStreamHandler {
InternalJarURLHandler(final String internalJarFileName, final URL bootstrapJarLocation) {
name = internalJarFileName;
notFound = new FileNotInInternalJar(internalJarFileName);
- final String filePrefix = internalJarFileName + "/";
JarFile jarFile = null;
try {
if (bootstrapJarLocation != null) {
jarFile = new JarFile(new File(bootstrapJarLocation.toURI()), false);
- final Enumeration<JarEntry> entries = jarFile.entries();
- while (entries.hasMoreElements()) {
- final JarEntry entry = entries.nextElement();
- if (!entry.isDirectory() && entry.getName().startsWith(filePrefix)) {
- String name = entry.getName();
- // remove data suffix
- int end = name.endsWith(".classdata") ? name.length() - 4 : name.length();
- String fileName = name.substring(internalJarFileName.length(), end);
- filenameToEntry.put(fileName, entry);
- }
- }
}
} catch (final URISyntaxException | IOException e) {
log.error("Unable to read internal jar", e);
}
- if (filenameToEntry.isEmpty()) {
- log.warn("No internal jar entries found");
- }
bootstrapJarFile = jarFile;
}
@@ -90,7 +71,7 @@ public class InternalJarURLHandler extends URLStreamHandler {
// and the key will be a new object each time.
Pair<String, JarEntry> pair = cache.get();
if (null == pair || !filename.equals(pair.getLeft())) {
- final JarEntry entry = filenameToEntry.get(filename);
+ JarEntry entry = bootstrapJarFile.getJarEntry(getResourcePath(filename));
if (null != entry) {
pair = Pair.of(filename, entry);
// the cache field is not volatile as a performance optimization
@@ -109,6 +90,20 @@ public class InternalJarURLHandler extends URLStreamHandler {
return new InternalJarURLConnection(url, bootstrapJarFile.getInputStream(pair.getRight()));
}
+ private String getResourcePath(String filename) {
+ boolean isClass = filename.endsWith(".class");
+ int length = name.length() + filename.length();
+ if (isClass) {
+ length += 4; // length of text "data" appended below
+ }
+ StringBuilder sb = new StringBuilder(length);
+ sb.append(name).append(filename);
+ if (isClass) {
+ sb.append("data");
+ }
+ return sb.toString();
+ }
+
private static class InternalJarURLConnection extends URLConnection {
private final InputStream inputStream;

MODIFY
agent-bootstrap/src/test/groovy/io/opentelemetry/auto/bootstrap/InternalJarURLHandlerTest.groovy
agent-bootstrap/src/test/groovy/io/opentelemetry/auto/bootstrap/InternalJarURLHandlerTest.groovy
@@ -43,8 +43,23 @@ class InternalJarURLHandlerTest extends AgentSpecification {
"isolated" | '/a/b/c/C.class'
}
-
- // guards against InternalJarURLHandler caching and re-using the same stream twice
+ // this test serves two purposes:
+ //
+ // (1) Constrain what is cached: I wanted to test that a load does not cache the stream after
+ // releasing it and losing control over its lifecyle.
+ // What's special about the second load which makes it distinct from the first load is that there
+ // was a predecessor.
+ // There is a caching mechanism in place, (the JarEntry is cached with the class name), but what
+ // if someone comes along later and tried to cache something closeable like the stream instead?
+ // That would be unfortunate, but this test would break if that ever happened.
+ //
+ // (2) Hitting the branch where caching has occurred: there happen to be about 800 classloads
+ // early on which go all the way down the chain and end up in the leaf-level classloader twice.
+ // Preventing this from happening would have been a much more invasive change than mitigating it
+ // with a cache.
+ // Unless you test the load twice, you won't test the branch where the cache is hit.
+ // The logic actually dismisses the cache after a hit, based on knowledge of this pattern, making
+ // the third load indistinguishable from the first load, hence the lack of a test for it.
def "test read class twice"() {
setup:
InternalJarURLHandler handler = new InternalJarURLHandler(dir, testJarLocation)

Do not eagerly load classfiles at startup (DataDog/dd-trace-java#1606)