-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fix reflection registration for JFR eventHandler fields #3662
Conversation
|| c.getCanonicalName().equals("jdk.jfr.events.AbstractJDKEvent")) { | ||
continue; | ||
} | ||
fieldRegistration.computeIfAbsent(c, this::registerEventHandler); |
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.
RuntimeReflectionSupport
already uses a set internally for deduplication. So, you shouldn't need ConcurrentHashMap fieldRegistration
.
if (eventClass != null && access.isReachable(eventClass)) { | ||
Set<Class<?>> s = access.reachableSubtypes(eventClass); | ||
for (Class<?> c : s) { | ||
if (c.getCanonicalName().equals("jdk.jfr.Event") |
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.
Please add a comment why it is necessary to compare the canonical names (instead of just comparing the classes directly).
@mcraj017 please merge this change to master. |
@christianhaeubl sure |
Hello @mcraj017 @christianhaeubl is there any chance we could get this (and #3639) backported to a potential 21.2.0.x bug-fix release? |
@zakkak Sure, I will check about the possibility of a potential 21.2.0.x bug-fix release and let you know |
Hi @zakkak |
21.2 added JFR support but it looks like #3608 prevents some (basic to my understanding) use cases.
Sure, as it's a new feature I guess it's OK to wait for the next dev release for a fix. |
Closes #3608