Add native-image metadata for the different ChannelHandler implementations by gradinac · Pull Request #12738 · netty/netty

gradinac · 2022-08-25T14:53:55Z

Motivation:

Due to a specific reflection usage pattern in io.netty5.channel.ChannelHandlerMask#mask0, the agent cannot automatically infer the necessary conditional metadata. Therefor, we must manually add such metadata for subclasses of the ChannelHandler.

See also #12725 (comment)

Modification:

This PR adds the necessary metadata for handlers used in the Netty 5 testsuite.

Result:

Should fix #12725

Note that for user-defined ChannelHandler implementations, users will still need to supply necessary metadata, either manually or by using the tracing agent

gradinac · 2022-08-25T14:55:18Z

cc @violetagg and thank you for helping with testing and cleaning up of the generated metadata :)

normanmaurer · 2022-08-25T16:07:14Z

@gradinac its really "sad" that we need to always manually update these whenever we add more stuff :( Is there no other way to make this work ?

gradinac · 2022-08-25T16:15:17Z

I agree completely, manual metadata maintenance = bad

Right now, we don't have anything on the native-image/native-image-agent side that would help with this particular issue :/ I can push a python script to automate generating these from the collected metadata (and convert the existing bash script that collects metadata into a python script)

If the above is okay, I can create a follow up PR with the above python script

chrisvest · 2022-08-25T16:27:54Z

It would be good to have a python script. We need to have a build step that checks if this metadata is up to date, so breakage can't sneak in.

pderop · 2022-08-25T16:58:24Z

@chrisvest ,

I did not have time to fully dive into all the details, but if this may help, in Reactor-Netty, we have a Junit test which scans all handlers using reflection, and the test will fail the build in case one handler is missing from the reflect-config.json.

See reactor/reactor-netty#2432

More specifically, see the junit test which performs the checking.

gradinac · 2022-08-25T17:11:47Z

@pderop using the Reflections library in a JUnit test to find all implementations of the ChannelHandler would be nice, as we could both generate and verify this metadata, however, I'm not sure how it would best be implemented in Netty
One requirement of that approach would be to have all Netty modules on the classpath for that test

violetagg · 2022-08-25T17:13:37Z

@pderop using the Reflections library in a JUnit test to find all implementations of the ChannelHandler would be nice, as we could both generate and verify this metadata, however, I'm not sure how it would best be implemented in Netty
One requirement of that approach would be to have all Netty modules on the classpath for that test

I think that netty-all has all dependencies

normanmaurer · 2022-08-25T20:21:14Z

Also one thing I was wonder was if we should really put this in common ? Shouldn't we have the config with the right entries in each artefact that has such a handler ?

That's a good point - the reason why I put them under the common directory is because these would be available to every project, so we would only have to maintain it in one place. During the image build, only handlers that are reachable (both available on the classpath and used in the application) would be included

I'm looking into the reflections approach for automatically generating this metadata now - if this approach ends up working, I could split the generated metadata on a per-project basis

gradinac · 2022-08-27T00:28:33Z

I've tried the org.reflections based approach on this PR - in essence, it adds the ChannelHandlerMetadataUtil class that finds all subclasses of ChannelHandler in the given packages, loads up existing metadata of the target module and then compares whether there should be any changes to the metadata.
Each Netty module that subtypes ChannelHandler has a small unit test that specifies where it's metadata file lives and which packages should be searched in that module.
I've currently added this test for the transport and codec modules, and used it to automatically generate metadata.

The benefits of this approach is that it would run as a standard unit test every time there's a change and point the exact different in metadata (if any), as well as that it would generate metadata on a per-module basis, rather than storing it as a monolithic file inside netty5-common

The downsides are that we have to add a bit of boilerplate code to each project (3 pom.xml dependencies - org.reflections, gson and a test jar dependency on netty5-transport) - if there's a smarter way to do this, I can give a go at implementing it :)

Would this approach be okay? If yes, I can add the necessary bits to the rest of the Netty modules and polish up the PR

gradinac · 2022-08-27T00:35:10Z

Maybe we could automatically update the metadata file here, rather than printing a message and letting the users do it

I think its fine for now

gradinac · 2022-08-27T00:37:45Z

This one bugged my mind a bit - is there a smarter way to find out if a class originates from the test sources? It would be nice to have all of the test classes filtered automatically, and this seems to work, but it also seems a bit ugly

gradinac · 2022-08-27T00:39:47Z

Maven seems to always set the current directory to the module it's currently building/testing - this lets us look up the reflect-config.json file directly from the module sources by using a relative path here

gradinac · 2022-08-27T00:42:25Z

Ideally we don't want to re-write the metadata generation for every test, but we also don't want to bloat up the non-test sources with test specific code. This creates a test-only dependency to the test-jar of netty5-transport where the metadata-generation logic lives

gradinac · 2022-08-28T20:59:36Z

It seems like the "is this class a test class" test is failing on Windows - most likely because I messed up the path checking logic. I'll look into it

chrisvest · 2022-08-29T21:53:04Z

You'll also need to rebase to avoid the build failing on some certificate validation. Some test certificates expired and have been replaced.

…ndler subtypes

…n a URL

gradinac · 2022-08-30T00:58:46Z

@chrisvest Thank you for the heads up! :)
I've rebased the PR to the latest master and I've fixed the Windows metadata generation issue in c3134de.

