Safepoint metrics no longer return warnings

[j-baker]

Still cross-jvm compatible due to usage of inner class. Generally this is a bit heinous as a tactic, though I don't really see a better way without hacks. Java won't let us compile against the code with our javac settings, while using reflection directly will trigger the detection mechanisms.

[changelog-app]

Generate changelog in changelog/@unreleased

Type

• Feature
• Improvement
• Fix
• Break
• Deprecation
• Manual task
• Migration

Description

Safepoint metrics no longer return illegal access warnings

Check the box to generate changelog(s)

• Generate changelog entry

[carterkozak]

I've begun the process of unblocking a proper fix via module-info:
palantir/safe-logging#515
palantir/gradle-baseline#1632

[j-baker]

@carterkozak your conclusion was that this does not in fact unblock a proper fix, right?

[schlosna]

@j-baker Will this address the runtime warnings in #482 as we're still reflectively getting the management bean?

[carterkozak]

I don't think there's a proper way to access safepoint time in jdk9+ without warnings, and 16+ will throw unless we specify --illegal-access=permit or --add-opens <value>. I'm not as familiar with the module-info type as I should be, but I haven't found any options to retain reflective access to internals without modifying jvm args.

[carterkozak]

This branch doesn't appear to resolve the problem on jdk16, however it does avoid illegal reflective access warnings on older builds.

java.lang.IllegalAccessError: class net.bytebuddy.renamed.java.lang.Object$ByteBuddy$5fCJ5exq (in unnamed module @0x3336e6b6) cannot access class sun.management.ManagementFactoryHelper (in module java.management) because module java.management does not export sun.management to unnamed module @0x3336e6b6

	at net.bytebuddy.renamed.java.lang.Object$ByteBuddy$5fCJ5exq.<clinit>(Unknown Source)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:78)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:480)
	at com.palantir.tritium.metrics.jvm.SafepointMetrics.getGauge(SafepointMetrics.java:106)

[carterkozak]

I think this PR has the right idea, but instead of using byte-buddy to generate non-reflective accesses we should use java8 javac. Setup will be a little odd, so I'd propose a new project which publishes a jar with a minimal API around accessing sun.management.HotspotRuntimeMBean when it's available.

We use the safepoint time internally in our server project, so it will be helpful to share the extraction code instead of duplicating it.

Thoughts?

[schlosna]

@carterkozak curious which JRE compatibility versions you're thinking about targeting. Would we still target a JRE 8 for now? Would we create a multi-release JAR with JRE 8 and 9+ (with proper module-info declarations) classes?

[carterkozak]

I haven’t been able to get module-info to work at all when encapsulation is enforced because the sun packages are not exposed. When we need module-info definitions we’ll have to decide between bumping the minimum version to java 11, or multi-release jars. I hope that by the time it’s necessary we’ll be able to safely require java 11, mr jars are difficult to test.

I can build a jar using java 8 which references the classes without reflection and it resolves the reflective access warnings, however java 16 still fails.

[j-baker]

@carterkozak maybe an easier fix is to have bytebuddy generate java 8 bytecode?

[carterkozak]

I think we can close this out now that #999 has merged, now we use java 8 bytecode from https://github.com/palantir/jvm-diagnostics

[carterkozak]

Closing, this has been resolved