Customizable advice dispatcher

[felixbarny]

AFAIK, there are currently two ways advices are dispatched.

By default, advice methods are inlined in to the instrumented method.

public class InlinedAdvice {
    @Advice.OnMethodEnter
    public static void onFoo(@Advice.Argument(0) boolean bar) {
        System.out.println("foo: " + bar);
    }
}

public class InstrumentedClass {
    public void foo(boolean bar) {
        System.out.println("foo: " + bar);
    }
}

If the inline property is set to false, only a static method call is inserted into the instrumented method that calls the advice method.

public class DelegatedAdvice {
    @Advice.OnMethodEnter(inline = false)
    public static void onFoo(@Advice.Argument(0) boolean bar) {
        System.out.println("foo: " + bar);
    }
}


public class InstrumentedClass {
    public void foo(boolean bar) {
        InlinedAdvice.onFoo(bar);
    }
}

I'd like to have a way to customize the dispatching.

public class CustomDelegationAdvice {
    @Advice.OnMethodEnter(inline = false)
    @Advice.Dispatcher(CustomDispatcher.class)
    public static void onFoo(@Advice.Argument(0) boolean bar) {
        System.out.println("foo: " + bar);
    }
}


public class InstrumentedClass {
    public void foo(boolean bar) {
        MethodHandleDispatcher
            .getMethodHandle(getClass().getClassLoader(), "CustomDelegationAdvice#onFoo")
            .invoke(bar);
    }
}

MethodHandleDispatcher is a registry where I would make sure to add method handles to all advice methods. Currently, I do that style of dispatching manually by creating an inlined advice that contains the MethodHandle lookup and invocation. It would be great if I could just write the advice as it was a inline = true advice and if Byte Buddy could inject the method handle lookup and invocation in a similar way that it currently does for non-inlined advices via net.bytebuddy.asm.Advice.Dispatcher.Delegating.

This is in the context of this PR: elastic/apm-agent-java#1090

Is that something that would be possible?

[raphw]

That's a good idea, but I think I'd take it one step further: I'd insert an INVOKEDYNAMIC call site and dispatch to a custom bootstrap method to which I'd dispatch a signature of:

public static CallSite bootstrap(
  MethodHandle.Lookup hostLookup, 
  MethodHandle sourceHandle,
  String adviceClass,
  String adviceMethod,
  MethodType adviceSignature
)

Do you think that covers your use case, too? I have thought about using invokedynamic in this context before and I still think it's a fit.

[felixbarny]

That would store the MethodHandle in the constant table of the class, right? That'd be great! It would also get rid of the overhead of the MethodHandle lookup and couples the lifecycle of the classloader of the instrumented method with the classloader of the called method more elegantly than what I'm currently doing.

Do you think that covers your use case, too?

I don't know tbh. I'm not too familiar with how CallSite and bootstrap methods work. How would I try that out? IIRC, you blogged about that in the context of singletons, right?

Do you know how stable the Java 7 support for that is? And I suppose Java 8 < 40 would have problems with that but we don't instrument those Java 8 versions anyways. We've seen some MethodHandle-related issues with Java 7 (u211) in the past: elastic/apm-agent-java#458.

[felixbarny]

Currently, a big downside of non-inlined advices is that the readOnly property is not allowed to be true. Do you think it would be possible to use the return value of advices to write to allow writing? For example:

@Advice.OnMethodExit(inline = false)
public static @Advice.Return(readOnly = false) String onMethodExit(@Advice.Argument(0) String arg, @Advice.Return String result) {
    return result + arg;
}

That would make non-inlined advices much more powerful.

[raphw]

That second suggestion would be more difficult to accomplish, I'd need to see how to work this out without breaking other assumptions. But I will look into it!

As for invokedynamic. Basically, you return a ConstantCallSite most of the time where the single argument is a MethodHandle.

This constant call site is then resolved a single time into the target and never invoked again. The overhead should be zero. And sure, there might be bugs in 7. There probably are since method handles are not used by javac in this version but the entire 7 release is full of bugs that are no longer fixed, it's really long stretch maintenance by now. People should ideally upgrade.

But out of curiosity: why does inlining not work for you? It should still be the most reliable form of instrumentation.

[felixbarny]

But out of curiosity: why does inlining not work for you? It should still be the most reliable form of instrumentation.

Good question. Let me elaborate a bit on that. It mainly has two reasons. One is visibility issues for helper classes, the other is limitations present in filtering classloaders (OSGi bootdelegation).

Visibility issues

What's great about inlining advices is that all the inlined code has the same class visibility as the instrumented method. However, it often leads to less than ideal designs where code is copy/pasted or it's quite difficult to have generic abstractions with framework-specific bindings.

One example of this would be a header extractor interface that requires a framework-specific implementation for each HTTP framework. Another one is when trying to implement a framework-specific interceptor/callback interface.

Most of the workarounds require deep understanding of how classloaders work and which classes are safe to access in which context. That significantly raises the bar for contributions, makes reviews harder and complicates development.

Fighting filtering classloaders

One way of solving the OSGi bootdelegation issue is instrumenting OSGi classloaders to always allow bootdelegation for our agent. However, that would be a big maintenance burden and would not work for non-OSGi filtering classloaders.

Instead, we only want to inject a single class into the bootstrap classloader - MethodHandleDispatcher. It's basically a Map<ClassLoader, Map<String, MethodHandle>> (omitting WeakReferences for brevity).
That class would be in the java.lang package so that filtering classloaders always allow access to it. The rest of the agent would be loaded from a child classloader of the bootstrap classloader.

This has the added benefit that the normal classloader hierarchy doesn't see the agent classes. Thus, classpath scanning tools are not slowed down or fail because of agent classes.

