[GR-32289] Programmatic serialization registration from inside features #3050

kkriske · 2020-12-06T17:40:02Z

It is only possible to add serialization registrations from serialization-config.json files, these changes would add a RuntimeSerialization class which is accessible from within feature implementations so during image building, classes can be registered for serialization programatically using RuntimeSerialization.register(YourClass.class).

This PR does 3 things:

add programatical registration of serialization classes
change the output of the serialization-config.json file to write out array classes as org.example.ClassName[] instead of [org.example.ClassName;
support reading in the []-suffix syntax from the config file, so this also fixes allow serialization config to contain java source style syntax classnames #3048

olpaw

Please rebase once #3060 is on master

kkriske · 2020-12-12T12:18:51Z

rebased & ready for review
Not sure why travis failed one of the checks.

kkriske · 2020-12-16T16:27:19Z

Current conflicts are caused by the introduction of the serialization-deny-config.json file, and I would like some pointers on what the preferred resolution is, as there are a couple of possibilities:

also provide the capability to set denied classes from features, which is the most flexible, but can cause conflicts:
- A class that has already been added for serialization can suddenly be in the deny list, at which point we can only fail since the serialization classes may already have been generated.
- A class that has already been denied can be added for serialization, which is currently handled with a warning, but for consistency with the first point probably should crash, since if they're added in a different order this is equal to the first point. I don't think we can rely on the order in which they are added to be consistent?
don't allow adding denied classes from features, which can keep the current way of handling collisions with warnings.

olpaw · 2020-12-17T08:09:30Z

Hi @kkriske

don't allow adding denied classes from features, which can keep the current way of handling collisions with warnings.

There is no value in adding to the deny-list via Feature API. Passing serialization-deny-config.json to an image build is enough to make sure certain classes that are considered to be a security risk if serialized/deserialized are excluded.

BTW, I will be off from next week until January. It is very likely I won't find the time to merge this before.

kkriske · 2020-12-17T17:37:00Z

Should you still find the time, it is again ready for review. Happy holidays.

olpaw · 2021-01-07T11:47:22Z

...rc/org.graalvm.nativeimage/src/org/graalvm/nativeimage/impl/RuntimeSerializationSupport.java

+
+public interface RuntimeSerializationSupport {
+
+    default void register(Class<?>... classes) {


Having a default method here just to save a few lines of code over here

graal/substratevm/src/com.oracle.svm.reflect/src/com/oracle/svm/reflect/serialize/hosted/SerializationFeature.java

Line 98 in 561fc00

new SerializationRegistryAdapter((clazz, checkSums) -> deniedClasses.put(clazz, true), imageClassLoader));

seems like a bad idea.

sdk/src/org.graalvm.nativeimage/src/org/graalvm/nativeimage/hosted/RuntimeSerialization.java

olpaw · 2021-01-07T15:42:07Z

...com.oracle.svm.reflect/src/com/oracle/svm/reflect/serialize/hosted/SerializationFeature.java

@@ -209,6 +200,28 @@ static void println(String str) {
        System.out.println(str);
        // Checkstyle: resume
    }
+
+    private class RuntimeSerializationSupportImpl extends SerializationSupport implements RuntimeSerializationSupport {


RuntimeSerializationSupportImpl must not be an inner class of hosted class SerializationFeature. It extends com.oracle.svm.reflect.serialize.SerializationSupport which holds data that will be part of an image. We do not allow any hosted classes to get built into an image. If you build an image that uses serialization with image builder assertions enabled (native-image -J-ea) you would run into issues with com.oracle.svm.hosted.NativeImageGenerator#checkName.

This is no longer an inner class.
I was however unable to recreate any issues with -J-ea with the previous implementation, so couldn't check if the current implementation mitigates that issue.

olpaw

See comments above.

...com.oracle.svm.reflect/src/com/oracle/svm/reflect/serialize/hosted/SerializationFeature.java

...om.oracle.svm.reflect/src/com/oracle/svm/reflect/serialize/hosted/SerializationRegistry.java

olpaw

See comments

olpaw · 2021-01-25T10:33:27Z

@kkriske serialization support got shipped as part of 21.0 and on master we merged one more fix that was necessary to support a rather exotic but still important usecase. I.e. the source code related to serialization is now stable enough so that your PR should not run into constant rebasing issues anymore if you want to give it one more try.

olpaw · 2021-08-05T09:41:41Z

@kkriske had to add kkriske@05f48f7 because mx eclipsformat failed in graal/sdk.

peter-hofer

Great contribution, thank you!

peter-hofer · 2021-08-05T11:39:41Z

...evm/src/com.oracle.svm.reflect/src/com/oracle/svm/reflect/serialize/hosted/package-info.java

+ */
+
+@Platforms(Platform.HOSTED_ONLY.class)
+package com.oracle.svm.reflect.serialize.hosted;


Shouldn't this already match our hosted-only package filter?

This is fine. It's the same as in svm/reflect/proxy/hosted/package-info.java:26

peter-hofer · 2021-08-05T11:41:34Z

substratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/SerializationRegistrationTest.java

+    }
+}
+
+@AutomaticFeature


It's dangerous to have a test-specific feature as an @AutomaticFeature like this because it might be enabled for other tests or on an even broader scope. Please use @RequiredFeatures.

Unfortunately @RequiredFeatures is part of com.oracle.svm.enterprise.test.runner.RequiredFeatures
I suggested using @AutomaticFeature because we are also using it in src/com/oracle/svm/test/NativeImageResourceFileSystemProviderTest.java.

@kkriske alternatively to @peter-hofer s suggestion we can do the following:

Remove the @AutomaticFeature annotation here

Add --features=com.oracle.svm.test.SerializationRegistrationTestFeature to Args in substratevm/src/com.oracle.svm.test/src/META-INF/native-image/com.oracle.svm.test/native-image.properties

This has the same effect as using @AutomaticFeature without the risk of having that feature enabled in other context then testing.

peter-hofer · 2021-08-05T11:48:14Z

...rc/org.graalvm.nativeimage/src/org/graalvm/nativeimage/impl/RuntimeSerializationSupport.java

+    void registerWithTargetConstructorClass(Class<?> clazz, Class<?> customTargetConstructorClazz);
+
+    void registerWithTargetConstructorClass(String className, String customTargetConstructorClassName);


If these are not exposed through RuntimeSerialization, we should move them to an internal subinterface.

@kkriske please also add these two to org.graalvm.nativeimage.hosted.RuntimeSerialization. There are cases were users have to use them and I guess they would be happy if they could it via the API.

Wouldn't the first one be enough? The (String, String) method is mostly there to facilitate the config files. When called from RuntimeSerialization it will crash if the String arguments do not resolve to an actual class so you might as well resolve them first before calling the (Class<?>, Class<?>) version.

Yes the first one is enough.

olpaw · 2021-08-05T12:27:43Z

@kkriske for any new changes on this PR please add new commits instead of rebasing your existing commits so that we can keep track of what was added when by whom.

olpaw · 2021-08-05T12:32:11Z

@kkriske 091c0e1 is needed for module system support. When the image builder runs on the module-path the access of package org.graalvm.nativeimage.impl of module org.graalvm.sdk from module org.graalvm.nativeimage.configure needs to allowed.

olpaw · 2021-08-05T12:45:58Z

@kkriske our CI run found one more issue where I can observe the following stacktrace:

java.lang.NullPointerException: null
    at java.util.Comparator.lambda$comparing$77a9974f$1(Comparator.java:469)
    at java.util.Comparator.lambda$thenComparing$36697e65$1(Comparator.java:217)
    at java.util.TimSort.countRunAndMakeAscending(TimSort.java:355)
    at java.util.TimSort.sort(TimSort.java:220)
    at java.util.Arrays.sort(Arrays.java:1512)
    at java.util.ArrayList.sort(ArrayList.java:1464)
    at com.oracle.svm.configure.config.SerializationConfiguration.printJson(SerializationConfiguration.java:63)
    at com.oracle.svm.agent.tracing.ConfigurationResultWriter.writeToDirectory(ConfigurationResultWriter.java:89)
    at com.oracle.svm.agent.NativeImageAgent.writeConfigurationFiles(NativeImageAgent.java:508)
    at com.oracle.svm.agent.NativeImageAgent.onUnloadCallback(NativeImageAgent.java:581)
    at com.oracle.svm.jvmtiagentbase.JvmtiAgentBase.onUnload(JvmtiAgentBase.java:267)

I'm trying to reproduce this locally now.

kkriske · 2021-08-05T14:00:33Z

@kkriske our CI run found one more issue where I can observe the following stacktrace:

java.lang.NullPointerException: null
    at java.util.Comparator.lambda$comparing$77a9974f$1(Comparator.java:469)
    at java.util.Comparator.lambda$thenComparing$36697e65$1(Comparator.java:217)
    at java.util.TimSort.countRunAndMakeAscending(TimSort.java:355)
    at java.util.TimSort.sort(TimSort.java:220)
    at java.util.Arrays.sort(Arrays.java:1512)
    at java.util.ArrayList.sort(ArrayList.java:1464)
    at com.oracle.svm.configure.config.SerializationConfiguration.printJson(SerializationConfiguration.java:63)
    at com.oracle.svm.agent.tracing.ConfigurationResultWriter.writeToDirectory(ConfigurationResultWriter.java:89)
    at com.oracle.svm.agent.NativeImageAgent.writeConfigurationFiles(NativeImageAgent.java:508)
    at com.oracle.svm.agent.NativeImageAgent.onUnloadCallback(NativeImageAgent.java:581)
    at com.oracle.svm.jvmtiagentbase.JvmtiAgentBase.onUnload(JvmtiAgentBase.java:267)

I'm trying to reproduce this locally now.

graal/substratevm/src/com.oracle.svm.configure/src/com/oracle/svm/configure/config/SerializationConfiguration.java

Lines 63 to 64 in 1b212eb

    
           list.sort(Comparator.comparing(SerializationConfigurationType::getQualifiedJavaName) 
        
                           .thenComparing(SerializationConfigurationType::getQualifiedCustomTargetConstructorJavaName));

getQualifiedCustomTargetConstructorJavaName can return null, so if I'm not mistaken, the exception occurs when there are multiple config entries for the same class, without custom target constructor. Or at least one entry without custom target constructor I suppose.

Adding a Comparator.nullsFirst around the second compare should fix the issue. But maybe making the SerializationConfigurationType implement Comparable would be preferred?

olpaw · 2021-08-05T14:51:26Z

But maybe making the SerializationConfigurationType implement Comparable would be preferred?

Yes that would be preferred.
Something like

    @Override
    public int compareTo(SerializationConfigurationType other) {
        int res = qualifiedJavaName.compareTo(other.qualifiedJavaName);
        if (res != 0) {
            return res;
        }
        Comparator<String> nullsFirstCompare = Comparator.nullsFirst(Comparator.naturalOrder());
        return nullsFirstCompare.compare(qualifiedCustomTargetConstructorJavaName, other.qualifiedCustomTargetConstructorJavaName);
    }

…in SerializationConfigurationType sorting

kkriske · 2021-08-05T18:41:00Z

I have no idea what the special case that requires registerWithTargetConstructorClass actually is, so I'm afraid I'm not able to add a test for that call with my current knowledge.

olpaw · 2021-08-06T08:52:14Z

I have no idea what the special case that requires registerWithTargetConstructorClass actually is, so I'm afraid I'm not able to add a test for that call with my current knowledge.

E.g. apache.spark makes direct use of ReflectionFactory.newConstructorForSerialization with a customTargetConstructorClass. Providing a test that covers this special case is beyond the scope of this PR.

olpaw · 2021-08-06T14:50:24Z

@kkriske your PR passed our CI and is now in the merge queue (eta early next week).
Have a nice weekend!

See oracle/graal#3050 Closes quarkusio#19338

`SerializationRegistry` has been moved from `com.oracle.svm.core.jdk.serialize` to `com.oracle.svm.reflect.serialize` in Graal oracle/graal#3050 Closes quarkusio#19338

`SerializationRegistry` class has been moved from `com.oracle.svm.core.jdk.serialize` to `com.oracle.svm.reflect.serialize` `addReflections` method moved from `com.oracle.svm.reflect.serialize.hosted.SerializationFeature` to `com.oracle.svm.reflect.serialize.hosted.SerializationBuilder` and became `private` See oracle/graal#3050 Note: oracle/graal#3050 also introduces `SerializationFeature.register(Class<?>... classes)` which should be a better alternative to the generated by Quarkus `registerSerializationForClass` method. Closes quarkusio#19338

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

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 (cherry picked from commit 4921b9f)

RuntimeSerialization#registerIncludingAssociatedClasses was introduced in GraalVM 21.3 by oracle#3050

RuntimeSerialization#registerIncludingAssociatedClasses was introduced in GraalVM 21.3 by #3050

graalvmbot added the oca-signed label Dec 6, 2020

kkriske force-pushed the serialization-registration-from-inside-features branch 3 times, most recently from 4dc8d37 to 87a1c27 Compare December 6, 2020 19:28

olpaw self-requested a review December 11, 2020 13:27

olpaw requested changes Dec 11, 2020

View reviewed changes

kkriske force-pushed the serialization-registration-from-inside-features branch 2 times, most recently from d323592 to f64be27 Compare December 12, 2020 11:55

kkriske force-pushed the serialization-registration-from-inside-features branch 2 times, most recently from e238f57 to acea686 Compare December 17, 2020 13:45

kkriske force-pushed the serialization-registration-from-inside-features branch 3 times, most recently from 0128724 to 561fc00 Compare December 20, 2020 17:52

olpaw reviewed Jan 7, 2021

View reviewed changes

sdk/src/org.graalvm.nativeimage/src/org/graalvm/nativeimage/hosted/RuntimeSerialization.java Show resolved Hide resolved

olpaw reviewed Jan 7, 2021

View reviewed changes

olpaw requested changes Jan 7, 2021

View reviewed changes

kkriske force-pushed the serialization-registration-from-inside-features branch 2 times, most recently from 9cef9bd to c1e8cfe Compare January 11, 2021 10:53

olpaw reviewed Jan 11, 2021

View reviewed changes

...com.oracle.svm.reflect/src/com/oracle/svm/reflect/serialize/hosted/SerializationFeature.java Outdated Show resolved Hide resolved

olpaw reviewed Jan 11, 2021

View reviewed changes

...om.oracle.svm.reflect/src/com/oracle/svm/reflect/serialize/hosted/SerializationRegistry.java Outdated Show resolved Hide resolved

olpaw requested changes Jan 11, 2021

View reviewed changes

kkriske force-pushed the serialization-registration-from-inside-features branch 3 times, most recently from f1c7dad to 95dbeae Compare January 11, 2021 14:55

olpaw changed the title ~~[GR-32289] Programatic serialization registration from inside features~~ [GR-32289] Programmatic serialization registration from inside features Aug 5, 2021

Style fix

05f48f7

peter-hofer approved these changes Aug 5, 2021

View reviewed changes

Add missing module export

091c0e1

Add 'registerWithTargetConstructorClass' call to public API, fix NPE …

8e12b11

…in SerializationConfigurationType sorting

Improve API method documentation

12fadf1

graalvmbot merged commit a1b86ff into oracle:master Aug 10, 2021

kkriske deleted the serialization-registration-from-inside-features branch August 10, 2021 07:55

zakkak mentioned this pull request Aug 11, 2021

CoreSerializationInGraalITCase of integration test Main fails with latest GraalVM quarkusio/quarkus#19338

Closed

zakkak added a commit to zakkak/quarkus that referenced this pull request Aug 12, 2021

SerializationRegistry class moved in GraalVM 21.3-dev

5a5329c

See oracle/graal#3050 Closes quarkusio#19338

zakkak mentioned this pull request Aug 19, 2021

Use new SerializationFeature.register API with GraalVM/Mandrel >=21.3 quarkusio/quarkus#19520

Merged

zakkak added a commit to zakkak/mandrel that referenced this pull request Jun 9, 2022

Fix RuntimeSerialization#registerIncludingAssociatedClasses @SInCE

6a8010c

RuntimeSerialization#registerIncludingAssociatedClasses was introduced in GraalVM 21.3 by oracle#3050

zakkak mentioned this pull request Jun 9, 2022

[GR-39144] Fix RuntimeSerialization#registerIncludingAssociatedClasses @since #4631

Closed

graalvmbot pushed a commit that referenced this pull request Jun 13, 2022

Fix RuntimeSerialization#registerIncludingAssociatedClasses @SInCE

1656f5f

RuntimeSerialization#registerIncludingAssociatedClasses was introduced in GraalVM 21.3 by #3050

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GR-32289] Programmatic serialization registration from inside features #3050

