From ac734ac6eee763979933477e5359622e6edb4c38 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Mon, 15 Apr 2019 17:46:28 -0700 Subject: [PATCH] More classloading issues with Glassfish MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Glassfish’s WebappClassLoader caches when a resource or class load fails, so we can’t load as a resource first to see if it is available. Also add additional logging. --- .../agent/tooling/DDLocationStrategy.java | 10 +++---- .../agent/tooling/ExceptionHandlers.java | 9 ++++++- .../trace/agent/tooling/Instrumenter.java | 2 +- .../classloading/ClassLoadingTest.groovy | 27 ++++++++++++++----- 4 files changed, 34 insertions(+), 14 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDLocationStrategy.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDLocationStrategy.java index 56955e001e..7a9ce42179 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDLocationStrategy.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDLocationStrategy.java @@ -7,23 +7,23 @@ import net.bytebuddy.dynamic.ClassFileLocator; import net.bytebuddy.utility.JavaModule; /** - * Locate resources with the loading classloader. If the loading classloader cannot find the desired - * resource, check up the classloader hierarchy until a resource is found or the bootstrap loader is - * reached. + * Locate resources with the loading classloader. Because of a quirk with the way classes appended + * to the bootstrap classpath work, we first check our bootstrap proxy. If the loading classloader + * cannot find the desired resource, check up the classloader hierarchy until a resource is found. */ public class DDLocationStrategy implements AgentBuilder.LocationStrategy { - public ClassFileLocator classFileLocator(ClassLoader classLoader) { + public ClassFileLocator classFileLocator(final ClassLoader classLoader) { return classFileLocator(classLoader, null); } @Override public ClassFileLocator classFileLocator(ClassLoader classLoader, final JavaModule javaModule) { final List locators = new ArrayList<>(); + locators.add(ClassFileLocator.ForClassLoader.of(Utils.getBootstrapProxy())); while (classLoader != null) { locators.add(ClassFileLocator.ForClassLoader.WeaklyReferenced.of(classLoader)); classLoader = classLoader.getParent(); } - locators.add(ClassFileLocator.ForClassLoader.of(Utils.getBootstrapProxy())); return new ClassFileLocator.Compound(locators.toArray(new ClassFileLocator[0])); } } diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/ExceptionHandlers.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/ExceptionHandlers.java index 209c2ab674..49db4f099f 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/ExceptionHandlers.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/ExceptionHandlers.java @@ -31,6 +31,9 @@ public class ExceptionHandlers { @Override public Size apply(final MethodVisitor mv, final Implementation.Context context) { + final String name = context.getInstrumentedType().getName(); + final ClassLoader classLoader = Thread.currentThread().getContextClassLoader(); + // writes the following bytecode: // try { // org.slf4j.LoggerFactory.getLogger((Class)ExceptionLogger.class) @@ -58,7 +61,11 @@ public class ExceptionHandlers { "(Ljava/lang/Class;)L" + LOGGER_NAME + ";", false); mv.visitInsn(Opcodes.SWAP); // stack: (top) throwable,logger - mv.visitLdcInsn("Failed to handle exception in instrumentation"); + mv.visitLdcInsn( + "Failed to handle exception in instrumentation for " + + name + + " on " + + classLoader); mv.visitInsn(Opcodes.SWAP); // stack: (top) throwable,string,logger mv.visitMethodInsn( Opcodes.INVOKEINTERFACE, diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java index 3fc7e93bf4..5f6be30f90 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java @@ -98,7 +98,7 @@ public interface Instrumenter { agentBuilder = agentBuilder.transform( new AgentBuilder.Transformer.ForAdvice() - .include(Utils.getAgentClassLoader()) + .include(Utils.getBootstrapProxy(), Utils.getAgentClassLoader()) .withExceptionHandler(ExceptionHandlers.defaultExceptionHandler()) .advice(entry.getKey(), entry.getValue())); } diff --git a/dd-java-agent/src/test/groovy/datadog/trace/agent/integration/classloading/ClassLoadingTest.groovy b/dd-java-agent/src/test/groovy/datadog/trace/agent/integration/classloading/ClassLoadingTest.groovy index 883730a769..d292fd0160 100644 --- a/dd-java-agent/src/test/groovy/datadog/trace/agent/integration/classloading/ClassLoadingTest.groovy +++ b/dd-java-agent/src/test/groovy/datadog/trace/agent/integration/classloading/ClassLoadingTest.groovy @@ -1,17 +1,15 @@ package datadog.trace.agent.integration.classloading +import static datadog.trace.agent.test.IntegrationTestUtils.createJarWithClasses + import datadog.test.ClassToInstrument import datadog.test.ClassToInstrumentChild -import datadog.trace.agent.test.IntegrationTestUtils import datadog.trace.api.Trace import datadog.trace.util.gc.GCUtils +import java.lang.ref.WeakReference import spock.lang.Specification import spock.lang.Timeout -import java.lang.ref.WeakReference - -import static datadog.trace.agent.test.IntegrationTestUtils.createJarWithClasses - @Timeout(10) class ClassLoadingTest extends Specification { @@ -94,8 +92,23 @@ class ClassLoadingTest extends Specification { loader1.count == loader2.count } - def "can find bootstrap resources"() { + def "can find classes but not resources loaded onto the bootstrap classpath"() { expect: - IntegrationTestUtils.getAgentClassLoader().getResources('datadog/trace/api/Trace.class') != null + Class.forName(name) != null + + // Resources from bootstrap injected jars can't be loaded. + // https://github.com/raphw/byte-buddy/pull/496 + if (onTestClasspath) { + assert ClassLoader.getSystemClassLoader().getResource(resource) != null + } else { + assert ClassLoader.getSystemClassLoader().getResource(resource) == null + } + + + where: + name | onTestClasspath + "datadog.trace.api.Trace" | true + "datadog.trace.bootstrap.instrumentation.java.concurrent.State" | false + resource = name.replace(".", "/") + ".class" } }