Reinitialize the com.google.protobuf.UnsafeUtil class at runtime #36642
Conversation
This comment has been minimized.
This comment has been minimized.
| @@ -75,7 +75,8 @@ NativeImageConfigBuildItem nativeImageConfiguration() { | |||
| .addRuntimeInitializedClass("io.grpc.netty.Utils") | |||
| .addRuntimeInitializedClass("io.grpc.netty.NettyServerBuilder") | |||
| .addRuntimeInitializedClass("io.grpc.netty.NettyChannelBuilder") | |||
| .addRuntimeInitializedClass("io.grpc.internal.RetriableStream"); | |||
| .addRuntimeInitializedClass("io.grpc.internal.RetriableStream") | |||
| .addRuntimeReinitializedClass("com.google.protobuf.UnsafeUtil"); | |||
There was a problem hiding this comment.
What's the diff between Initialized and Reinitialized class ?
There was a problem hiding this comment.
Re-initialized re-run the static initialization at runtime.
In this case, if you use runtime initialized, a lot of the things we do at build time would need to be at runtime (because all the users of the UnsafeUtil would need to be changed). With re-initialized, we will recompute the offset at runtime (which is the root of the issue).
So https://github.com/protocolbuffers/protobuf/blob/main/java/core/src/main/java/com/google/protobuf/UnsafeUtil.java#L21-L59 will be recomputed at runtime.
|
Actually, before merging this PR, it would be great to test with GraalVM EE. |
|
Ok, it does NOT work.... |
Also fix the Unsafe accessor. Fix quarkusio#30293. This has been tested on GraalVM CE and EE.
cescoffier
force-pushed
the
reinitialize-grpc-unsafe
branch
from
122d944 to
03857b4
Compare
|
Ok, issue fixed, it should be good now. |
There was a problem hiding this comment.
LGTM, Thanks @cescoffier!
FTR I am copy pasting a snippet from a zulip discussion where this PR was discussed.
Clement Escoffier: the subsitution is not about the offset, that's the re-initialization
Clement Escoffier: but then, we got the unsafe access issue.
Clement Escoffier: My initial idea was to use a larger subs, that would recompute the field values.
Foivos Zakkak: Yes, that was what I expected. I guess you ended up preferring the re-init to keep things simpler in the code base, right?
Clement Escoffier: yes
Clement Escoffier: my point was that not a lot of things would be inlined correctly anyway (in this class)
Clement Escoffier: so, a re-init would work.
Clement Escoffier: it's better than just init, in the sense that we do have callers initialized at build time.
gsmet
added
the
triage/waiting-for-ci
Failing Jobs - Building 03857b4
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 17 Windows #- Failing: extensions/micrometer/deployment
! Skipped: extensions/micrometer-registry-prometheus/deployment extensions/scheduler/deployment integration-tests/cache and 19 more📦 extensions/micrometer/deployment✖ |
gsmet
changed the title
quarkus-bot
bot
removed
the
triage/waiting-for-ci
Adaptation of quarkusio#36642 for the shaded `com.google.protobuf.UnsafeUtil` class in kafka-clients. Fixes: quarkusio#40100
Adaptation of quarkusio#36642 for the shaded `com.google.protobuf.UnsafeUtil` class in kafka-clients. Fixes: quarkusio#40100 (cherry picked from commit dcb3411)
Adaptation of quarkusio#36642 for the shaded `com.google.protobuf.UnsafeUtil` class in kafka-clients. Fixes: quarkusio#40100 (cherry picked from commit dcb3411)
Adaptation of quarkusio#36642 for the shaded `com.google.protobuf.UnsafeUtil` class in kafka-clients. Fixes: quarkusio#40100
Fix #30293.