Force inlining of classloading instrumentations to prevent indy recursions (#13282)

Co-authored-by: Trask Stalnaker <[email protected]>
Co-authored-by: Lauri Tulmin <[email protected]>

Author: 3 people

.github/workflows/build-common.yml

@@ -223,7 +223,7 @@ jobs:
           path: "sboms/*.json"
 
   test:
-    name: test${{ matrix.test-partition }} (${{ matrix.test-java-version }}, ${{ matrix.vm }})
+    name: test${{ matrix.test-partition }} (${{ matrix.test-java-version }}, ${{ matrix.vm }}, indy ${{ matrix.test-indy }})
     runs-on: ubuntu-latest
     strategy:
       matrix:
@@ -242,6 +242,9 @@ jobs:
           - 1
           - 2
           - 3
+        test-indy:
+          - false
+          - true
         exclude:
           - vm: ${{ inputs.skip-openj9-tests && 'openj9' || '' }}
       fail-fast: false
@@ -308,6 +311,7 @@ jobs:
           ${{ env.test-tasks }}
           -PtestJavaVersion=${{ matrix.test-java-version }}
           -PtestJavaVM=${{ matrix.vm }}
+          -PtestIndy=${{ matrix.test-indy }}
           -Porg.gradle.java.installations.paths=${{ steps.setup-test-java.outputs.path }}
           -Porg.gradle.java.installations.auto-download=false
           ${{ inputs.no-build-cache && ' --no-build-cache' || '' }}
@@ -325,15 +329,22 @@ jobs:
         with:
           result-encoding: string
           script: |
-            const { data: workflow_run } = await github.rest.actions.listJobsForWorkflowRun({
-              owner: context.repo.owner,
-              repo: context.repo.repo,
-              run_id: context.runId,
-              per_page: 100
-            });
             const matrix = JSON.parse(process.env.matrix);
-            const job_name = `common / test${ matrix['test-partition'] } (${ matrix['test-java-version'] }, ${ matrix.vm })`;
-            return workflow_run.jobs.find((job) => job.name === job_name).html_url;
+            const job_name = `common / test${ matrix['test-partition'] } (${ matrix['test-java-version'] }, ${ matrix.vm }, indy ${ matrix['test-indy'] })`;
+            
+            const workflow_jobs_nested = await github.paginate(
+              github.rest.actions.listJobsForWorkflowRun,
+              {
+                owner: context.repo.owner,
+                repo: context.repo.repo,
+                run_id: context.runId,
+                per_page: 100
+              },
+              (response) => {
+                return response.data;
+              },
+            );
+            return workflow_jobs_nested.flat().find((job) => job.name === job_name).html_url;
 
       - name: Flaky test report
         if: ${{ !cancelled() }}
@@ -350,7 +361,7 @@ jobs:
         if: failure()
         uses: actions/upload-artifact@4cec3d8aa04e39d1a68397de0c4cd6fb9dce8ec1 # v4.6.1
         with:
-          name: deadlock-detector-test-${{ matrix.test-java-version }}-${{ matrix.vm }}-${{ matrix.test-partition }}
+          name: deadlock-detector-test-${{ matrix.test-java-version }}-${{ matrix.vm }}-${{ matrix.test-partition }}-indy-${{ matrix.test-indy }}
           path: /tmp/deadlock-detector-*
           if-no-files-found: ignore
 

.github/workflows/build-daily-no-build-cache.yml

@@ -24,13 +24,6 @@ jobs:
     secrets:
       FLAKY_TEST_REPORTER_ACCESS_KEY: ${{ secrets.FLAKY_TEST_REPORTER_ACCESS_KEY }}
 
-  test-indy:
-    uses: ./.github/workflows/reusable-test-indy.yml
-    with:
-      no-build-cache: true
-    secrets:
-      FLAKY_TEST_REPORTER_ACCESS_KEY: ${{ secrets.FLAKY_TEST_REPORTER_ACCESS_KEY }}
-
   # muzzle is not included here because it doesn't use gradle cache anyway and so is already covered
   # by the normal daily build
 

.github/workflows/build-daily.yml

@@ -20,11 +20,6 @@ jobs:
     secrets:
       FLAKY_TEST_REPORTER_ACCESS_KEY: ${{ secrets.FLAKY_TEST_REPORTER_ACCESS_KEY }}
 
-  test-indy:
-    uses: ./.github/workflows/reusable-test-indy.yml
-    secrets:
-      FLAKY_TEST_REPORTER_ACCESS_KEY: ${{ secrets.FLAKY_TEST_REPORTER_ACCESS_KEY }}
-
   muzzle:
     uses: ./.github/workflows/reusable-muzzle.yml
 

.github/workflows/build-pull-request.yml

@@ -29,11 +29,6 @@ jobs:
     with:
       cache-read-only: true
 
-  test-indy:
-    uses: ./.github/workflows/reusable-test-indy.yml
-    with:
-      cache-read-only: true
-
   test-native:
     uses: ./.github/workflows/reusable-native-tests.yml
     with:

.github/workflows/reusable-test-indy.yml

@@ -6,7 +6,6 @@
 package io.opentelemetry.javaagent.instrumentation.internal.classloader;
 
 import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.extendsClass;
-import static java.util.logging.Level.WARNING;
 import static net.bytebuddy.matcher.ElementMatchers.isMethod;
 import static net.bytebuddy.matcher.ElementMatchers.isProtected;
 import static net.bytebuddy.matcher.ElementMatchers.isPublic;
@@ -17,17 +16,14 @@
 import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
 import static net.bytebuddy.matcher.ElementMatchers.takesArguments;
 
-import io.opentelemetry.javaagent.bootstrap.BootstrapPackagePrefixesHolder;
 import io.opentelemetry.javaagent.bootstrap.CallDepth;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
-import io.opentelemetry.javaagent.tooling.Constants;
-import java.lang.invoke.MethodHandle;
-import java.lang.invoke.MethodHandles;
-import java.lang.invoke.MethodType;
-import java.util.List;
-import java.util.logging.Logger;
+import io.opentelemetry.javaagent.tooling.Utils;
+import io.opentelemetry.javaagent.tooling.bytebuddy.ExceptionHandlers;
+import net.bytebuddy.agent.builder.AgentBuilder;
 import net.bytebuddy.asm.Advice;
+import net.bytebuddy.description.method.MethodDescription;
 import net.bytebuddy.description.type.TypeDescription;
 import net.bytebuddy.matcher.ElementMatcher;
 
@@ -53,7 +49,7 @@ public ElementMatcher<TypeDescription> typeMatcher() {
 
   @Override
   public void transform(TypeTransformer transformer) {
-    transformer.applyAdviceToMethod(
+    ElementMatcher.Junction<MethodDescription> methodMatcher =
         isMethod()
             .and(named("loadClass"))
             .and(
@@ -64,37 +60,14 @@ public void transform(TypeTransformer transformer) {
                             .and(takesArgument(0, String.class))
                             .and(takesArgument(1, boolean.class))))
             .and(isPublic().or(isProtected()))
-            .and(not(isStatic())),
-        BootDelegationInstrumentation.class.getName() + "$LoadClassAdvice");
-  }
-
-  public static class Holder {
-
-    public static final List<String> bootstrapPackagesPrefixes = findBootstrapPackagePrefixes();
-
-    /**
-     * We have to make sure that {@link BootstrapPackagePrefixesHolder} is loaded from bootstrap
-     * class loader. After that we can use in {@link LoadClassAdvice}.
-     */
-    private static List<String> findBootstrapPackagePrefixes() {
-      try {
-        Class<?> holderClass =
-            Class.forName(
-                "io.opentelemetry.javaagent.bootstrap.BootstrapPackagePrefixesHolder", true, null);
-        MethodHandle methodHandle =
-            MethodHandles.publicLookup()
-                .findStatic(
-                    holderClass, "getBoostrapPackagePrefixes", MethodType.methodType(List.class));
-        //noinspection unchecked
-        return (List<String>) methodHandle.invokeExact();
-      } catch (Throwable e) {
-        Logger.getLogger(Holder.class.getName())
-            .log(WARNING, "Unable to load bootstrap package prefixes from the bootstrap CL", e);
-        return Constants.BOOTSTRAP_PACKAGE_PREFIXES;
-      }
-    }
-
-    private Holder() {}
+            .and(not(isStatic()));
+    // Inline instrumentation to prevent problems with invokedynamic-recursion
+    transformer.applyTransformer(
+        new AgentBuilder.Transformer.ForAdvice()
+            .include(Utils.getBootstrapProxy(), Utils.getAgentClassLoader())
+            .withExceptionHandler(ExceptionHandlers.defaultExceptionHandler())
+            .advice(
+                methodMatcher, BootDelegationInstrumentation.class.getName() + "$LoadClassAdvice"));
   }
 
   @SuppressWarnings("unused")
@@ -113,7 +86,7 @@ public static Class<?> onEnter(@Advice.Argument(0) String name) {
       }
 
       try {
-        for (String prefix : Holder.bootstrapPackagesPrefixes) {
+        for (String prefix : BootstrapPackagesHelper.bootstrapPackagesPrefixes) {
           if (name.startsWith(prefix)) {
             try {
               return Class.forName(name, false, null);
@@ -134,13 +107,12 @@ public static Class<?> onEnter(@Advice.Argument(0) String name) {
     }
 
     @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
-    @Advice.AssignReturned.ToReturned
-    public static Class<?> onExit(
-        @Advice.Return Class<?> result, @Advice.Enter Class<?> resultFromBootstrapLoader) {
+    public static void onExit(
+        @Advice.Return(readOnly = false) Class<?> result,
+        @Advice.Enter Class<?> resultFromBootstrapLoader) {
       if (resultFromBootstrapLoader != null) {
-        return resultFromBootstrapLoader;
+        result = resultFromBootstrapLoader;
       }
-      return result;
     }
   }
 }

instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/BootDelegationInstrumentation.java

@@ -0,0 +1,45 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.internal.classloader;
+
+import static java.util.logging.Level.WARNING;
+
+import io.opentelemetry.javaagent.bootstrap.BootstrapPackagePrefixesHolder;
+import io.opentelemetry.javaagent.tooling.Constants;
+import java.lang.invoke.MethodHandle;
+import java.lang.invoke.MethodHandles;
+import java.lang.invoke.MethodType;
+import java.util.List;
+import java.util.logging.Logger;
+
+public class BootstrapPackagesHelper {
+
+  public static final List<String> bootstrapPackagesPrefixes = findBootstrapPackagePrefixes();
+
+  /**
+   * We have to make sure that {@link BootstrapPackagePrefixesHolder} is loaded from bootstrap class
+   * loader. After that we can use in {@link BootDelegationInstrumentation.LoadClassAdvice}.
+   */
+  private static List<String> findBootstrapPackagePrefixes() {
+    try {
+      Class<?> holderClass =
+          Class.forName(
+              "io.opentelemetry.javaagent.bootstrap.BootstrapPackagePrefixesHolder", true, null);
+      MethodHandle methodHandle =
+          MethodHandles.publicLookup()
+              .findStatic(
+                  holderClass, "getBoostrapPackagePrefixes", MethodType.methodType(List.class));
+      //noinspection unchecked
+      return (List<String>) methodHandle.invokeExact();
+    } catch (Throwable e) {
+      Logger.getLogger(BootstrapPackagesHelper.class.getName())
+          .log(WARNING, "Unable to load bootstrap package prefixes from the bootstrap CL", e);
+      return Constants.BOOTSTRAP_PACKAGE_PREFIXES;
+    }
+  }
+
+  private BootstrapPackagesHelper() {}
+}