Fix ClassLatch never latching on Java 8 (quick fix)

internal-api/src/main/java/datadog/trace/util/ClassLatch.java

@@ -3,6 +3,9 @@
 import datadog.trace.api.function.Strategy;
 import datadog.trace.api.function.StrategyConsumer;
 import datadog.trace.api.function.ThrowingFunction;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+import java.util.Arrays;
 import javax.annotation.Nullable;
 import javax.annotation.concurrent.ThreadSafe;
 
@@ -143,10 +146,11 @@ protected final void unlatch(T target) {
 
   /**
    * Latches the target's key if the error's message names that class as the receiver that lacks
-   * {@code methodName}. Returns whether it latched. An error that does not name the key, for
-   * example one thrown inside a wrapper's delegate, is left alone — and so is one that names the
-   * key but for a different method, for example one the guarded method's own implementation calls
-   * internally.
+   * {@code methodName}, or, when the JVM gave no message, if that class still has {@code
+   * methodName} as an abstract method. Returns whether it latched. An error that does not name the
+   * key, for example one thrown inside a wrapper's delegate, is left alone — and so is one that
+   * names the key but for a different method, for example one the guarded method's own
+   * implementation calls internally.
    */
   protected final boolean latchIfNamed(T target, String methodName, AbstractMethodError error) {
     if (isNamedIn(error, keyOf(target), methodName)) {
@@ -159,10 +163,10 @@ protected final boolean latchIfNamed(T target, String methodName, AbstractMethod
   /**
    * For a call to a method that some implementations may lack: returns {@link #fallback} if the
    * call raises {@link AbstractMethodError} or {@link UnsupportedOperationException}, latching the
-   * key first in the former case if, and only if, the error names it (see {@link #latchIfNamed}).
-   * An error that does not name the key still yields the fallback; it just is not latched. An
-   * unsupported operation names no class, so it is never latched, and is caught on every call.
-   * Anything else, checked exceptions included, propagates unchanged.
+   * key first in the former case if, and only if, the error is attributed to it (see {@link
+   * #latchIfNamed}). An error that is not attributed to the key still yields the fallback; it just
+   * is not latched. An unsupported operation names no class, so it is never latched, and is caught
+   * on every call. Anything else, checked exceptions included, propagates unchanged.
    *
    * <pre>{@code
    * protected Properties apply(Connection c) throws SQLException {
@@ -236,8 +240,9 @@ protected final R handleNoSuchMethod(T target, @Strategy ThrowingFunction<T, R,
    * classes ({@link AbstractMethodError}): both yield {@link #fallback}, as does {@link
    * UnsupportedOperationException}. This is {@link #handleAbstractMethod} and {@link
    * #handleNoSuchMethod} together, with the same latching rules: an {@link AbstractMethodError}
-   * latches only if its message names the key, and a {@link NoSuchMethodError} latches the target's
-   * key. The two are easy to confuse, so prefer this one unless you know which a call site can see.
+   * latches only if it is attributed to the key (see {@link #latchIfNamed}), and a {@link
+   * NoSuchMethodError} latches the target's key. The two are easy to confuse, so prefer this one
+   * unless you know which a call site can see.
    *
    * <pre>{@code
    * protected Properties apply(Connection c) throws SQLException {
@@ -257,6 +262,51 @@ protected final R handleNoSuchOrAbstractMethod(
     }
   }
 
+  /**
+   * Whether a public method named {@code methodName} is still abstract on {@code type}, as an
+   * interface method that a concrete class never implemented is. This attributes the error to the
+   * class just as a message naming it would: a wrapper that delegates the method has a concrete
+   * implementation, so an error raised by its delegate is not attributed to it. Overloads are not
+   * told apart. Each class is scanned once, whatever the answer, so a class that keeps failing
+   * without being latched does not repeat the reflection.
+   */
+  static boolean lacksImplementation(Class<?> type, String methodName) {
+    for (final String abstractMethod : ABSTRACT_METHOD_NAMES.get(type)) {
+      if (abstractMethod.equals(methodName)) {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  private static final String[] NO_NAMES = new String[0];
+
+  /**
+   * Per class, the names of its public methods that are still abstract. Only computed for classes
+   * that failed without a message. The value is a JDK type, like {@link #latched}'s.
+   */
+  private static final ClassValue<String[]> ABSTRACT_METHOD_NAMES =
+      new ClassValue<String[]>() {
+        @Override
+        protected String[] computeValue(Class<?> type) {
+          try {
+            int count = 0;
+            final Method[] methods = type.getMethods();
+            final String[] names = new String[methods.length];
+            for (final Method method : methods) {
+              if (Modifier.isAbstract(method.getModifiers())) {
+                names[count++] = method.getName();
+              }
+            }
+            return count == 0 ? NO_NAMES : Arrays.copyOf(names, count);
+          } catch (final SecurityException | LinkageError ignored) {
+            // cannot tell, so never latch: getMethods() resolves every public signature, and one
+            // that references a class missing from the class path raises NoClassDefFoundError
+            return NO_NAMES;
+          }
+        }
+      };
+
   /**
    * Matches the receiver class named by HotSpot's message against a class. Two formats exist:
    *
@@ -271,11 +321,15 @@ protected final R handleNoSuchOrAbstractMethod(
    * method's implementation can call a different, unimplemented method on the very same receiver,
    * raising an error that names the key class but blames a method other than {@code methodName}, so
    * the method name is checked too. An unparseable message never matches.
+   *
+   * <p>JDK 8 omits the message once the call site has dispatched to a class that does implement the
+   * method, which in production is nearly always. Without a message the class itself is checked
+   * instead: see {@link #lacksImplementation}.
    */
   static boolean isNamedIn(AbstractMethodError e, Class<?> type, String methodName) {
     final String message = e.getMessage();
     if (message == null) {
-      return false;
+      return lacksImplementation(type, methodName);
     }
     final String name = type.getName();
     if (message.startsWith(RECEIVER_PREFIX)) {

internal-api/src/test/java/datadog/trace/util/ClassLatchTest.java

@@ -6,6 +6,9 @@
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
 import java.sql.SQLException;
 import java.util.concurrent.atomic.AtomicInteger;
 import org.junit.jupiter.api.Test;
@@ -300,12 +303,126 @@ void attributesTheHotSpotMessageFormats() {
     assertFalse(ClassLatch.isNamedIn(new AbstractMethodError(name), String.class, "getClientInfo"));
     assertFalse(
         ClassLatch.isNamedIn(new AbstractMethodError(name + "."), String.class, "getClientInfo"));
+    // no message: String has no abstract getClientInfo, so the error is not attributed to it
     assertFalse(ClassLatch.isNamedIn(new AbstractMethodError(), String.class, "getClientInfo"));
     assertFalse(
         ClassLatch.isNamedIn(
             new AbstractMethodError("something else"), String.class, "getClientInfo"));
   }
 
+  /** Stands in for a class that never implemented an interface method it declares. */
+  abstract static class Unimplemented {
+    public abstract String m();
+
+    public String implemented() {
+      return "ok";
+    }
+  }
+
+  /** Stands in for a wrapper that implements the method by delegating it. */
+  static class Delegating extends Unimplemented {
+    @Override
+    public String m() {
+      return "delegated";
+    }
+  }
+
+  @Test
+  void attributesAnErrorWithoutAMessageByTheClassItself() {
+    // JDK 8 gives no message once the call site has seen a class that implements the method
+    AbstractMethodError noMessage = new AbstractMethodError();
+    assertTrue(ClassLatch.isNamedIn(noMessage, Unimplemented.class, "m"));
+    // the class implements this method, so an error from it must come from elsewhere
+    assertFalse(ClassLatch.isNamedIn(noMessage, Unimplemented.class, "implemented"));
+    // a delegating wrapper is not blamed for its delegate's error
+    assertFalse(ClassLatch.isNamedIn(noMessage, Delegating.class, "m"));
+    assertFalse(ClassLatch.isNamedIn(noMessage, Unimplemented.class, "missing"));
+  }
+
+  /** Stands in for a type that is missing from the class path at run time. */
+  public static class Missing {}
+
+  /** A class with a public signature that references {@link Missing}. */
+  public static class ReferencesMissing {
+    public Missing m() {
+      return null;
+    }
+  }
+
+  @Test
+  void anUnresolvableSignatureMeansCannotTellNotAnEscapingError() throws Exception {
+    Class<?> type = withoutMissing().loadClass(ReferencesMissing.class.getName());
+    // getMethods() resolves every public signature, so the missing return type fails it
+    assertThrows(NoClassDefFoundError.class, type::getMethods);
+
+    assertFalse(ClassLatch.isNamedIn(new AbstractMethodError(), type, "m"));
+    // the answer is cached, so a second failure neither rescans nor throws
+    assertFalse(ClassLatch.isNamedIn(new AbstractMethodError(), type, "m"));
+  }
+
+  /** Defines {@link ReferencesMissing} itself, and refuses to load {@link Missing}. */
+  private static ClassLoader withoutMissing() {
+    return new ClassLoader(ClassLatchTest.class.getClassLoader()) {
+      @Override
+      protected Class<?> loadClass(String name, boolean resolve) throws ClassNotFoundException {
+        if (name.equals(Missing.class.getName())) {
+          throw new ClassNotFoundException(name);
+        }
+        if (!name.equals(ReferencesMissing.class.getName())) {
+          return super.loadClass(name, resolve);
+        }
+        synchronized (getClassLoadingLock(name)) {
+          Class<?> loaded = findLoadedClass(name);
+          if (loaded == null) {
+            byte[] bytes = classBytes(name);
+            loaded = defineClass(name, bytes, 0, bytes.length);
+          }
+          return loaded;
+        }
+      }
+    };
+  }
+
+  private static byte[] classBytes(String name) throws ClassNotFoundException {
+    try (InputStream in =
+        ClassLatchTest.class
+            .getClassLoader()
+            .getResourceAsStream(name.replace('.', '/') + ".class")) {
+      if (in == null) {
+        throw new ClassNotFoundException(name);
+      }
+      ByteArrayOutputStream out = new ByteArrayOutputStream();
+      byte[] buffer = new byte[4096];
+      for (int read; (read = in.read(buffer)) != -1; ) {
+        out.write(buffer, 0, read);
+      }
+      return out.toByteArray();
+    } catch (IOException e) {
+      throw new ClassNotFoundException(name, e);
+    }
+  }
+
+  @Test
+  void latchIfNamedLatchesOnAnErrorWithoutAMessage() {
+    final boolean[] result = new boolean[1];
+    ClassLatch<Object, String, RuntimeException> latch =
+        new ClassLatch<Object, String, RuntimeException>() {
+          @Override
+          protected Class<?> keyOf(Object target) {
+            return Unimplemented.class;
+          }
+
+          @Override
+          protected String apply(Object target) {
+            result[0] = latchIfNamed(target, "m", new AbstractMethodError());
+            return "unnamed";
+          }
+        };
+    latch.tryApply("x");
+    assertTrue(result[0]);
+    assertTrue(latch.isLatched("x"));
+  }
+
   @Test
   void checkedExceptionsPropagateWithoutLatching() {
     ClassLatch<Object, String, SQLException> latch =