Make internal classloader instrumentation indy compatible #12242
Conversation
| * Copy of {@link Constants#BOOTSTRAP_PACKAGE_PREFIXES} because the Constants class | ||
| * is not accessible from here | ||
| */ | ||
| public static final List<String> BOOTSTRAP_PACKAGE_PREFIXES = |
There was a problem hiding this comment.
This test seemed to have relied on the side effect of Constants being injected into every ClassLoader containing ClassLoaders. We don't inject it anymore, which caused this test to fail for indy.
...ntelemetry/javaagent/instrumentation/internal/classloader/BootDelegationInstrumentation.java
Show resolved
Hide resolved
| // TODO: the ToReturened does nothing except for signaling the AdviceTransformer that it must | ||
| // not touch this advice | ||
| // this is done to ensure that ClassLoaderInstrumentationModule.loadAdviceClassesEagerly works | ||
| // correctly | ||
| // we can therfore remove it, as soon as the AdviceTransformer is not applied anymore |
There was a problem hiding this comment.
[minor] maybe having a dedicated annotation for this could help, otherwise we just rely on a side-effect of having this ToReturned annotation present.
There was a problem hiding this comment.
I didn't want to introduce a separate mechanism here because this instrumentation is likely the very only case where this is needed and AdviceTransformer will be a temporary thing anyway
...opentelemetry/javaagent/instrumentation/internal/classloader/DefineClassInstrumentation.java
Outdated
Show resolved
Hide resolved
...opentelemetry/javaagent/instrumentation/internal/classloader/DefineClassInstrumentation.java
Outdated
Show resolved
Hide resolved
| if (justDelegateAdvice) { | ||
| context.disableReturnTypeChange(); | ||
| } |
There was a problem hiding this comment.
[for reviewer] without this the advice return type is assumed to be an array, which means skipOnIndex is added when using skipOn annotation attribute, and this breaks when advice is not inlined.
| // Ensure that CallDepth is already loaded in case of bootstrapAdvice recursions with | ||
| // ClassLoader.loadClass | ||
| // This is required because CallDepth is a bootstrap class and therefore triggers our | ||
| // ClassLoader.loadClass instrumentations | ||
| Class.forName(CallDepth.class.getName()); |
There was a problem hiding this comment.
[minor] maybe add that this class needs to be explicitly initialized early because it's the one preventing recursive calls and is thus the one called first when loadClass advice method is executed. Adding a comment where this first call is made would also be relevant too just in case someone tries to add something before it.
Also, could we make this static init part of the advice static init instead ? If possible that would be easier to link with the advice method code. On the other hand this CallDepth init call might be needed elsewhere.
There was a problem hiding this comment.
It is in fact not the case that this class needs to be initialized early. It instead needs to be linked to IndyBootstrap early:
When some IndyBootstrap.bootstrapAdvice is executed for the first time, the JVM encounters a call to CallDepth.forClass. When linking this call, the JVM itself invokes AgentClassLoader.loadClass(CallDepth) to link that callsite. This in turn will delegate to the instrumented ClassLoader.loadClass because it is a bootstrap class. It doesn't matter where within bootstrapAdvice this call happens, it just happened by chance that it is the first one.
Also, could we make this static init part of the advice static init instead
For the reasons above: No, this is not possible, because it wouldn't link the usage of CallDepth from within the IndyBootstrap class.
...lemetry/javaagent/instrumentation/internal/classloader/LoadInjectedClassInstrumentation.java
Show resolved
Hide resolved
JonasKunz
force-pushed
the
indy-classloader-instrumentations
branch
from
ac1ae8a
to
f351043
Compare
...ntelemetry/javaagent/instrumentation/internal/classloader/BootDelegationInstrumentation.java
Outdated
Show resolved
Hide resolved
…ain/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/BootDelegationInstrumentation.java Co-authored-by: SylvainJuge <763082+SylvainJuge@users.noreply.github.com>
These instrumentations were quite some fun ones.
They are slightly problematic, because they instrument
ClassLoader.loadClassandClassLoader.defineClass.This is difficult with invokedynamic, because the bootstrapping of the
invokedynamicinstructions inserted for these instrumentations itself involves loading and defining of classes, causing an infinite recursion.Here is how this PR avoid these recursions:
Avoiding ClassLoader.loadClass recursions
When an Advice is bootstrapped,
ClassLoader.loadClassis invoked for loading the Advice-class and for initializing the dependencies ofIndyBootsrapin case this is the very first bootstrapped advice. We ahve avoided this by:AgentClassLoaderandInstrumentationModuleClassloaderfrom instrumentations instrumentingClassLoader.loadClassoverriddesAgentClassLoaderandInstrumentationModuleClassloaderoverride all overloads ofClassLoader.loadClassThe latter was needed because otherwise we would still get a recursion if invoking and overload, because it would fallback to the instrumented super method.
This is almost good enough, because it avoid the recursion for all classes loaded by the excluded loaders themselves. However, for classes bootstrap-classloader classes (such as
CallDepth) used byIndyBootstrap.bootstrapwe still would get a recursion, because thenAgentClassLoaderdelegates tosuper.loadClass. This could occur on the very first invocation toIndyBootstrap.bootstrapif those classes are not linked yet. We avoid this by linking the corresponding classes (justCallDepthat the moment) eagerly in the static initializer ofIndyBootstrap.Avoiding ClassLoader.defineClass recursions
When an Advice is bootstrapped, the target Advice-class is loaded for the first time and therefore
ClassLoader.defineClassis invoked for it. This therefore by defintion yields a invokedynamic bootstrapping recursion for theDefineClassInstrumentation.We avoid this by adding a new mechanism to allow Advice-classes to not be loaded on the first invocation of the instrumented method, but instead already loading them when the
invokedynamicbytecode is inserted. This already prevents the recursion, because then the loading of the Advice class during bootstrapping will not involve adefineClasscall anymore.