-
Notifications
You must be signed in to change notification settings - Fork 1k
Don't use unsafe on jdk23+ #15091
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
base: main
Are you sure you want to change the base?
Don't use unsafe on jdk23+ #15091
Conversation
| -PtestJavaVersion=${{ matrix.test-java-version }} | ||
| -PtestJavaVM=${{ matrix.vm }} | ||
| -PtestIndy=${{ matrix.test-indy }} | ||
| -PdenyUnsafe=${{ matrix.test-java-version >= 23 && 'true' || 'false' }} |
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.
Run jdk25 tests with unsafe disabled to verify that everything works. I guess it is an open question whether we want to test without unsafe at all at this point or whether we want to do it in some other way as running with unsafe requires disabling tests that use unsafe.
| import java.util.function.Supplier; | ||
|
|
||
| /** | ||
| * Helper class that provides a MethodHandles.Lookup that allows defining classes in this package. |
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.
This won't be needed with indy advice that would define helper classes in a separate class loader. With inline advice this class provides access to a lookup that lets us define classes in the package used by the executors instrumentation.
| * jdk.internal.misc.Unsafe. Used when jdk provided sun.misc.Unsafe is not available which can | ||
| * happen when running modular application without jdk.unsupported module. | ||
| */ | ||
| class SunMiscUnsafeGenerator { |
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.
When a modular application was run without jdk.unsupported module we generate a copy of sun.misc.Unsafe that delegated to jdk.internal.misc.Unsafe. Since we don't rely on sun.misc.Unsafe any more we don't need this.
| private void injectBootstrapClassLoader(Map<String, Supplier<byte[]>> inject) throws IOException { | ||
| static { | ||
| // add lookups for instrumentations that define classes in boot loader | ||
| addPackageLookup(new ExecutorLookupSupplier()); |
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.
Providing lookup that can define classes in these packages isn't strictly necessary because there is a fallback to ClassInjector.UsingInstrumentation. While ClassInjector.UsingInstrumentation works it requires packaging classes to a jar and using Instrumentation.appendToBootstrapClassLoaderSearch. Apparently every time that method is called
OpenJDK 64-Bit Server VM warning: Sharing is only supported for boot loader classes because bootstrap classpath has been appended
is printed, which is a minor annoyance. Another potential issue, which I didn't verify, is that on windows the jars that are appended to boot loader can't probably be deleted, at least not right away. We might need to check whether this is true or whether they are deleted only on exit. Could also depend on the vm version.
instrumentation/log4j/log4j-appender-2.17/javaagent/build.gradle.kts
Outdated
Show resolved
Hide resolved
| // in the constructor of DefaultStreamMessage replace new MpscChunkedArrayQueue with new | ||
| // ConcurrentLinkedQueue |
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.
😂❤️
| // using a system property here will enable the instrumentation when declarative config is used | ||
| return Boolean.getBoolean("otel.instrumentation.deny-unsafe.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.
cc @zeitlinger
No description provided.