Implemented InstrumentationModuleClassLoader for invokedynamic Advice dispatching

[JonasKunz]

Implements the InstrumentationModuleClassloader with the delegation model described in #8999.
It might also be useful to have a look at the full PoC implementation to see how things fit in the bigger picture.

The classloader also ensures that the lookup of .class resources is consistent with the classloading delegation.
This should avoid the need for manual resource injection in cases such as the SpringWebInstrumentationModule.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: JonasKunz / name: Jonas Kunz (7a71b65, 0f2d140, 89d9005, baa567e, aa2215b, b7e42f9, ed3de5b, 2547bda, 9cc0492, c28a853, 71a0210, f820ede, 712ea36, 0eca9c5, ee912f0, 0d3829d)
• ✅ login: trask / name: Trask Stalnaker (9cc0492)

[laurit]

I believe that on jdk8 this might not be safe and may result in IllegalArgumentException from https://github.com/openjdk/jdk8u/blob/587090ddc17c073d56f4d3f52b61f6477d6322b0/jdk/src/share/classes/java/lang/ClassLoader.java#L1587. getPackage searches for package from the parent loader and then from the current loader. As this is a child first loader when it starts to define class parent loader is not locked and can define the same package at the same time which would result in a race where package in parent loader is not define when findPackage is called but already is defined when definePackage is called. For this race to happen this class loader would need to define a class that is in the same package as a class defined in the parent loader at the same time, IDK whether this is really possible. If this were a parallel capable class loader then there could be more ways it could hit a race here and get the same IllegalArgumentException.

[JonasKunz]

Any idea how to guard against this?
I can only think of wrapping the definePackage in a try ... catch(IllegalArgumentException) and then printing the occurring exception to the debug logs.

In the elastic-agent we use a bytebuddy ByteArrayInjectingClassloader. This comes with the downside that (a) all the bytecode of injected classes needs to be loaded eagerly and (b) the resource lookup for .class files is either inconsistent or costs heap because the bytecode byte[] need to be kept in memory.
This is something I wanted to improve here by using the ClassCopySource instead. I did my best of basically "inlining" the required logic from ByteArrayClassloader for defining classes here.

You can find the bytebuddy code for defining the package here. It is in theory prone to the same problem, but we have never encountered it in the elastic apm agent so far.

[laurit]

tomcat ignore is https://github.com/apache/tomcat/blob/39c388b51817d346031189a8a942340b6e46b940/java/org/apache/catalina/loader/WebappClassLoaderBase.java#L2280
URLClassLoader catches it and checks whether package is defined before failing https://github.com/openjdk/jdk8u/blob/587090ddc17c073d56f4d3f52b61f6477d6322b0/jdk/src/share/classes/java/net/URLClassLoader.java#L437
I think either of these approaches is fine. In my opinion logging the exception isn't necessary, if you feel it is necessary do make sure to log it at a level where it doesn't get printed to stdout unless debug is enabled.

[JonasKunz]

I think reverifying in the catch and rethrowing if the package doesn't exist is the best approach, as this way we won't loose IllegalArgumentExceptions which have a different root cause.

[trask]

nice test, I like the "markers" 👍

[JonasKunz]

I think the test failures are unrelated flaky tests. Would be great if someone with the permissions could re-trigger the build, because I don't have the permissions to do that it seems.

[mateuszrzeszutek]

👍