-
Notifications
You must be signed in to change notification settings - Fork 867
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
Remove virtual field interfaces from reflection results #4722
Conversation
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 hope you know what you are doing :D Instrumenting java.lang.Class
seems scary
@laurit do you think we could get rid of these interfaces and use method handles to access the generated methods? |
@trask Firstly we would need to figure out a new way to detect whether virtual fields have been added to a class which is currently handled by testing for |
@@ -36,6 +38,7 @@ private ReflectionHelper() {} | |||
return methods; | |||
} else if (containingClass.isInterface() |
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.
If filtering of the interfaces works then this branch shouldn't evaluate to true. Similarly I suspect that https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/internal/internal-proxy/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/proxy/ProxyHelper.java might not be necessary any more.
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.
nice, I'll open an issue to try this 👍
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.
presence of
<clinit>
method also changes the default serial version uid.
😭
@@ -36,6 +38,7 @@ private ReflectionHelper() {} | |||
return methods; | |||
} else if (containingClass.isInterface() |
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.
nice, I'll open an issue to try this 👍
…ry#4722) * Remove virtual field interfaces from reflection results * fix java8 and openj9
Resolves #4370
I initially planned to use asm
SerialVersionUIDAdder
to addserialVersionUID
field to serializable classes that don't declare it. This is needed to make sure that instrumented version of class has the sameserialVersionUID
as uninstrumented version. This approach failed with one of spring-batch tests because inside spring there is code that copies instances of an internal spring class via reflection and fails because it can't write toserialVersionUID
which is a static final field. We can't use the same reflection filtering code as we use for fields & methods added by virtual field implementetion because that filtering completely removes these members from reflection results. ForserialVersionUID
we'd need it to be visible fromgetDeclaredField
, so thatObjectStreamClass
can read it, and hidden fromgetDeclaredFields
, so that code that isn't explicitly looking for it wouldn't stumble upon it. Instead of attempting to removeserialVersionUID
fromgetDeclaredFields
I chose to remove interfaces added by virtual field implementation from the result ofgetInterfaces
. This makes the defaultserialVersionUID
computation algorithm return the correct result on instrumented classes and there is no need to precomputeserialVersionUID
for them.