Polish and documentation

- Rename helper class loader to plugin class loader
- Add GlobalVariables registry

apm-agent-core/src/main/java/co/elastic/apm/agent/bci/ElasticApmInstrumentation.java

@@ -195,6 +195,46 @@ public Advice.OffsetMapping.Factory<?> getOffsetMapping() {
     public void onTypeMatch(TypeDescription typeDescription, ClassLoader classLoader, ProtectionDomain protectionDomain, @Nullable Class<?> classBeingRedefined) {
     }
 
+    /**
+     * When this method returns {@code true} the whole package (starting at the {@linkplain #getAdviceClass() advice's} package)
+     * will be loaded from a plugin class loader that has both the agent class loader and the class loader of the class this instruments as
+     * the parent.
+     * <p>
+     * This instructs Byte Buddy to dispatch to the advice methods via an {@code INVOKEDYNAMIC} instruction.
+     * Upon first invocation of an instrumented method,
+     * this will call {@link IndyBootstrap#bootstrap} to determine the target {@link java.lang.invoke.ConstantCallSite}.
+     * </p>
+     * <p>
+     * Things to watch out for when using indy dispatch:
+     * <ul>
+     *     <li>
+     *         When an advice instruments classes in multiple class loaders, the plugin classes will be loaded form multiple class loaders.
+     *         In order to still share state across those plugin class loaders, use {@link co.elastic.apm.agent.util.GlobalVariables} or {@link GlobalState}.
+     *         That's necessary as a static variables are scoped to the class loader they are defined in.
+     *     </li>
+     *     <li>
+     *         Don't use {@link ThreadLocal}s as it can lead to class loader leaks.
+     *         Use {@link co.elastic.apm.agent.threadlocal.RemoveOnGetThreadLocal} instead.
+     *     </li>
+     *     <li>
+     *         Due to the automatic plugin classloader creation that is based on package scanning,
+     *         plugins need be in their own uniquely named package.
+     *         As the package of the {@link #getAdviceClass()} is used as the root,
+     *         all advices have to be at the top level of the plugin.
+     *     </li>
+     *     <li>
+     *         Set {@link Advice.OnMethodEnter#inline()} and {@link Advice.OnMethodExit#inline()} to {@code false} on all advices.
+     *         As the {@code readOnly} flag in Byte Buddy annotations such as {@link Advice.Return#readOnly()} cannot be used with non
+     *         {@linkplain Advice.OnMethodEnter#inline() inlined advices},
+     *         use {@link co.elastic.apm.agent.bci.bytebuddy.postprocessor.AssignTo} and friends.
+     *     </li>
+     * </ul>
+     * </p>
+     *
+     * @return whether to load the classes of this plugin in dedicated plugin class loaders (one for each unique class loader)
+     * and dispatch to the {@linkplain #getAdviceClass() advice} via an {@code INVOKEDYNAMIC} instruction.
+     * @see IndyBootstrap
+     */
     public boolean indyDispatch() {
         return false;
     }

apm-agent-core/src/main/java/co/elastic/apm/agent/bci/GlobalState.java

@@ -11,9 +11,9 @@
  * the Apache License, Version 2.0 (the "License"); you may
  * not use this file except in compliance with the License.
  * You may obtain a copy of the License at
- * 
+ *
  *   http://www.apache.org/licenses/LICENSE-2.0
- * 
+ *
  * Unless required by applicable law or agreed to in writing,
  * software distributed under the License is distributed on an
  * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
@@ -40,6 +40,9 @@
  * the instrumentation classes will also be loaded by multiple class loaders.
  * The effect of that is that state added to static variables in one class loader does not affect the static variable in other class loaders.
  * </p>
+ * <p>
+ * An alternative to this is {@link co.elastic.apm.agent.util.GlobalVariables} which can be used to make individual variables scoped globally.
+ * </p>
  */
 @Retention(RetentionPolicy.RUNTIME)
 @Target(ElementType.TYPE)

apm-agent-core/src/main/java/co/elastic/apm/agent/bci/HelperClassManager.java

@@ -276,7 +276,7 @@ private synchronized T loadAndReferenceHelper(Class<?> classOfTargetClassLoader)
         }
     }
 
