Merge pull request #417 from DataDog/mar-kolya/apache-http-client-fix…

…-exception-handling

Apache http client fix exception handling

dd-java-agent-ittests/src/test/groovy/datadog/trace/agent/integration/classloading/ClassLoadingTest.groovy

@@ -1,17 +1,22 @@
 package datadog.trace.agent.integration.classloading
 
+import datadog.test.ClassToInstrument
+import datadog.test.ClassToInstrumentChild
 import datadog.trace.agent.test.IntegrationTestUtils
 import datadog.trace.api.Trace
 import spock.lang.Specification
 
+import java.lang.ref.WeakReference
+
 import static datadog.trace.agent.test.IntegrationTestUtils.createJarWithClasses
 
 class ClassLoadingTest extends Specification {
 
+  final URL[] classpath = [createJarWithClasses(ClassToInstrument, ClassToInstrumentChild, Trace)]
+
   /** Assert that we can instrument classloaders which cannot resolve agent advice classes. */
   def "instrument classloader without agent classes"() {
     setup:
-    final URL[] classpath = [createJarWithClasses(ClassToInstrument, Trace)]
     final URLClassLoader loader = new URLClassLoader(classpath, (ClassLoader) null)
 
     when:
@@ -25,6 +30,61 @@ class ClassLoadingTest extends Specification {
     instrumentedClass.getClassLoader() == loader
   }
 
+  def "make sure ByteBuddy does not hold strong references to ClassLoader"() {
+    setup:
+    final URLClassLoader loader = new URLClassLoader(classpath, (ClassLoader) null)
+    final WeakReference<URLClassLoader> ref = new WeakReference<>(loader)
+
+    when:
+    loader.loadClass(ClassToInstrument.getName())
+    loader = null
+
+    IntegrationTestUtils.awaitGC()
+
+    then:
+    null == ref.get()
+  }
+
+  // We are doing this because Grovy cannot properly resolve constructor argument types in anonymous classes
+  static class CountingClassLoader extends URLClassLoader {
+    public int count = 0
+
+    CountingClassLoader(URL[] urls) {
+      super(urls, (ClassLoader) null)
+    }
+
+    @Override
+    URL getResource(String name) {
+      count++
+    }
+  }
+
+  def "make sure that ByteBuddy reads classes's bytes only once"() {
+    setup:
+    final CountingClassLoader loader = new CountingClassLoader(classpath)
+
+    when:
+    loader.loadClass(ClassToInstrument.getName())
+    loader.loadClass(ClassToInstrumentChild.getName())
+
+    then:
+    loader.count == 2
+  }
+
+  def "make sure that ByteBuddy doesn't resue cached type descriptions between different classloaders"() {
+    setup:
+    final CountingClassLoader loader1 = new CountingClassLoader(classpath)
+    final CountingClassLoader loader2 = new CountingClassLoader(classpath)
+
+    when:
+    loader1.loadClass(ClassToInstrument.getName())
+    loader2.loadClass(ClassToInstrument.getName())
+
+    then:
+    loader1.count == 1
+    loader2.count == 1
+  }
+
   def "can find bootstrap resources"() {
     expect:
     IntegrationTestUtils.getAgentClassLoader().getResources('datadog/trace/api/Trace.class') != null

dd-java-agent-ittests/src/test/java/datadog/test/ClassToInstrument.java

@@ -0,0 +1,9 @@
+package datadog.test;
+
+import datadog.trace.api.Trace;
+
+/** Note: this has to stay in 'datadog.test' package to be considered for instrumentation */
+public class ClassToInstrument {
+  @Trace
+  public static void someMethod() {}
+}

dd-java-agent-ittests/src/test/java/datadog/test/ClassToInstrumentChild.java

@@ -0,0 +1,4 @@
+package datadog.test;
+
+/** Note: this has to stay in 'datadog.test' package to be considered for instrumentation */
+public class ClassToInstrumentChild extends ClassToInstrument {}

dd-java-agent-ittests/src/test/java/datadog/trace/agent/test/IntegrationTestUtils.java

@@ -7,6 +7,7 @@
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
+import java.lang.ref.WeakReference;
 import java.lang.reflect.Field;
 import java.net.URL;
 import java.util.UUID;
@@ -156,4 +157,14 @@ public static String[] getAgentPackagePrefixes() throws Exception {
             .getField("AGENT_PACKAGE_PREFIXES");
     return (String[]) f.get(null);
   }
+
+  public static void awaitGC() {
+    System.gc(); // For good measure.
+    Object obj = new Object();
+    final WeakReference ref = new WeakReference<>(obj);
+    obj = null;
+    while (ref.get() != null) {
+      System.gc();
+    }
+  }
 }

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java

@@ -42,6 +42,7 @@ public static ResettableClassFileTransformer installBytebuddyAgent(
         new AgentBuilder.Default()
             .disableClassFormatChanges()
             .with(AgentBuilder.RedefinitionStrategy.RETRANSFORMATION)
+            .with(AgentBuilder.DescriptionStrategy.Default.POOL_ONLY)
             .with(new LoggingListener())
             .with(new DDLocationStrategy())
             .ignore(any(), skipClassLoader())

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/ByteBuddyElementMatchers.java

@@ -0,0 +1,143 @@
+package datadog.trace.agent.tooling;
+
+import static net.bytebuddy.matcher.ElementMatchers.erasure;
+
+import java.util.HashSet;
+import java.util.Set;
+import lombok.extern.slf4j.Slf4j;
+import net.bytebuddy.build.HashCodeAndEqualsPlugin;
+import net.bytebuddy.description.type.TypeDefinition;
+import net.bytebuddy.description.type.TypeDescription;
+import net.bytebuddy.description.type.TypeList;
+import net.bytebuddy.matcher.ElementMatcher;
+import net.bytebuddy.matcher.ElementMatchers;
+
+/**
+ * This class provides some custom ByteBuddy element matchers to use when applying instrumentation
+ */
+@Slf4j
+public class ByteBuddyElementMatchers {
+
+  /**
+   * Matches any type description that declares a super type that matches the provided matcher.
+   * Exceptions during matching process are logged and ignored.
+   *
+   * @param matcher The type to be checked for being a super type of the matched type.
+   * @param <T> The type of the matched object.
+   * @return A matcher that matches any type description that declares a super type that matches the
+   *     provided matcher.
+   * @see ElementMatchers#hasSuperType(net.bytebuddy.matcher.ElementMatcher)
+   */
+  public static <T extends TypeDescription> ElementMatcher.Junction<T> safeHasSuperType(
+      final ElementMatcher<? super TypeDescription> matcher) {
+    return safeHasGenericSuperType(erasure(matcher));
+  }
+
+  /**
+   * Matches any type description that declares a super type that matches the provided matcher.
+   * Exceptions during matching process are logged and ignored.
+   *
+   * @param matcher The type to be checked for being a super type of the matched type.
+   * @param <T> The type of the matched object.
+   * @return A matcher that matches any type description that declares a super type that matches the
+   *     provided matcher.
+   * @see ElementMatchers#hasGenericSuperType(net.bytebuddy.matcher.ElementMatcher)
+   */
+  public static <T extends TypeDescription> ElementMatcher.Junction<T> safeHasGenericSuperType(
+      final ElementMatcher<? super TypeDescription.Generic> matcher) {
+    return new SafeHasSuperTypeMatcher<>(matcher);
+  }
+
+  /**
+   * An element matcher that matches a super type. This is different from {@link
+   * net.bytebuddy.matcher.HasSuperTypeMatcher} in the following way:
+   *
+   * <ul>
+   *   <li>Exceptions are logged
+   *   <li>When exception happens the rest of the inheritance subtree is discarded (since ByteBuddy
+   *       cannot load/parse type information for it) but search in other subtrees continues
+   * </ul>
+   *
+   * <p>This is useful because this allows us to see when matcher's check is not complete (i.e. part
+   * of it fails), at the same time it makes best effort instead of failing quickly (like {@code
+   * failSafe(hasSuperType(...))} does) which means the code is more resilient to classpath
+   * inconsistencies
+   *
+   * @param <T> The type of the matched entity.
+   * @see net.bytebuddy.matcher.HasSuperTypeMatcher
+   */
+  @HashCodeAndEqualsPlugin.Enhance
+  public static class SafeHasSuperTypeMatcher<T extends TypeDescription>
+      extends ElementMatcher.Junction.AbstractBase<T> {
+
+    /** The matcher to apply to any super type of the matched type. */
+    private final ElementMatcher<? super TypeDescription.Generic> matcher;
+
+    /**
+     * Creates a new matcher for a super type.
+     *
+     * @param matcher The matcher to apply to any super type of the matched type.
+     */
+    public SafeHasSuperTypeMatcher(final ElementMatcher<? super TypeDescription.Generic> matcher) {
+      this.matcher = matcher;
+    }
+
+    @Override
+    public boolean matches(final T target) {
+      final Set<TypeDescription> checkedInterfaces = new HashSet<>();
+      // We do not use foreach loop and iterator interface here because we need to catch exceptions
+      // in {@code getSuperClass} calls
+      TypeDefinition typeDefinition = target;
+      while (typeDefinition != null) {
+        if (matcher.matches(typeDefinition.asGenericType())
+            || hasInterface(typeDefinition, checkedInterfaces)) {
+          return true;
+        }
+        typeDefinition = safeGetSuperClass(typeDefinition);
+      }
+      return false;
+    }
+
+    private TypeDefinition safeGetSuperClass(final TypeDefinition typeDefinition) {
+      try {
+        return typeDefinition.getSuperClass();
+      } catch (final Exception e) {
+        log.info("Exception trying to get next type definition:", e);
+        return null;
+      }
+    }
+
+    /**
+     * Matches a type's interfaces against the provided matcher.
+     *
+     * @param typeDefinition The type for which to check all implemented interfaces.
+     * @param checkedInterfaces The interfaces that have already been checked.
+     * @return {@code true} if any interface matches the supplied matcher.
+     */
+    private boolean hasInterface(
+        final TypeDefinition typeDefinition, final Set<TypeDescription> checkedInterfaces) {
+      for (final TypeDefinition interfaceType : safeGetInterfaces(typeDefinition)) {
+        if (checkedInterfaces.add(interfaceType.asErasure())
+            && (matcher.matches(interfaceType.asGenericType())
+                || hasInterface(interfaceType, checkedInterfaces))) {
+          return true;
+        }
+      }
+      return false;
+    }
+
+    private TypeList.Generic safeGetInterfaces(final TypeDefinition typeDefinition) {
+      try {
+        return typeDefinition.getInterfaces();
+      } catch (final Exception e) {
+        log.info("Exception trying to get interfaces:", e);
+        return new TypeList.Generic.Empty();
+      }
+    }
+
+    @Override
+    public String toString() {
+      return "safeHasSuperType(" + matcher + ")";
+    }
+  }
+}

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDLocationStrategy.java

@@ -12,16 +12,14 @@
  * reached.
  */
 public class DDLocationStrategy implements AgentBuilder.LocationStrategy {
-  private static final ClassLoader BOOTSTRAP_RESOURCE_LOCATOR = new ClassLoader(null) {};
-
   @Override
-  public ClassFileLocator classFileLocator(ClassLoader classLoader, JavaModule javaModule) {
-    List<ClassFileLocator> locators = new ArrayList<ClassFileLocator>();
+  public ClassFileLocator classFileLocator(ClassLoader classLoader, final JavaModule javaModule) {
+    final List<ClassFileLocator> locators = new ArrayList<>();
     while (classLoader != null) {
       locators.add(ClassFileLocator.ForClassLoader.of(classLoader));
       classLoader = classLoader.getParent();
     }
-    locators.add(ClassFileLocator.ForClassLoader.of(BOOTSTRAP_RESOURCE_LOCATOR));
+    locators.add(ClassFileLocator.ForClassLoader.of(Utils.getBootstrapProxy()));
     return new ClassFileLocator.Compound(locators.toArray(new ClassFileLocator[0]));
   }
 }

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/HelperInjector.java

@@ -30,9 +30,10 @@ public class HelperInjector implements Transformer {
    * Construct HelperInjector.
    *
    * @param helperClassNames binary names of the helper classes to inject. These class names must be
-   *     resolvable by the classloader returned by DDAdvice#getAgentClassLoader(). Classes are
-   *     injected in the order provided. This is important if there is interdependency between
-   *     helper classes that requires them to be injected in a specific order.
+   *     resolvable by the classloader returned by
+   *     datadog.trace.agent.tooling.Utils#getAgentClassLoader(). Classes are injected in the order
+   *     provided. This is important if there is interdependency between helper classes that
+   *     requires them to be injected in a specific order.
    */
   public HelperInjector(final String... helperClassNames) {
     this.helperClassNames = new LinkedHashSet<>(Arrays.asList(helperClassNames));