[GR-32289] Programmatic serialization registration from inside features #3050

kkriske commented Dec 6, 2020

olpaw left a comment

kkriske commented Dec 12, 2020

kkriske commented Dec 16, 2020

olpaw commented Dec 17, 2020

kkriske commented Dec 17, 2020

olpaw Jan 7, 2021

kkriske Jan 11, 2021

olpaw Jan 7, 2021

kkriske Jan 11, 2021

olpaw left a comment

olpaw left a comment

olpaw commented Jan 25, 2021 •

edited

Loading

olpaw commented Aug 5, 2021

peter-hofer left a comment

peter-hofer Aug 5, 2021

olpaw Aug 5, 2021

peter-hofer Aug 5, 2021

olpaw Aug 5, 2021 •

edited

Loading

peter-hofer Aug 5, 2021 •

edited

Loading

olpaw Aug 5, 2021

kkriske Aug 5, 2021

olpaw Aug 5, 2021

olpaw commented Aug 5, 2021

olpaw commented Aug 5, 2021

olpaw commented Aug 5, 2021

kkriske commented Aug 5, 2021 •

edited

Loading

olpaw commented Aug 5, 2021 •

edited

Loading

kkriske commented Aug 5, 2021

olpaw commented Aug 6, 2021

olpaw commented Aug 6, 2021


		public interface RuntimeSerializationSupport {

		default void register(Class<?>... classes) {

		void registerWithTargetConstructorClass(Class<?> clazz, Class<?> customTargetConstructorClazz);

		void registerWithTargetConstructorClass(String className, String customTargetConstructorClassName);

[GR-32289] Programmatic serialization registration from inside features #3050

[GR-32289] Programmatic serialization registration from inside features #3050

Conversation

kkriske commented Dec 6, 2020

olpaw left a comment

Choose a reason for hiding this comment

kkriske commented Dec 12, 2020

kkriske commented Dec 16, 2020

olpaw commented Dec 17, 2020

kkriske commented Dec 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olpaw left a comment

Choose a reason for hiding this comment

olpaw left a comment

Choose a reason for hiding this comment

olpaw commented Jan 25, 2021 • edited Loading

olpaw commented Aug 5, 2021

peter-hofer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olpaw Aug 5, 2021 • edited Loading

Choose a reason for hiding this comment

peter-hofer Aug 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olpaw commented Aug 5, 2021

olpaw commented Aug 5, 2021

olpaw commented Aug 5, 2021

kkriske commented Aug 5, 2021 • edited Loading

olpaw commented Aug 5, 2021 • edited Loading

kkriske commented Aug 5, 2021

olpaw commented Aug 6, 2021

olpaw commented Aug 6, 2021

olpaw commented Jan 25, 2021 •

edited

Loading

olpaw Aug 5, 2021 •

edited

Loading

peter-hofer Aug 5, 2021 •

edited

Loading

kkriske commented Aug 5, 2021 •

edited

Loading

olpaw commented Aug 5, 2021 •

edited

Loading