Don't use unsafe on jdk23+ #15091
Don't use unsafe on jdk23+ #15091
Conversation
.github/workflows/build-common.yml
Outdated
| -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.
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.
There was a problem hiding this comment.
how about adding it to the matrix so we test 25 both with and without denyUnsafe, e.g.
deny-unsafe:
- false
- true
exclude:
- test-java-version: 8
deny-unsafe: true
- test-java-version: 11
deny-unsafe: true
- test-java-version: 17
deny-unsafe: true
- test-java-version: 21
deny-unsafe: true
There was a problem hiding this comment.
did it a bit differently, added 25-deny-unsafe to test-java-version
| 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.
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.
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.
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 |
| // 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.
I think it is the simplest option. I wouldn't bother with trying to enable this via declarative config. Hopefully the libraries we use for testing will eventually get update and we won't need this instrumentation at all.
There was a problem hiding this comment.
@zeitlinger probably because the comment in code mentions declarative config
There was a problem hiding this comment.
we also have other options that don't support declarative config - mostly because they are needed early or are internal (such as this one)
If they are user facing, they should be added to https://opentelemetry.io/docs/zero-code/java/agent/declarative-configuration/#environment-variables-and-system-properties-only-options
3 participants
No description provided.