Fix ClassLatch's JDK-version NoSuchMethodError handling and require a method-name check

tryApplyOrNull previously only caught NoSuchMethodError, missing the
BootstrapMethodError-wrapped form that some JVM versions raise for the
same linkage failure, which let it escape the latch uncaught.

handleAbstractMethod/latchIfNamed/isNamedIn now also take the method
name the call site expects, since a receiver's own implementation can
raise an AbstractMethodError naming the key class while blaming a
different method it calls internally; without the check that error
would incorrectly latch the class for a method it does implement.

Extracts the duplicated compile() test helper into CompilingClassLoaders.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Author: dougqh

dd-java-agent/instrumentation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/JDBCDecorator.java

@@ -54,7 +54,7 @@ public class JDBCDecorator extends DatabaseClientDecorator<DBInfo> {
       new ClassLatch<Connection, Properties, SQLException>() {
         @Override
         protected Properties apply(Connection connection) throws SQLException {
-          return handleAbstractMethod(connection, Connection::getClientInfo);
+          return handleAbstractMethod(connection, "getClientInfo", Connection::getClientInfo);
         }
       };
 

internal-api/src/jmh/java/datadog/trace/util/ClassLatchBenchmark.java

@@ -154,23 +154,23 @@ private static Object invokeHandle(Object target) {
       new ClassLatch<Object, Object, RuntimeException>() {
         @Override
         protected Object apply(Object target) {
-          return handleAbstractMethod(target, ClassLatchBenchmark::invokeHandle);
+          return handleAbstractMethod(target, "b", ClassLatchBenchmark::invokeHandle);
         }
       };
 
   private static final ClassLatch<Object, Object, RuntimeException> WRAPPER =
       new ClassLatch<Object, Object, RuntimeException>() {
         @Override
         protected Object apply(Object target) {
-          return handleAbstractMethod(target, ClassLatchBenchmark::invokeHandle);
+          return handleAbstractMethod(target, "b", ClassLatchBenchmark::invokeHandle);
         }
       };
 
   private static final ClassLatch<Object, Object, RuntimeException> PRESENT =
       new ClassLatch<Object, Object, RuntimeException>() {
         @Override
         protected Object apply(Object target) {
-          return handleAbstractMethod(target, ClassLatchBenchmark::invokeHandle);
+          return handleAbstractMethod(target, "b", ClassLatchBenchmark::invokeHandle);
         }
       };
 
@@ -183,7 +183,7 @@ protected final Object apply(Object target) {
       try {
         return invoke(target);
       } catch (AbstractMethodError e) {
-        latchIfNamed(target, e);
+        latchIfNamed(target, "b", e);
         return null;
       } catch (UnsupportedOperationException e) {
         return null;

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

@@ -69,7 +69,26 @@ protected Class<?> keyOf(T target) {
    */
   @Nullable
   public final R tryApplyOrNull(@Nullable T target) throws E {
-    return target == null || isLatched(target) ? null : apply(target);
+    if (target == null || isLatched(target)) {
+      return null;
+    }
+    try {
+      return apply(target);
+    } catch (NoSuchMethodError e) {
+      // a method reference passed to handleNoSuchMethod resolves where it is written, in apply's
+      // own frame, so a missing target method surfaces here, outside the helper's own try, as the
+      // invokedynamic call site's own linkage failure; HotSpot reports this directly as
+      // NoSuchMethodError on some JVM versions
+      latch(target);
+      return null;
+    } catch (BootstrapMethodError e) {
+      // on other JVM versions the same linkage failure is wrapped instead
+      if (e.getCause() instanceof NoSuchMethodError) {
+        latch(target);
+        return null;
+      }
+      throw e;
+    }
   }
 
   /**
@@ -103,12 +122,14 @@ protected final void unlatch(T target) {
   }
 
   /**
-   * Latches the target's key if the error's message names that class as the receiver that lacks the
-   * 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.
+   * 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.
    */
-  protected final boolean latchIfNamed(T target, AbstractMethodError error) {
-    if (isNamedIn(error, keyOf(target))) {
+  protected final boolean latchIfNamed(T target, String methodName, AbstractMethodError error) {
+    if (isNamedIn(error, keyOf(target), methodName)) {
       latch(target);
       return true;
     }
@@ -124,7 +145,7 @@ protected final boolean latchIfNamed(T target, AbstractMethodError error) {
    *
    * <pre>{@code
    * protected Properties apply(Connection c) throws SQLException {
-   *   return handleAbstractMethod(c, Connection::getClientInfo);
+   *   return handleAbstractMethod(c, "getClientInfo", Connection::getClientInfo);
    * }
    * }</pre>
    *
@@ -133,18 +154,23 @@ protected final boolean latchIfNamed(T target, AbstractMethodError error) {
    * handle; see {@link #handleNoSuchMethod} and {@link #handleNoSuchOrAbstractMethod}, and prefer
    * the latter when unsure which a call site can see.
    *
+   * <p>{@code methodName} must be {@code call}'s own method, not merely some method of {@code T}:
+   * the error's message can name the key class while blaming a different method that the guarded
+   * method's implementation happens to call internally, and only a method name check tells the two
+   * apart.
+   *
    * <p>Pass a method reference or a non-capturing lambda, and keep this method small so it inlines:
    * that is what lets the JIT see the exact function at each call site (see {@link Strategy}).
    * Compose {@link #latchIfNamed} and {@link #latch} directly for anything more involved.
    */
   @Nullable
   @StrategyConsumer
-  protected final R handleAbstractMethod(T target, @Strategy ThrowingFunction<T, R, E> call)
-      throws E {
+  protected final R handleAbstractMethod(
+      T target, String methodName, @Strategy ThrowingFunction<T, R, E> call) throws E {
     try {
       return call.apply(target);
     } catch (AbstractMethodError e) {
-      latchIfNamed(target, e);
+      latchIfNamed(target, methodName, e);
       return null;
     } catch (UnsupportedOperationException e) {
       // no class to attribute it to, and it may come from a delegate: never latched
@@ -194,16 +220,16 @@ protected final R handleNoSuchMethod(T target, @Strategy ThrowingFunction<T, R,
    *
    * <pre>{@code
    * protected Properties apply(Connection c) throws SQLException {
-   *   return handleNoSuchOrAbstractMethod(c, Connection::getClientInfo);
+   *   return handleNoSuchOrAbstractMethod(c, "getClientInfo", Connection::getClientInfo);
    * }
    * }</pre>
    */
   @Nullable
   @StrategyConsumer
-  protected final R handleNoSuchOrAbstractMethod(T target, @Strategy ThrowingFunction<T, R, E> call)
-      throws E {
+  protected final R handleNoSuchOrAbstractMethod(
+      T target, String methodName, @Strategy ThrowingFunction<T, R, E> call) throws E {
     try {
-      return handleAbstractMethod(target, call);
+      return handleAbstractMethod(target, methodName, call);
     } catch (NoSuchMethodError e) {
       latch(target);
       return null;
@@ -220,9 +246,12 @@ protected final R handleNoSuchOrAbstractMethod(T target, @Strategy ThrowingFunct
    * </ul>
    *
    * A concrete object's class is never the abstract method's declaring class or an interface, so an
-   * exact match can only be the receiver. An unparseable message never matches.
+   * exact match can only be the receiver. The message also names the resolved method itself: that
+   * 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.
    */
-  static boolean isNamedIn(AbstractMethodError e, Class<?> type) {
+  static boolean isNamedIn(AbstractMethodError e, Class<?> type, String methodName) {
     final String message = e.getMessage();
     if (message == null) {
       return false;
@@ -232,7 +261,8 @@ static boolean isNamedIn(AbstractMethodError e, Class<?> type) {
       final int end = RECEIVER_PREFIX.length() + name.length();
       return message.startsWith(name, RECEIVER_PREFIX.length())
           && message.length() > end
-          && message.charAt(end) == ' ';
+          && message.charAt(end) == ' '
+          && message.indexOf(methodName + "(", end) > end;
     }
     if (message.startsWith(name) && message.length() > name.length() + 1) {
       // JDK 8: the method name follows the class name and runs up to the descriptor, so it
@@ -241,7 +271,9 @@ static boolean isNamedIn(AbstractMethodError e, Class<?> type) {
       final int paren = message.indexOf('(', methodStart);
       return message.charAt(name.length()) == '.'
           && paren > methodStart
-          && message.indexOf('.', methodStart) < 0;
+          && message.indexOf('.', methodStart) < 0
+          && paren == methodStart + methodName.length()
+          && message.regionMatches(methodStart, methodName, 0, methodName.length());
     }
     return false;
   }

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

@@ -172,7 +172,7 @@ void latchIfNamedLatchesOnlyWhenTheErrorNamesTheKey() {
         new ClassLatch<Object, String, RuntimeException>() {
           @Override
           protected String apply(Object target) {
-            result[0] = latchIfNamed(target, receiverError(target.getClass()));
+            result[0] = latchIfNamed(target, "m", receiverError(target.getClass()));
             return "named";
           }
         };
@@ -184,7 +184,7 @@ protected String apply(Object target) {
         new ClassLatch<Object, String, RuntimeException>() {
           @Override
           protected String apply(Object target) {
-            result[0] = latchIfNamed(target, receiverError(Integer.class));
+            result[0] = latchIfNamed(target, "m", receiverError(Integer.class));
             return "other";
           }
         };
@@ -193,35 +193,78 @@ protected String apply(Object target) {
     assertFalse(other.isLatched("x"));
   }
 
+  @Test
+  void latchIfNamedDoesNotLatchWhenTheErrorNamesTheKeyButADifferentMethod() {
+    // the receiver's own implementation of "m" can call some other method internally; an
+    // AbstractMethodError raised by that other method still names the receiver class, but it is
+    // not evidence that "m" itself is missing
+    final boolean[] result = new boolean[1];
+    ClassLatch<Object, String, RuntimeException> latch =
+        new ClassLatch<Object, String, RuntimeException>() {
+          @Override
+          protected String apply(Object target) {
+            result[0] =
+                latchIfNamed(
+                    target,
+                    "m",
+                    new AbstractMethodError(
+                        "Receiver class "
+                            + target.getClass().getName()
+                            + " does not define or inherit an implementation of the resolved"
+                            + " method 'abstract java.lang.String other()' of interface I."));
+            return "named";
+          }
+        };
+    latch.tryApplyOrNull("x");
+    assertFalse(result[0]);
+    assertFalse(latch.isLatched("x"));
+  }
+
   @Test
   void attributesTheHotSpotMessageFormats() {
-    assertTrue(ClassLatch.isNamedIn(receiverError(String.class), String.class));
+    assertTrue(ClassLatch.isNamedIn(receiverError(String.class), String.class, "m"));
+    // the message names the receiver class, but blames a different method than "m"
+    assertFalse(ClassLatch.isNamedIn(receiverError(String.class), String.class, "other"));
     // "java.lang.String" is a prefix of "java.lang.StringBuilder", but not the same class
-    assertFalse(ClassLatch.isNamedIn(receiverError(String.class), StringBuilder.class));
+    assertFalse(ClassLatch.isNamedIn(receiverError(String.class), StringBuilder.class, "m"));
     assertFalse(
         ClassLatch.isNamedIn(
-            new AbstractMethodError("Receiver class " + String.class.getName()), String.class));
+            new AbstractMethodError("Receiver class " + String.class.getName()),
+            String.class,
+            "m"));
 
     // JDK 8 reports "<receiver class>.<method><descriptor>"
     String name = String.class.getName();
     assertTrue(
         ClassLatch.isNamedIn(
             new AbstractMethodError(name + ".getClientInfo()Ljava/util/Properties;"),
-            String.class));
+            String.class,
+            "getClientInfo"));
+    // the message names the receiver class, but blames a different method
+    assertFalse(
+        ClassLatch.isNamedIn(
+            new AbstractMethodError(name + ".otherMethod()Ljava/util/Properties;"),
+            String.class,
+            "getClientInfo"));
     // a different class whose name merely starts with this one
     assertFalse(
         ClassLatch.isNamedIn(
             new AbstractMethodError(name + "Builder.getClientInfo()Ljava/util/Properties;"),
-            String.class));
+            String.class,
+            "getClientInfo"));
     // a class in a package named like this class
     assertFalse(
         ClassLatch.isNamedIn(
             new AbstractMethodError(name + ".Inner.getClientInfo()Ljava/util/Properties;"),
-            String.class));
-    assertFalse(ClassLatch.isNamedIn(new AbstractMethodError(name), String.class));
-    assertFalse(ClassLatch.isNamedIn(new AbstractMethodError(name + "."), String.class));
-    assertFalse(ClassLatch.isNamedIn(new AbstractMethodError(), String.class));
-    assertFalse(ClassLatch.isNamedIn(new AbstractMethodError("something else"), String.class));
+            String.class,
+            "getClientInfo"));
+    assertFalse(ClassLatch.isNamedIn(new AbstractMethodError(name), String.class, "getClientInfo"));
+    assertFalse(
+        ClassLatch.isNamedIn(new AbstractMethodError(name + "."), String.class, "getClientInfo"));
+    assertFalse(ClassLatch.isNamedIn(new AbstractMethodError(), String.class, "getClientInfo"));
+    assertFalse(
+        ClassLatch.isNamedIn(
+            new AbstractMethodError("something else"), String.class, "getClientInfo"));
   }
 
   @Test

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

@@ -0,0 +1,35 @@
+package datadog.trace.util;
+
+import static java.util.Collections.singletonList;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import javax.tools.JavaCompiler;
+
+/** Shared support for tests that compile throwaway sources to pin real JVM error shapes. */
+final class CompilingClassLoaders {
+  private CompilingClassLoaders() {}
+
+  static void compile(JavaCompiler compiler, Path out, Path classpath, String name, String source)
+      throws IOException {
+    Path file = out.resolve(name + ".java");
+    Files.write(file, singletonList(source), StandardCharsets.UTF_8);
+    int result =
+        classpath == null
+            ? compiler.run(null, null, null, "-d", out.toString(), file.toString())
+            : compiler.run(
+                null,
+                null,
+                null,
+                "-cp",
+                classpath.toString() + File.pathSeparator,
+                "-d",
+                out.toString(),
+                file.toString());
+    assertEquals(0, result, "compiling " + name);
+  }
+}