-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Use new SerializationFeature.register API with GraalVM/Mandrel >=21.3 #19520
Conversation
CI run with 21.3-dev: https://github.com/graalvm/mandrel/actions/runs/1148784045 (please don't merge before it completes) |
The tests are looking good, but I am switching this to draft to make it use |
Great work with the fix and thanks for spotting this early on @zakkak! 👏 |
e7f1750
to
dfedfb2
Compare
New CI run to test branch with 21.3: https://github.com/graalvm/mandrel/actions/runs/1150563667 |
Yeah, thanks @zakkak ! I'll have a look early next week. |
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building dfedfb2
Full information is available in the Build summary check run. Test Failures⚙️ Gradle Tests - JDK 11 Windows #📦 integration-tests/gradle✖
⚙️ JVM Tests - JDK 11 #📦 integration-tests/hibernate-reactive-panache✖
|
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.
Spotted a small typo and added a question.
@zakkak I suppose we want this in 2.2?
ofMethod("com.oracle.svm.reflect.serialize.hosted.SerializationFeature", "addReflections", void.class, | ||
Class.class, Class.class), | ||
clazz, objectClass); | ||
ResultHandle addRedlectionsArgs2 = tc.newArray(Class.class, tc.load(2)); |
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.
ResultHandle addRedlectionsArgs2 = tc.newArray(Class.class, tc.load(2)); | |
ResultHandle addReflectionsArgs2 = tc.newArray(Class.class, tc.load(2)); |
ResultHandle addRedlectionsArgs2 = tc.newArray(Class.class, tc.load(2)); | ||
tc.writeArrayValue(addRedlectionsArgs2, 0, clazz); | ||
tc.writeArrayValue(addRedlectionsArgs2, 1, objectClass); | ||
tc.invokeVirtualMethod(invokeMethodDescriptor, addReflectionsLookupMethod, tc.loadNull(), addRedlectionsArgs2); |
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.
tc.invokeVirtualMethod(invokeMethodDescriptor, addReflectionsLookupMethod, tc.loadNull(), addRedlectionsArgs2); | |
tc.invokeVirtualMethod(invokeMethodDescriptor, addReflectionsLookupMethod, tc.loadNull(), addReflectionsArgs2); |
ResultHandle addRedlectionsArgs2 = tc.newArray(Class.class, tc.load(2)); | ||
tc.writeArrayValue(addRedlectionsArgs2, 0, clazz); | ||
tc.writeArrayValue(addRedlectionsArgs2, 1, objectClass); | ||
tc.invokeVirtualMethod(invokeMethodDescriptor, addReflectionsLookupMethod, tc.loadNull(), addRedlectionsArgs2); |
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.
What is the tc.loadNull()
supposed to be?
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.
invoke
's first argument is the this
object to invoke the method on, so since this is a static method I am passing null
as the first argument.
If the underlying method is static, then the specified obj argument is ignored. It may be null.
See https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/reflect/Method.html#invoke(java.lang.Object,java.lang.Object...)
oracle/graal#3050 introduces a number of changes around the internal APIs that are being used by Quarkus to register classes for serialization thus breaking serialization support when using 21.3-dev These changes are: 1. `SerializationRegistry` class has been moved from `com.oracle.svm.core.jdk.serialize` to `com.oracle.svm.reflect.serialize` 2. `addReflections` method moved from `com.oracle.svm.reflect.serialize.hosted.SerializationFeature` to `com.oracle.svm.reflect.serialize.hosted.SerializationBuilder` and became `private` oracle/graal#3050 also introduces `SerializationFeature.register(Class<?>... classes)` which is available as a public API (not internal) and can save us from having to keep up with the internal APIs as new releases come out. This PR leverages the new API to register classes for serialization when using GraalVM/Mandrel >=21.3. Closes quarkusio#19338
dfedfb2
to
4921b9f
Compare
Good catch! Fixed and answered respectively :)
Yes we will eventually need it in 2.2. |
Thanks, nice work! |
oracle/graal#3050 introduces a number of changes
around the internal APIs that are being used by Quarkus to register
classes for serialization thus breaking serialization support when using
21.3-dev
These changes are:
SerializationRegistry
class has been moved fromcom.oracle.svm.core.jdk.serialize
tocom.oracle.svm.reflect.serialize
addReflections
method moved fromcom.oracle.svm.reflect.serialize.hosted.SerializationFeature
tocom.oracle.svm.reflect.serialize.hosted.SerializationBuilder
andbecame
private
oracle/graal#3050 also introduces
SerializationFeature.register(Class<?>... classes)
which is availableas a public API (not internal) and can save us from having to keep up
with the internal APIs as new releases come out. This PR leverages the
new API to register classes for serialization when using GraalVM/Mandrel >=21.3.
Closes #19338