Our solution

Bringing the two points together, the MethodHandles in the dispatcher point to classes loaded from a helper classloader that is created as a child of the target classloader (the one with the classes to be instrumented). The helper classloader has a second parent - the agent classloader.

This allows the helpers to have access to both, framework-specific types from the target classloader, and agent classes form the agent classloader.
The clue is that the MethodHandle lookup takes not only the name of the method into consideration but also the classloader of the instrumented class. That is because there's a helper classloader for each classloader we instrument types in.

See also this diagram: https://github.com/elastic/apm-agent-java/blob/a9a4611830e8d41e75ddacf247ae9144a0be4669/apm-agent-core/src/main/java/co/elastic/apm/agent/bootstrap/MethodHandleDispatcher.java#L54-L72

By having a dedicated Agent.Dispatcher for this I hope to make things even easier. The idea would be that advices are loaded from a helper class loader (that has the target CL and the agent CL as its parent). Advice methods (annotated with @Advice.OnMethodEnter or @Advice.OnMethodExit) are automatically added to the MethodHandleDispatcher. The Agent.Dispatcher would then inject a MethodHandle invocation into the instrumented methods that points to the advice method of its corresponding helper class loader.

As an added bonus, it allows setting breakpoints in advices and makes exception stack traces are more useful.

Summary

In summary, this makes development much easier, allows for proper abstractions, separates the agent classes from the regular classloader hierarchy, and doesn't require to inject classes into the target classloader.

---

And sure, there might be bugs in 7

One workaround could be to use java.lang.reflect.Method invocations instead of MethodHandles when the agent is running on Java 7.

[felixbarny]

dispatch to a custom bootstrap method to which I'd dispatch a signature of:

public static CallSite bootstrap(
 MethodHandle.Lookup hostLookup, 
  MethodHandle sourceHandle,
  String adviceClass,
  String adviceMethod,
  MethodType adviceSignature
)

Do you think that covers your use case, too?

I think I'd need another parameter for the classloader of the instrumented class in order to lookup the method handle from the corresponding helper class loader. The hostLookup is a lookup that is gathered in the context from the instrumented method? Does sourceHandle resemble a handle to the instrumented method?

[raphw]

That should be as easy as: lookup.lookupClass().getClassLoader(). And yes, the source handle would be a handle to the instrumented method.

[felixbarny]

Right, thanks!

Does the helper class loader architecture sound sane to you? Maybe this is even something that could be included in Byte Buddy itself. I think one missing piece in Byte Buddy is an optional opinionated layer to make the handling of class visibility and bootstrapping an agent, including a proper classloader setup, easier.

I assume the public static CallSite bootstrap-based approach would only reliably work as of Java 8u40? How feasible would a java.lang.reflect.Method-based workaround be for Java 7?

[raphw]

I just added the possibility to add a bootstrapping class. You can do so by:

Advice.withCustomMapping().bootsrap(someMethod).to(...);

what causes an invokedynamic call site to be generated for all invocations. I am not yet sure if I want to support reflection-based invocation. It's really messy with the boxing and checked exceptions and the performance is rather poor. Is supporting Java 6 for this form of dispatching crucial? I'd suggest just injecting the whole bunch into the bootstrap loader, probably Java 6 applications are rather static anyways.

[felixbarny]

Thanks, I’ll try this out soon.
Luckily, we don’t support Java 6. However, we saw some problems with Java 7 and MethodHandles in the past (see also the link I posted earlier). After a C2 compilation there was a segfault. This may or may not be an issue with the indy instruction you are using for the advice dispatching. If it’s not problematic it’s all the better! Did you run some benchmarks or stress tests to check that there’s no segfault after C2 kicks in?

Am I right in assuming that overriding a parameter or return value is not possible using the indy dispatcher?

[raphw]

No, the indy solution is currently analogous to the regular static dispatcher. I'll look into the possibility to add such return value reuse but I need to check how and if this can be done without bloating the API or implementation.

[felixbarny]

The docs about Groovy indy support for Java 7 suggest that it's stable as of Java 7u60.

If it turns out it's a similar story for the indy dispatch in Byte Buddy it would be great news as a reflection-based alternative wouldn't be necessary for our use case.

https://groovy-lang.org/indy.html

All JDK 7 versions ranging from 7u21 to 7u55 are buggy with regards to invokedynamic. If you plan to use invokedynamic support, make sure you either use 7u60 or JDK 8.

[felixbarny]

The API works great!

Just one suggestion: If for whatever reason the target method could not be resolved, it would be nice to be able to return null from the bootstrap method to disable calling the advice.

As almost all our advices override parameters or return values, having support for that in non-inlined advices is crucial.

[raphw]

I'll look into the possibility of adding additional semantics to a method return value.

As for returning null: the bootstrap mechanism does not support this. You can however bind a constant call site that drops all arguments and returns null, a primitive 0 or nothing depending on the advice methods return type. Have a look at the lookup API for creating such a constant handle.

[felixbarny]

Does the Method that's provided in Advice.WithCustomMapping#bootstrap(java.lang.reflect.Method) have to be visible to the instrumented methods? I think it does, right? So it probably makes sense to first inject the class containing the bootstrap method into the bootstrap classloader. Otherwise, instrumenting classes defined by the bootstrap classloader would result in an error. Maybe mention that in the Javadocs?

Out of curiosity, what's the use case for the Advice.WithCustomMapping#bootstrap(java.lang.reflect.Constructor<?>) method? What kind of constructor is expected here?

[raphw]

Any constructor of an instance of CallSite can be used as a bootstrap method. You could for example subclass constant call site for doing so.

And yes, the bootstrap method needs to be visible (and exported) . Otherwise you could breach into protected code using invokedynamic.