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

Mojarra fails to initialize when BDA is empty according to Weld #5238

Merged
merged 6 commits into from
Sep 7, 2023

Conversation

BalusC
Copy link
Contributor

@BalusC BalusC commented May 20, 2023

@arjantijms
Copy link
Contributor

force Weld to reinitialize when it skipped initialization

@manovotn see here for a practical need to have a standard way to "init" CDI in container mode.

@manovotn
Copy link
Contributor

force Weld to reinitialize when it skipped initialization

@manovotn see here for a practical need to have a standard way to "init" CDI in container mode.

Looking at the issue, the user app has no need for CDI container as such, so why start it?
How exactly does Mojarra rely on CDI here?

@arjantijms
Copy link
Contributor

How exactly does Mojarra rely on CDI here?

Many of Mojarra's internals are based on CDI. So when we activate Faces (which happens conditionally), we always also need CDI.

weld.onStartup(null, servletContext);
result = (BeanManager) applicationMap.get("org.jboss.weld.environment.servlet.jakarta.enterprise.inject.spi.BeanManager");
} catch (Exception | LinkageError e) {
LOGGER.log(WARNING, "Reinitializing Weld failed - giving up, please make sure your project has at least one @Named bean and retry", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think @Named is enough here.
If I understand correctly what you're running into, there are two factors in play. Firstly it's the discovery mode the app uses (all or annotated, the latter being default from CDI 4.0). Secondly, if it's annotated, then you need to have some bean with bean defining annotation[1][2]; @Named isn't bean defining.
I assume you're hitting this only with annotated discovery (since with all you're likely to find something that could be a bean), so you could log something like:

"Reinitializing Weld failed - giving up, please make sure your project contains at least one bean class with a bean defining annotation and retry"

[1] https://jakarta.ee/specifications/cdi/4.0/jakarta-cdi-spec-4.0.html#bean_defining_annotations
[2] https://jakarta.ee/specifications/cdi/4.0/jakarta-cdi-spec-4.0.html#bean_defining_annotations_full

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Point taken, fixed.

ServletContext servletContext = (ServletContext) facesContext.getExternalContext().getContext();
servletContext.setInitParameter("org.jboss.weld.environment.servlet.archive.isolation", "false");
ServletContainerInitializer weld = (ServletContainerInitializer) Class.forName("org.jboss.weld.environment.servlet.EnhancedListener").getConstructor().newInstance();
weld.onStartup(null, servletContext);
Copy link
Contributor

Choose a reason for hiding this comment

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

@BalusC is this basically just for detection purposes? Assuming Weld bootstrap fails the first time around, re-triggering it for the exact same deployment will have the same result.
If it's detection only, perhaps we could instead use ServletContext#setInitParameter() on Weld side which you can then detect here?

Copy link
Contributor Author

@BalusC BalusC Jul 9, 2023

Choose a reason for hiding this comment

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

is this basically just for detection purposes? [...] re-triggering it for the exact same deployment will have the same result.

No, it will actually fix the issue. See also attached ticket.

@manovotn
Copy link
Contributor

But really, Weld should probably be readjusted to not skip initialization when it finds a FacesServlet being explicitly registered in web.xml. @arjantijms perhaps you can ping Weld guys for this?

(taken from the linked issue)
Is there some way that we can easily detect this? We aren't parsing web.xml ourselves.
Also, other JSF impls (there was MyFaces too IIRC) might not need this, right? So we should rather be looking into how to achieve this for Mojarra?

How exactly does Mojarra rely on CDI here?

Many of Mojarra's internals are based on CDI. So when we activate Faces (which happens conditionally), we always also need CDI.

Hmm, Weld's bootstrap is determined by the need of user app as that's the deployment (WAR) we get to scan. I don't think we want to change the default behavior though.
Next thing coming to my mind is adding a Weld-specific config that you could set to true to tell Weld to boot anyway. However, I am not sure what kind of config would be universally usable for Mojarra as you can get to this situation from an EE server as well as from just a servlet. Or is it just the servlet? If it's the servlet, would it be possible for Mojarra to set something like a ServletContext init parameter that Weld could read or is Weld bootstrapping too early for that?

Also, I can see Mojarra itself has some CDI parts, namely at least a CDI extension and several other modules declaring beans.xml - how are those recognized and picked up?

@arjantijms
Copy link
Contributor

Also, other JSF impls (there was MyFaces too IIRC) might not need this, right? So we should rather be looking into how to achieve this for Mojarra?

As internals of Faces are required to be CDI based, every Faces implementation would need this. There is basically only one other Faces implementation, and that is indeed MyFaces.

I don't think though scanning web.xml is beneficial. The detection algorithm for Faces content is a little bit more complex than that.

See https://github.com/eclipse-ee4j/mojarra/blob/master/impl/src/main/java/com/sun/faces/config/FacesInitializer.java#L104

It would be unreasonable for CDI to execute this algorithm, as its spec defined and would tie the CDI spec to the Faces spec. As CDI is the lower level in the EE architecture, this is unwanted.

Extra complexity is introduced by EE not defining how and in which order CDI and Faces are initialised. Some containers (probably most) use a ServletContainerInitializer for this, but even then their order is not guaranteed.

What could maybe work is an extra static method for jakarta.enterprise.inject.spi.CDI<T>, maybe called init() or forceInit() or something like that. If the CDI initialisation code has not run yet, it would set some kind of flag that the initialisation code later picks up (and in case of Weld proceeds in the same way as-if a non-empty BDA was found). If the CDI initialisation has already run, the method would cause the immediate re-execution of it.

Thoughts?

@manovotn
Copy link
Contributor

It would be unreasonable for CDI to execute this algorithm, as its spec defined and would tie the CDI spec to the Faces spec. As CDI is the lower level in the EE architecture, this is unwanted.

Right, that's what I thought.

What could maybe work is an extra static method for jakarta.enterprise.inject.spi.CDI, maybe called init() or forceInit() or something like that. If the CDI initialisation code has not run yet, it would set some kind of flag that the initialisation code later picks up (and in case of Weld proceeds in the same way as-if a non-empty BDA was found).

CDI is a universal entry point - this can be accessed from SE, servlet or EE.
And while it might work for this servlet case, I don't think it makes sense in SE where, even now, you can start the container in whichever way you like, including adding synth archives etc.
And EE is such a massive tangle of complexity due to various EE specs coming together and trying to boot up with CDI in one way or another - I cannot really imagine "just restarting it" since other places already depend on CDI container being in certain state. Last but not least, CDI doesn't prohibit you from having several containers runnings (I certainly saw that with SE) - at which point using CDI.init() does... what exactly? :)

If the CDI initialisation has already run, the method would cause the immediate re-execution of it.

What's the point of re-executing it? If the CDI container detected a state that prevented it from booting, it will simply do so again. The method won't allow you to change the deployment setup (nor can it due to variety of envs it can be invoked from).

As internals of Faces are required to be CDI based, ...

Can you please point me to relevant spec parts? I don't mean to sound rude, I just want to check.
If it states that you need to provide certain components as CDI beans, then I am not sure it makes a lot of sense to provide them to a deployment that's not using CDI anyway?

Extra complexity is introduced by EE not defining how and in which order CDI and Faces are initialised. Some containers (probably most) use a ServletContainerInitializer for this, but even then their order is not guaranteed.

So I take it you couldn't provide some servlet context parameter for this? This seems more and more like an issue in servlet, so any servlet specific solution would also do the trick I suppose. I am not well versed there tho, so I am open to suggestions :)
You could also use any Weld-specific config like I said. TBF I am not sure this behavior is mandated by spec, it might well be Weld choice not to boot if there is no BDA.

@arjantijms
Copy link
Contributor

Last but not least, CDI doesn't prohibit you from having several containers runnings (I certainly saw that with SE) - at which point using CDI.init() does... what exactly? :

It should do pretty much what the container did to init CDI when loading the application that uses Faces. The concept of an "application" (and with it the application scope etc) should be well enough defined by both Faces and Servlet.

What's the point of re-executing it? If the CDI container detected a state that prevented it from booting, it will simply do so again.

I tested this on a patched Weld, and it won't.

Since at that point the above mentioned flag would have been set to true, and it would continue booting. At least for Weld (which I patched and tested a while ago) this worked on GlassFish, Piranha Cloud and Tomcat. For Piranha Cloud and Tomcat (which mainly use the Weld Provided WeldInitListener from org.jboss.weld.servlet:weld-servlet-core) I did "cheat" somewhat by adding a dummy class to the BDA at that point.

Can you please point me to relevant spec parts? I don't mean to sound rude, I just want to check.

Several features depend on CDI, for instance:

But more importantly, implicit object now must be resolved via CDI:

This is what may trigger the error in many cases. People use a Facelet, which uses some expression language, and for some reason or the other, they don't have a CDI bean in their application. Faces will now internally try to resolve via CDI, but that fails if Weld thought it didn't need to continue booting.

If it states that you need to provide certain components as CDI beans, then I am not sure it makes a lot of sense to provide them to a deployment that's not using CDI anyway?

Faces uses CDI internally. And the plans are to increase that usage. I.e. Faces uses many factories to obtain a lot of internal instances of things (like the lifecycle instance, the view instance, you name it). We'd like to remove many of these and use CDI instead. The application does not have to explicitly use CDI in order for CDI to be needed by Faces.

@arjantijms
Copy link
Contributor

So I take it you couldn't provide some servlet context parameter for this? This seems more and more like an issue in servlet, so any servlet specific solution would also do the trick I suppose.

Partially if CDI states that within a container that supports Servlet, CDI must be initialised using a ServletContainerInitializer. But that's not the case now.

Then, Faces could use two ServletContainerInitializers to sandwich itself around the CDI one. The first one would run the Faces detection algorithm that sets a flag which CDI then must pick up and init itself even when no beans are found in the application BDA.

That requires multiple changes though to both the Servlet spec (e.g. logical names for ServletContainerInitializers and before and after ordering) and the change for the CDI spec to honour said flag / parameter and the ServletContainerInitializer requirement.

@arjantijms
Copy link
Contributor

ps.

In general this also highlight a major issue among specs in EE.

Several things that Faces once did itself have been extracted to separate specs, among which Expression Language and CDI itself (partially, in a way). But then those extracted specs don't understand the requirements of the specs they came from and/or which depend on them.

A lot of time is then spend convincing people that run those other specs of those things, which are relatively obvious to the depending spec. The alternative is that Faces introduces its own bean model again, so we can simply arrange those things ourselves, but that's a major step back. I'm not sure how to solve this. We can't expect the CDI team to know about the intricate details in Faces, Security, REST, etc, but the current situation is not optimal either.

@manovotn
Copy link
Contributor

manovotn commented Jul 3, 2023

Faces uses CDI internally. And the plans are to increase that usage. I.e. Faces uses many factories to obtain a lot of internal instances of things (like the lifecycle instance, the view instance, you name it). We'd like to remove many of these and use CDI instead. The application does not have to explicitly use CDI in order for CDI to be needed by Faces.

I see, in this regard you are basically a "consumer" of CDI functionality just as much as user app is (or can be).

Partially if CDI states that within a container that supports Servlet, CDI must be initialised using a ServletContainerInitializer. But that's not the case now.

Right, good point.

I'm not sure how to solve this. We can't expect the CDI team to know about the intricate details in Faces, Security, REST, etc, but the current situation is not optimal either.

I am not sure either, any solution would be impl specific because this is an issue specific to servlets (since EE server boots whole CDI, I suppose they can simply choose to do that based on presence of Faces) and CDI spec doesn't cover pure servlet approach anyway.

@arjantijms arjantijms added the 4.0 label Jul 6, 2023
@arjantijms arjantijms added this to the 4.0.3 milestone Jul 6, 2023
BalusC added 3 commits July 9, 2023 10:38
'Fail fast' line should be in ConfigureListener because ordering of
ServletContainerInitializer is undefined
@pizzi80
Copy link
Contributor

pizzi80 commented Aug 16, 2023

To me the best way is to improve Servlet API to detect and init CDI
which is also used in Servlets and in JSP pages with EL ${bean.property} for example

Faces integrate Servlets, Filters and CDI to simplify
the web development life, so it's a high level technology that
should not take care to initialize a core technology like CDI

I think that there is a similar problem with JPA / CDI / JTA ....

At the moment the biggest problem of Jakarta EE is the lack
of integrations between all the modules, (where is the framework?).

To me (as a developer) the "Jakarta EE framework" should
be basically something like Apache Deltaspike...

@BalusC BalusC added this to the 4.0.4 milestone Sep 2, 2023
weld.onStartup(null, servletContext);
result = (BeanManager) applicationMap.get("org.jboss.weld.environment.servlet.jakarta.enterprise.inject.spi.BeanManager");
} catch (Exception | LinkageError e) {
LOGGER.log(WARNING, "Reinitializing Weld failed - giving up, please make sure your project has at least one @Named bean and retry", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

has at least one @nAmed bean and retry

Doens't have to be an @Named bean. Any enabled Bean counts.

@arjantijms
Copy link
Contributor

We probably need something more/better in the future, but let's do it this way for now.

@arjantijms arjantijms merged commit a1f0134 into 4.0 Sep 7, 2023
@arjantijms arjantijms deleted the mojarra_issue_5232 branch September 7, 2023 10:55
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.

4 participants