Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implemented InstrumentationModuleClassLoader for invokedynamic Advice dispatching #9177
Implemented InstrumentationModuleClassLoader for invokedynamic Advice dispatching #9177
Changes from 8 commits
7a71b65
0f2d140
89d9005
baa567e
aa2215b
b7e42f9
ed3de5b
2547bda
9cc0492
c28a853
71a0210
f820ede
712ea36
0eca9c5
ee912f0
0d3829d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 whenfindPackage
is called but already is defined whendefinePackage
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 sameIllegalArgumentException
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any idea how to guard against this?
I can only think of wrapping the
definePackage
in atry ... 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 bytecodebyte[]
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 fromByteArrayClassloader
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.