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

HHH-18982 make BeanContainer loadable as a Java service #9656

Merged
merged 2 commits into from
Jan 26, 2025

Conversation

gavinking
Copy link
Member

@gavinking gavinking commented Jan 21, 2025

also remove unnecessary use of reflection for creation of the CDI BeanContainer

[Please describe here what your change is about]


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.


https://hibernate.atlassian.net/browse/HHH-18982

@gavinking
Copy link
Member Author

@sebersole in my testing, the use of reflection in CdiBeanContainerBuilder appears to not be needed.

@gavinking gavinking changed the title HHH-18982 make BeanManager instead loadable as a Java service HHH-18982 make BeanCountainer loadable as a Java service Jan 21, 2025
@gavinking gavinking changed the title HHH-18982 make BeanCountainer loadable as a Java service HHH-18982 make BeanContainer loadable as a Java service Jan 21, 2025
@sebersole
Copy link
Member

sebersole commented Jan 22, 2025

@sebersole in my testing, the use of reflection in CdiBeanContainerBuilder appears to not be needed.

The pertinent javadoc is :

 * We need to avoid statically linking CDI classed into the ClassLoader which
 * would lead to errors if CDI is not available on the classpath.

If CDI is not in the classpath and it stil works, then I'm confused but great

@gavinking
Copy link
Member Author

Well I mean if the classes that import CDI types are never loaded, then CDI types aren't loaded either.

@gavinking
Copy link
Member Author

The VM won't load classes it doesn't need.

@sebersole
Copy link
Member

Depends if CDI is always on the classpath of the user code when run in a container. Wild Fly, e.g., always passes Hibernate a BeanManager reference iiuc which would trigger this code.

@gavinking
Copy link
Member Author

gavinking commented Jan 23, 2025

But if WildFly passes a BeanManager then we're going to instantiate a CdiBeanContainerXxxxAccess and those classes are going to be loaded anyway, no?

@sebersole
Copy link
Member

sebersole commented Jan 23, 2025

Sure. But the reflection was intended to give a useful waring -

[subsystem]  WARN org.hibernate.orm.beans ManagedBeanRegistryInitiator:67 - HHH10005001: An explicit CDI BeanManager reference [java.lang.Object@16132f21] was passed to Hibernate, but CDI is not available on the Hibernate ClassLoader. This is likely going to lead to exceptions later on in bootstrap.

Ultimately the exception will be the same ofc -

Unable to create requested service [org.hibernate.resource.beans.spi.ManagedBeanRegistry] due to: Unable to load class [jakarta.enterprise.inject.spi.BeanManager]
org.hibernate.service.spi.ServiceException: Unable to create requested service [org.hibernate.resource.beans.spi.ManagedBeanRegistry] due to: Unable to load class [jakarta.enterprise.inject.spi.BeanManager]
...

Now, we could look at simply catching the class-loading exception somewhere and logging the nice warning.

@gavinking
Copy link
Member Author

Now, we could look at simply catching the class-loading exception somewhere and logging the nice warning

OK, so what I did instead was do the check upfront and throw a sensible exception.

If this change breaks anything we will know soon enough, and the cause of breakage will be clear.

@gavinking gavinking merged commit c5eba23 into hibernate:main Jan 26, 2025
26 checks passed
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.

2 participants