Skip to content
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

Access RecordComponent via Class.forName. #2465

Merged
merged 1 commit into from
Aug 11, 2023

Conversation

eamonnmcmanus
Copy link
Member

@eamonnmcmanus eamonnmcmanus commented Aug 10, 2023

This should mean that GraalVM will understand the reflective lookup of its methods. See documentation.

Fixes #2464.

This should mean that GraalVM will understand the reflective lookup of its methods.
See [documentation](https://www.graalvm.org/latest/reference-manual/native-image/dynamic-features/Reflection/#automatic-detection).
Copy link
Collaborator

@Marcono1234 Marcono1234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and it appears your investigation is correct. If I change the code of ReflectionHelper to rethrow the NoSuchMethodException and then use the GraalVM Native Image Maven Plugin, the exception is indeed about a method of RecordComponent:

Caused by: java.lang.NoSuchMethodException: java.lang.reflect.RecordComponent.getName()
    java.base@17.0.8/java.lang.Class.checkMethod(DynamicHub.java:1038)
    java.base@17.0.8/java.lang.Class.getMethod(DynamicHub.java:1023)
    com.google.gson.internal.reflect.ReflectionHelper$RecordSupportedHelper.<init>(ReflectionHelper.java:223)
    com.google.gson.internal.reflect.ReflectionHelper$RecordSupportedHelper.<init>(ReflectionHelper.java:212)
    com.google.gson.internal.reflect.ReflectionHelper.<clinit>(ReflectionHelper.java:35)
    [...]

With your change it becomes a Graal exception which the user probably has to fix (and which should not or cannot be solved by Gson I assume):

Caused by: com.oracle.svm.core.jdk.UnsupportedFeatureError: Record components not available for record class com.google.gson.native_test.Java17RecordTest$MyRecord. All record component accessor methods of this record class must be included in the reflection configuration at image build time, then this method can be called.
    org.graalvm.nativeimage.builder/com.oracle.svm.core.util.VMError.unsupportedFeature(VMError.java:92)
    java.base@17.0.8/java.lang.Class.getRecordComponents0(DynamicHub.java:1160)
    java.base@17.0.8/java.lang.Class.getRecordComponents(DynamicHub.java:2436)
    [...]

(though I am not familiar with Graal so I might have made a mistake in my test setup)

@Marcono1234
Copy link
Collaborator

Do you think it would be worth adding a GraalVM Native Image integration test to Gson? I have already an unfinished proof of concept locally which uses the Testing Support of the GraalVM Native Image Maven Plugin, and that seems to work.

However, because execution requires GraalVM and because building the native image for testing is quite resource intensive (more than 1GB of memory for me, and 100% CPU usage) it would probably only make sense to have that enabled for the GitHub workflow, and not be part of the normal Maven build which you execute locally.
Though because it is so resource intensive I am not sure if this is really worth it, what do you think?

@eamonnmcmanus
Copy link
Member Author

If you already have such a test, I think it would be a great idea to add it to the CI. I felt bad about not adding a test for this PR, and I think your test could potentially cover that. If it turns out to be a really huge burden on the CI then we might change our minds, but otherwise I think it's good to be covered.

@eamonnmcmanus eamonnmcmanus merged commit c5c1fbf into google:main Aug 11, 2023
@eamonnmcmanus eamonnmcmanus deleted the graal branch August 11, 2023 17:57
@eamonnmcmanus
Copy link
Member Author

By the way, thanks for checking all those details!

@Marcono1234
Copy link
Collaborator

By the way, thanks for checking all those details!

No problem! It was also interesting for me to become a bit more familiar with this. Though I am still not that familiar with it, so I hope the integration test added in #2466 is reasonable and correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using gson to deserialize json in native image fails
2 participants