-    public static class ForDispatcher {
+    public static class ForIndyPlugin {
 
         private static final Map<ClassLoader, Map<Collection<String>, WeakReference<ClassLoader>>> alreadyInjected = new WeakHashMap<ClassLoader, Map<Collection<String>, WeakReference<ClassLoader>>>();
 
@@ -285,7 +285,7 @@ public static class ForDispatcher {
          * The agent class loader is currently the bootstrap CL but in the future it will be an isolated CL that is a child of the bootstrap CL.
          */
         @Nullable
-        public synchronized static ClassLoader inject(@Nullable ClassLoader targetClassLoader, @Nullable ProtectionDomain protectionDomain, List<String> classesToInject, ElementMatcher<? super TypeDescription> exclusionMatcher) throws Exception {
+        public synchronized static ClassLoader getOrCreatePluginClassLoader(@Nullable ClassLoader targetClassLoader, @Nullable ProtectionDomain protectionDomain, List<String> classesToInject, ElementMatcher<? super TypeDescription> exclusionMatcher) throws Exception {
             classesToInject = new ArrayList<>(classesToInject);
 
             Map<Collection<String>, WeakReference<ClassLoader>> injectedClasses = getOrCreateInjectedClasses(targetClassLoader);
@@ -301,16 +301,16 @@ public synchronized static ClassLoader inject(@Nullable ClassLoader targetClassL
                     classesToInjectCopy.add(className);
                 }
             }
-            logger.debug("Creating helper class loader for {} containing {}", targetClassLoader, classesToInjectCopy);
+            logger.debug("Creating plugin class loader for {} containing {}", targetClassLoader, classesToInjectCopy);
 
-            ClassLoader parent = getHelperClassLoaderParent(targetClassLoader);
+            ClassLoader parent = getPluginClassLoaderParent(targetClassLoader);
             Map<String, byte[]> typeDefinitions = getTypeDefinitions(classesToInjectCopy);
-            // child first semantics are important here as the helper CL contains classes that are also present in the agent CL
-            ClassLoader helperCL = new ByteArrayClassLoader.ChildFirst(parent, true, typeDefinitions, ByteArrayClassLoader.PersistenceHandler.MANIFEST);
-            injectedClasses.put(classesToInject, new WeakReference<>(helperCL));
+            // child first semantics are important here as the plugin CL contains classes that are also present in the agent CL
+            ClassLoader pluginClassLoader = new ByteArrayClassLoader.ChildFirst(parent, true, typeDefinitions, ByteArrayClassLoader.PersistenceHandler.MANIFEST);
+            injectedClasses.put(classesToInject, new WeakReference<>(pluginClassLoader));
 
 
-            return helperCL;
+            return pluginClassLoader;
         }
 
         private static Map<Collection<String>, WeakReference<ClassLoader>> getOrCreateInjectedClasses(@Nullable ClassLoader targetClassLoader) {
@@ -322,13 +322,13 @@ private static Map<Collection<String>, WeakReference<ClassLoader>> getOrCreateIn
             return injectedClasses;
         }
 
-        private static ClassLoader getHelperClassLoaderParent(@Nullable ClassLoader targetClassLoader) {
+        private static ClassLoader getPluginClassLoaderParent(@Nullable ClassLoader targetClassLoader) {
             ClassLoader agentClassLoader = HelperClassManager.class.getClassLoader();
             if (agentClassLoader == null) {
                 agentClassLoader = ClassLoader.getSystemClassLoader();
             }
-            // the helper class loader has both, the agent class loader and the target class loader as the parent
-            // this is important so that the helper class loader has direct access to the agent class loader
+            // the plugin class loader has both, the agent class loader and the target class loader as the parent
+            // this is important so that the plugin class loader has direct access to the agent class loader
             // otherwise, filtering class loaders (like OSGi) have a chance to interfere
             return new MultipleParentClassLoader(Arrays.asList(agentClassLoader, targetClassLoader));
         }

apm-agent-core/src/main/java/co/elastic/apm/agent/bci/IndyBootstrap.java

@@ -30,6 +30,7 @@
 import org.stagemonitor.util.IOUtils;
 
 import javax.annotation.Nullable;
+import java.lang.invoke.CallSite;
 import java.lang.invoke.ConstantCallSite;
 import java.lang.invoke.MethodHandle;
 import java.lang.invoke.MethodHandles;
@@ -45,11 +46,119 @@
 
 /**
  * When {@link ElasticApmInstrumentation#indyDispatch()} returns {@code true},
- * we instruct byte buddy to dispatch {@linkplain Advice.OnMethodEnter#inline()} non-inlined advices} via an invokedynamic (indy) instruction.
+ * we instruct Byte Buddy (via {@link Advice.WithCustomMapping#bootstrap(java.lang.reflect.Method)})
+ * to dispatch {@linkplain Advice.OnMethodEnter#inline() non-inlined advices} via an invokedynamic (indy) instruction.
+ * The target method is linked to a dynamically created plugin class loader that is specific to an instrumentation plugin
+ * and the class loader of the instrumented method.
+ * <p>
+ * The first invocation of an {@code INVOKEDYNAMIC} causes the JVM to dynamically link a {@link CallSite}.
+ * In this case, it will use the {@link #bootstrap} method to do that.
+ * This will also create the plugin class loader.
+ * </p>
+ * <pre>
+ *   Bootstrap CL ←──────────────────────────── Agent CL
+ *       ↑ └java.lang.IndyBootstrapDispatcher ─ ↑ ─→ └ {@link IndyBootstrap#bootstrap}
+ *     Ext/Platform CL               ↑          │       ╷
+ *       ↑                           ╷          │       ↓
+ *     System CL                     ╷          │ {@link HelperClassManager.ForIndyPlugin#getOrCreatePluginClassLoader}
+ *       ↑                           ╷          │       ╷
+ *     Common               linking of CallSite │       ╷
+ *     ↑    ↑             (on first invocation) │    creates
+ * WebApp1  WebApp2                  ╷          │       ╷
+ *          ↑ - InstrumentedClass    ╷          │       ╷
+ *          │                ╷       ╷          │       ╷
+ *          │                INVOKEDYNAMIC      │       ↓
+ *          └────────────────┼──────────────────Plugin CL
+ *                           └╶╶╶╶╶╶╶╶╶╶╶╶╶╶╶╶→ ├ AdviceClass
+ *                                              └ AdviceHelper
+ * Legend:
+ *  ╶╶→ method calls
+ *  ──→ class loader parent/child relationships
+ * </pre>
+ * <p>
+ * Advantages:
+ * </p>
+ * <ul>
+ *     <li>
+ *         <b>OSGi class loaders</b>
+ *         can't interfere with class loading.
+ *         Instrumented classes only need to see java.lang.IndyBootstrapDispatcher.
+ *         The actual advice class is invoked via {@code INVOKEDYNAMIC} instruction,
+ *         basically a dynamically looked up {@link MethodHandle}.
+ *     </li>
+ *     <li>
+ *         <b>Performance:</b>
+ *         As the target {@link MethodHandle} is not changed after the initial lookup
+ *         (we return a {@link ConstantCallSite} from {@link IndyBootstrap#bootstrap}),
+ *         the JIT can easily inline the advice into the instrumented method.
+ *     </li>
+ *     <li>
+ *         <b>Runtime attachment</b>
+ *         This approach circumvents any OSGi issues even when attaching the agent at runtime.
+ *         Setting the {@code org.osgi.framework.bootdelegation} property after the OSGi class loaders have already initialized has no effect.
+ *         This is also a more holistic solution that also works for non-OSGi filtering class loaders.
+ *     </li>
+ *     <li>
+ *         <b>Runtime detachment:</b>
+ *         After un-instrumenting classes ({@link ElasticApmAgent#reset()}) and stopping all agent threads there should be no references
+ *         to any Plugin CL or the Agent CL.
+ *         This means the GC should be able to completely remove all loaded classes and class loaders of the agent,
+ *         except for {@code java.lang.IndyBootstrapDispatcher}.
+ *         This can be useful to completely remove/detach the agent at runtime or to update the agent version without restarting the application.
+ *     </li>
+ *     <li>
+ *         <b>Class visibility:</b>
+ *         The plugins can access both the specific types of the library they access and the agent classes as the Plugin CL
+ *         has both the Agent CL and the CL of the instrumented class as its parent.
+ *         Again, OSGi class loaders can't interfere here as both the Plugin CL and the Agent CL are under full control of the agent.
+ *     </li>
+ *     <li>
+ *         <b>Debugging advices:</b>
+ *         Advice classes can easily be debugged as they are not inlined in the instrumented methods.
+ *     </li>
+ *     <li>
+ *         <b>Unit testing:</b>
+ *         Classes loaded from the bootstrap class loader can be instrumented in unit tests.
+ *     </li>
+ * </ul>
+ * <p>
+ * Challenges:
+ * </p>
+ * <ul>
+ *     <li>
+ *         As we're working with {@code INVOKEDYNAMIC} instructions that have only been introduced in Java 7,
+ *         we have to patch classes we instrument that are compiled with Java 6 bytecode level (50) to Java 7 bytecode level (51).
+ *         This involves re-computing stack frames and removing JSR instructions.
+ *         See also {@link co.elastic.apm.agent.bci.ElasticApmAgent#applyAdvice}.
+ *         This makes instrumentation a bit slower but it seems to work reliably,
+ *         even when re-transforming classes (important for runtime attachment).
+ *     </li>
+ *     <li>
+ *         The {@code INVOKEDYNAMIC} support of early Java 7 versions is not reliable.
+ *         That's why we disable the agent on them.
+ *         See also {@link AgentMain#isJavaVersionSupported}
+ *     </li>
+ *     <li>
+ *         There are some things to watch out for when writing plugins,
+ *         as explained in {@link ElasticApmInstrumentation#indyDispatch()}
+ *     </li>
+ * </ul>
+ * @see ElasticApmInstrumentation#indyDispatch()
  */
 public class IndyBootstrap {
+
+    /**
+     * Starts with {@code java.lang} so that OSGi class loaders don't restrict access to it
+     */
     private static final String INDY_BOOTSTRAP_CLASS_NAME = "java.lang.IndyBootstrapDispatcher";
+    /**
+     * The class file of {@code java.lang.IndyBootstrapDispatcher}.
+     * Ends with {@code clazz} because if it ended with {@code clazz}, it would be loaded like a regular class.
+     */
     private static final String INDY_BOOTSTRAP_RESOURCE = "bootstrap/IndyBootstrapDispatcher.clazz";
+    /**
+     * Caches the names of classes that are defined within a package and it's subpackages
+     */
     private static final ConcurrentMap<String, List<String>> classesByPackage = new ConcurrentHashMap<>();
     @Nullable
     static Method indyBootstrapMethod;
@@ -71,6 +180,9 @@ public static Method getIndyBootstrapMethod() {
         }
     }
 
+    /**
+     * Injects the {@code java.lang.IndyBootstrapDispatcher} class into the bootstrap class loader if it wasn't already.
+     */
     private static Class<?> initIndyBootstrap() throws Exception {
         try {
             return Class.forName(INDY_BOOTSTRAP_CLASS_NAME, false, null);
@@ -84,7 +196,6 @@ private static Class<?> initIndyBootstrap() throws Exception {
     /**
      * Is called by {@code java.lang.IndyBootstrapDispatcher#bootstrap} via reflection.
      * <p>
-     * <p>
      * This is to make it impossible for OSGi or other filtering class loaders to restrict access to classes in the bootstrap class loader.
      * Normally, additional classes that have been injected have to be explicitly allowed via the {@code org.osgi.framework.bootdelegation}
      * system property.
@@ -104,7 +215,7 @@ private static Class<?> initIndyBootstrap() throws Exception {
      * The advice can access both agent types and the types of the instrumented library.
      * </p>
      * <p>
-     * Exceptions and {@code null} return values are handled by caller.
+     * Exceptions and {@code null} return values are handled by {@code java.lang.IndyBootstrapDispatcher#bootstrap}.
      * </p>
      */
     @Nullable
@@ -118,15 +229,19 @@ public static ConstantCallSite bootstrap(MethodHandles.Lookup lookup,
                                              int enter) throws Exception {
         Class<?> adviceClass = Class.forName(adviceClassName);
         String packageName = adviceClass.getPackage().getName();
-        List<String> helperClasses = classesByPackage.get(packageName);
-        if (helperClasses == null) {
+        List<String> pluginClasses = classesByPackage.get(packageName);
+        if (pluginClasses == null) {
             classesByPackage.putIfAbsent(packageName, PackageScanner.getClassNames(packageName));
-            helperClasses = classesByPackage.get(packageName);
+            pluginClasses = classesByPackage.get(packageName);
         }
-        ClassLoader helperClassLoader = HelperClassManager.ForDispatcher.inject(lookup.lookupClass().getClassLoader(), instrumentedType.getProtectionDomain(), helperClasses, isAnnotatedWith(named(GlobalState.class.getName())));
-        if (helperClassLoader != null) {
-            Class<?> adviceInHelperCL = helperClassLoader.loadClass(adviceClassName);
-            MethodHandle methodHandle = MethodHandles.lookup().findStatic(adviceInHelperCL, adviceMethodName, adviceMethodType);
+        ClassLoader pluginClassLoader = HelperClassManager.ForIndyPlugin.getOrCreatePluginClassLoader(
+            lookup.lookupClass().getClassLoader(),
+            instrumentedType.getProtectionDomain(),
+            pluginClasses,
+            isAnnotatedWith(named(GlobalState.class.getName())));
+        if (pluginClassLoader != null) {
+            Class<?> adviceInPluginCL = pluginClassLoader.loadClass(adviceClassName);
+            MethodHandle methodHandle = MethodHandles.lookup().findStatic(adviceInPluginCL, adviceMethodName, adviceMethodType);
             return new ConstantCallSite(methodHandle);
         }
         return null;

apm-agent-core/src/main/java/co/elastic/apm/agent/threadlocal/RemoveOnGetThreadLocal.java

@@ -11,9 +11,9 @@
  * the Apache License, Version 2.0 (the "License"); you may
  * not use this file except in compliance with the License.
  * You may obtain a copy of the License at
- * 
+ *
  *   http://www.apache.org/licenses/LICENSE-2.0
- * 
+ *
  * Unless required by applicable law or agreed to in writing,
  * software distributed under the License is distributed on an
  * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
@@ -27,21 +27,37 @@
 import com.blogspot.mydailyjava.weaklockfree.DetachedThreadLocal;
 
 import javax.annotation.Nullable;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
 
 public class RemoveOnGetThreadLocal<T> extends DetachedThreadLocal<T> {
 
+    private static final ConcurrentMap<String, RemoveOnGetThreadLocal<?>> registry = new ConcurrentHashMap<>();
     @Nullable
     private final T defaultValue;
 
-    public RemoveOnGetThreadLocal() {
-        this(null);
-    }
-
-    public RemoveOnGetThreadLocal(@Nullable T defaultValue) {
+    private RemoveOnGetThreadLocal(@Nullable T defaultValue) {
         super(Cleaner.INLINE);
         this.defaultValue = defaultValue;
     }
 
+    public static <T> RemoveOnGetThreadLocal<T> get(Class<?> adviceClass, String key) {
+        return get(adviceClass.getName() + "." + key, null);
+    }
+
+    public static <T> RemoveOnGetThreadLocal<T> get(Class<?> adviceClass, String key, @Nullable T defaultValue) {
+        return get(adviceClass.getName() + "." + key, defaultValue);
+    }
+
+    private static <T> RemoveOnGetThreadLocal<T> get(String key, @Nullable T defaultValue) {
+        RemoveOnGetThreadLocal<?> threadLocal = registry.get(key);
+        if (threadLocal == null) {
+            registry.putIfAbsent(key, new RemoveOnGetThreadLocal<T>(defaultValue));
+            threadLocal = registry.get(key);
+        }
+        return (RemoveOnGetThreadLocal<T>) threadLocal;
+    }
+
     @Nullable
     public T getAndRemove() {
         T value = get();