Skip to content

Conversation

geoand
Copy link
Contributor

@geoand geoand commented May 19, 2025

This comment has been minimized.

@geoand
Copy link
Contributor Author

geoand commented May 19, 2025

It seems like this change has caused more types to be reachable and tripping GraalVM, so it needs more work.

@geoand geoand marked this pull request as draft May 20, 2025 06:56
@geoand
Copy link
Contributor Author

geoand commented May 20, 2025

I can continue adding the necessary GraalVM configurations, but I am pretty sure I am missing how things currently work in main WRT native-image for this validator extension, so I'll wait for input from @gsmet on that

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Yeah, so the problem is that with the new code, the ValidatorFactory is initialized at runtime and not at static init.

And things work atm because the VF is initialized at static init.

@geoand
Copy link
Contributor Author

geoand commented May 20, 2025

That definitely makes sense, but the way the Synthetic Bean is created, it should be static-init and not runtime init.

@gsmet
Copy link
Member

gsmet commented May 20, 2025

I added some debug statements and the Function is called when the test is executed, not during the native image build.

@geoand
Copy link
Contributor Author

geoand commented May 20, 2025

@mkouba what is the expectation here for synthetic beans that are staticInit=true? When should the function itself run?

@mkouba
Copy link
Contributor

mkouba commented May 20, 2025

@mkouba what is the expectation here for synthetic beans that are staticInit=true? When should the function itself run?

The recorder is called during ExecutionTime#STATIC_INIT, i.e. from a generated static initializer...

@geoand
Copy link
Contributor Author

geoand commented May 20, 2025

The recorder yes, but how about the function itself (the one returned by the recorder)?

@mkouba
Copy link
Contributor

mkouba commented May 20, 2025

The recorder yes, but how about the function itself?

When the relevant bean is created. When exactly? That depends on the scope and the "clients" (e.g. other CDI beans that invoke methods of the bean).

@geoand
Copy link
Contributor Author

geoand commented May 20, 2025

I see, that makes sense. Assuming the bean is @Singleton, is there anyway to guarantee a very eager (i.e. static init) initialization of the bean without writing some kind of dummy code?

@gsmet
Copy link
Member

gsmet commented May 20, 2025

Like an equivalent of startup() but for static init.

@mkouba
Copy link
Contributor

mkouba commented May 20, 2025

Like an equivalent of startup() but for static init.

Yes, there's BeanConfiguratorBase#startup(int), SyntheticBeanBuildItem.ExtendedBeanConfigurator extends the BeanConfiguratorBase.

@gsmet
Copy link
Member

gsmet commented May 20, 2025

Yes but my understanding of startup() is that it will be run at runtime init?

At least my attempt to use it didn't change at thing.

@gsmet
Copy link
Member

gsmet commented May 20, 2025

@geoand FYI, I pushed an additional commit with some further cleanup that should be squashed once we figure out how to do things properly.

I dropped the additional GraalVM stuff as it's not the way to go.

@geoand
Copy link
Contributor Author

geoand commented May 20, 2025

👍🏽

I'm also trying something to fix native.

@geoand
Copy link
Contributor Author

geoand commented May 20, 2025

Okay, fixed it

@mkouba
Copy link
Contributor

mkouba commented May 20, 2025

Yes but my understanding of startup() is that it will be run at runtime init?

Ah, yes you're right. I misread your comment. Normal beans can observe the @Initialized(ApplicationScoped.class) lifecycle event but I don't think we have something for synthetic beans. So a workaround would be (a) Arc.container().instance(Foo.class).get() in a recorder method or (b) create an additional class-based bean with an @Initialized(ApplicationScoped.class) observer that consumes Foo...

@geoand
Copy link
Contributor Author

geoand commented May 20, 2025

I pushed a commit that should also be squashed if we agree it's the way to go.

// this is done in order to ensure that HibernateValidatorFactory is fully initialized at static init
// so completely in heap and ready to go when a native image is built
public void hibernateValidatorFactoryInit(BeanContainer beanContainer) {
HibernateValidatorFactory hibernateValidatorFactory = beanContainer.beanInstance(HibernateValidatorFactory.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this only works because HibernateValidatorFactory is @Singleton and not a normal scoped bean with a client proxy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, I'm aware. If it were ApplicationScoped I would have called toString

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

If you end up with a green CI, I think it's good to merge.

@geoand
Copy link
Contributor Author

geoand commented May 20, 2025

CI was green on my fork so I squashed, rebased and my the PR ready for review

@geoand geoand marked this pull request as ready for review May 20, 2025 18:29
Copy link

quarkus-bot bot commented May 20, 2025

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 86d200b.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@geoand geoand merged commit 3e2f9e6 into quarkusio:main May 20, 2025
48 of 49 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.24 - main milestone May 20, 2025
@geoand geoand deleted the #47945 branch May 21, 2025 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NPE at AbstractMethodValidationInterceptor.validateMethodInvocation

3 participants