I've also added the logic for automatic metadata generation to remaining Netty modules that needed it

normanmaurer · 2022-09-02T08:15:57Z

@gradinac I am correct to assume we would also need this for 4.1 ?

normanmaurer · 2022-09-02T08:20:02Z

I think its fine for now

normanmaurer · 2022-09-02T08:20:26Z

+
+            System.err.println("Expected metadata file contents:\n");
+            System.err.println(getMetadataJsonString(handlerMetadata));
+            Assertions.fail();


Why not add what you pass to System.err.println() to the fail method ?

That's a good point, I'll move all System.err.println messages to the fail method instead

normanmaurer · 2022-09-02T08:20:34Z

+                    "This metadata was not found under " + existingMetadataPath);
+            System.err.println("Please create this file with the following content: ");
+            System.err.println(getMetadataJsonString(handlerMetadata));
+            Assertions.fail();


Why not add what you pass to System.err.println() to the fail method ?

normanmaurer · 2022-09-02T08:21:49Z

+        return gson.toJson(metadataList, HANDLER_METADATA_LIST_TYPE);
+    }
+
+    private static class Condition {


Suggested change

private static class Condition {

private static final class Condition {

normanmaurer · 2022-09-02T08:22:00Z

+            this.typeReachable = typeReachable;
+        }
+
+        String typeReachable;


Suggested change

String typeReachable;

final String typeReachable;

normanmaurer · 2022-09-02T08:22:21Z

+    }
+
+    private static class HandlerMetadata {
+        String name;


Suggested change

String name;

final String name;

normanmaurer · 2022-09-02T08:22:30Z

+    private static class HandlerMetadata {
+        String name;
+
+        Condition condition;


Suggested change

Condition condition;

final Condition condition;

normanmaurer · 2022-09-02T08:22:40Z

+
+        Condition condition;
+
+        boolean queryAllPublicMethods;


Suggested change

boolean queryAllPublicMethods;

final boolean queryAllPublicMethods;

gradinac · 2022-09-05T12:04:28Z

@gradinac I am correct to assume we would also need this for 4.1 ?

This is correct - I didn't want to create a Netty 4.1 PR yet though as I wasn't sure if I had taken the right approach for generating this metadata

gradinac · 2022-09-06T11:35:46Z

I've pushed the changes that should address the review - in the meantime, I've also prepared a Netty 4 PR and gathered the required metadata. Once that metadata is tested, I'll create a Netty 4 PR as well

normanmaurer · 2022-09-08T06:49:00Z

Thanks a lot! Great work

gradinac · 2022-09-08T11:01:21Z

Thank you very much for looking into this! :) I'll have a PR for Netty 4.1 ready soon with the same approach

Motivation: Enhance the native image reflect-config.json with conditions This is based on netty/netty#12738 Modification: - Enhance the native image reflect-config.json with conditions - Add test for collecting and comparing the configuration Result: Enhance the native image reflect-config.json with conditions

@violetagg

…#12786) Motivation: #12738 introduced automatic metadata generation for handlers. However, @violetagg rightly noticed that this metadata was not under META-INF. This means it wouldn't automatically be picked up by the native-image builder. This slipped through testing as I was manually merging the metadata before sending it over for tests with an existing Netty version. Modification: Move all generated `reflect-config.json` files from `src/main/resources/native-image/...` to `src/main/resources/META-INF/native-image/` Result: Native-image will properly pick up the generated handler metadata

…andler implementations. (#12794) Motivation: Analogous to #12738, this PR introduces automatic conditional native-image metadata generation for different ChannelHandler implementations Modification: This PR is basically the same as #12738, but adapted to conform to the Java language level 6. This PR also adds the metadata generation to more subprojects that contain ChannelHandler implementations. Result: Fixes #12725

normanmaurer reviewed Aug 25, 2022

View reviewed changes

gradinac force-pushed the gradinac/fix-netty-5-handler-metadata branch from fe4a00c to 6b3af75 Compare August 27, 2022 00:20

gradinac commented Aug 27, 2022

View reviewed changes

Aleksandar Gradinac added 3 commits August 30, 2022 02:18

Automatically generate native-image reflection metadata for ChannelHa…

7c30c4b

…ndler subtypes

Use forward slashes on all platforms when searching for a directory i…

c3134de

…n a URL

Add handler metadata generation for the rest of the netty modules

847b681

gradinac force-pushed the gradinac/fix-netty-5-handler-metadata branch from 6b3af75 to 847b681 Compare August 30, 2022 00:54

normanmaurer requested changes Sep 2, 2022

View reviewed changes

Cleanups

07ddd74

gradinac force-pushed the gradinac/fix-netty-5-handler-metadata branch from de9921e to 07ddd74 Compare September 5, 2022 15:15

gradinac mentioned this pull request Sep 7, 2022

Refine Netty 4.1.80 metadata oracle/graalvm-reachability-metadata#47

Closed

chrisvest approved these changes Sep 7, 2022

View reviewed changes

normanmaurer merged commit 338df93 into netty:main Sep 8, 2022

normanmaurer added this to the 5.0.0.Alpha5 milestone Sep 8, 2022

violetagg mentioned this pull request Sep 8, 2022

Enhance the native image reflect-config.json with conditions netty-contrib/socks-proxy#24

Merged

violetagg mentioned this pull request Sep 8, 2022

Enhance the native image reflect-config.json with conditions netty-contrib/codec-haproxy#14

Merged

gradinac mentioned this pull request Sep 9, 2022

Move native-image metadata from src/main/resources/native-image/ to src/main/resources/META-INF/native-image/ #12786

Merged

gradinac mentioned this pull request Sep 11, 2022

Automatically generate native-image conditional metadata for ChannelHandler implementations. #12794

Merged

FoghostCn mentioned this pull request Jun 15, 2023

add tri native image support apache/dubbo#12536

Merged

8 tasks

	private static class Condition {
	private static final class Condition {

	boolean queryAllPublicMethods;
	final boolean queryAllPublicMethods;

Uh oh!

Conversation

gradinac commented Aug 25, 2022

Uh oh!

gradinac commented Aug 25, 2022

Uh oh!

normanmaurer commented Aug 25, 2022

Uh oh!

gradinac commented Aug 25, 2022

Uh oh!

chrisvest commented Aug 25, 2022

Uh oh!

pderop commented Aug 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gradinac commented Aug 25, 2022

Uh oh!

violetagg commented Aug 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gradinac commented Aug 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gradinac commented Aug 28, 2022

Uh oh!

chrisvest commented Aug 29, 2022

Uh oh!

gradinac commented Aug 30, 2022

Uh oh!

normanmaurer commented Sep 2, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gradinac commented Sep 5, 2022

Uh oh!

gradinac commented Sep 6, 2022

Uh oh!

normanmaurer commented Sep 8, 2022

Uh oh!

gradinac commented Sep 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pderop commented Aug 25, 2022 •

edited

Loading