Do not use enum to store CDI BeanManager #256

Closed
mkouba opened this Issue May 24, 2016 · 10 comments

Projects

None yet

3 participants

@mkouba
mkouba commented May 24, 2016

https://github.com/omnifaces/omnifaces/blob/master/src/main/java/org/omnifaces/config/BeanManager.java#L66

I don't think it's a good idea to store a hard reference to a CDI BeanManager in enum value. If there is a hard reference to a class which prevents the relevant ClassLoader from being garbage collected then the whole container will remain in memory (AFAIK enum values are treated as static class members), possibly causing OutOfMemoryError with frequent redeploys (see for example https://issues.jboss.org/browse/WFLY-6173). Using WeakReference might help. In CDI 1.1+ you could probably also use CDI.current().getBeanManager() (but it's not ideal either).

Also note that the CDI spec does not say anything like "there is a single BeanManager per application". On the contrary, some BeanManager methods require some context - e.g. getBeans() "Returns the set of beans which have the given required type and qualifiers and are available for injection in the module or library containing the class into which the BeanManager was injected...". In Weld, there is a BeanManager per bean archive.

@arjantijms
Member

Thanks for the comment Martin, this is surely a point that needs attention.

Also note that the CDI spec does not say anything like "there is a single BeanManager per application".

I wonder how much this could be a problem in practice. As libraries like OmniFaces can only be used in the web module, they can only really obtain the BeanManager from the web module, can't they? What other modules would come into play here? E.g. it's not the intention that OmniFaces is somehow referenced from an EJB module and/or put into EAR/lib.

What do you think btw of how we handle the bean manager in Mojarra?

It's stored as an instance variable of the Application class, see https://github.com/jsf-spec/mojarra/blob/master/jsf-ri/src/main/java/com/sun/faces/application/ApplicationImpl.java#L2696

But it's initially fetched from JDNI (with a Weld specific fallback) and then stored in the application context. See: https://github.com/jsf-spec/mojarra/blob/master/jsf-ri/src/main/java/com/sun/faces/util/Util.java#L1205

@mkouba
mkouba commented May 24, 2016

Hi Arjan,

I wonder how much this could be a problem in practice. As libraries like OmniFaces can only be used in the web module...

Alternatives, interceptors and decorators enabled/selected for a bean archive (not by means of @Priority) might be problematic. For example, Weld treats jars in WEB-INF/lib as separate bean archives. So if you place a jar with beans.xml and alternative class selected for this bean archive only, it will not be available in the bean archive representing WEB-INF/classes.

I don't know much about Mojarra/JSF internals but it looks ok.

@arjantijms
Member

Alternatives, interceptors and decorators enabled/selected for a bean archive (not by means of @Priority) might be problematic. For example, Weld treats jars in WEB-INF/lib as separate bean archives.

So how will this work with a bean manager obtained from JNDI ("java:comp/BeanManager")? That one does not have a concept of being a bean manager for a specific bean archive, does it?

So if you place a jar with beans.xml and alternative class selected for this bean archive only, it will not be available in the bean archive representing WEB-INF/classes.

Isn't this a problem regardless of the problem we're discussing here?

@mkouba
mkouba commented May 24, 2016

So how will this work with a bean manager obtained from JNDI... That one does not have a concept...

Well, I think it has. The BeanManager obtained from JNDI should be the one of Java EE module the lookup is performed from (i.e. WEB-INF/classes in this case). Note this is not entirely clear and implementations (and even EE container integrations) behave differently - there are some CDI spec clarification issues.

Isn't this a problem regardless of the problem we're discussing here?

I don't know anything about OmniFaces internals and what is it using the BeanManager for.

@BalusC
Member
BalusC commented May 24, 2016 edited

I don't know anything about OmniFaces internals and what is it using the BeanManager for.

It's mainly used as work around to obtain the BeanManager where @Inject is not supported (some JSF specific artifacts), or has bugs (Tomcat and/or OpenWebBeans mainly), or where CDI.current() fails (e.g. during close of web socket in WildFly).

@arjantijms
Member

Well, I think it has. The BeanManager obtained from JNDI should be the one of Java EE module the lookup is performed from (i.e. WEB-INF/classes in this case).

But then, theoretically, this bean manager would not be able to obtain beans from any archives in WEB-INF/lib?

I don't know anything about OmniFaces internals and what is it using the BeanManager for.

I was more referring to selecting alternatives for single bean archives in general (without OmniFaces), which would then necessitate updating all other possible bean archives?

To me this sounds like something you almost never would want in practice. If I select an alternative in say the beans.xml that's in WEB-INF/ I would always want that to apply globally (for all bean archives that contain classes that the web module class loader can see). But maybe I'm not understanding the issue completely here.

@mkouba
mkouba commented May 24, 2016

@arjantijms

But then, theoretically, this bean manager would not be able to obtain beans from any archives in WEB-INF/lib...

It will be - classes from a web module see each other and CDI follows the underlying module architecture.

To me this sounds like something you almost never would want in practice.

This is how selection for a bean archive works. If you want to select it globally, use the @Priority.

@BalusC
Member
BalusC commented Jun 8, 2016 edited

Given that OmniFaces is designed as a WAR library, not as an EAR library (deploy/runtime would fail when library is misplaced so), we can assure that any classloader related leaks during (re)deployment are to be contributed to the server itself, not to OmniFaces.

I only do agree that keeping BeanManager in an enum is kind of fishy. Perhaps we should clarify in the documentation that this shouldn't be interpreted as "best practice anywhere", i.e. it must not be copied to different environments with different classloading heuristics. Alternatively, we could choose to store the BeanManager instance as servlet context attribute instead (as Weld itself also does), but this wouldn't work further down in the code where neither FacesContext nor ServletContext is available (such as in a web socket). As this is a major API change, I'd rather not do it before OmniFaces 3.0 -- if we would do.

We can also take a step back: make sure that CDI.current() is guaranteed available everywhere in every environment and returns exactly the expected BeanManager. Also during e.g. close of a web socket. Then we can use it in places where @Inject isn't supported and drop our enum altogether.

@mkouba
mkouba commented Jun 8, 2016

I agree that OmniFaces are not causing the leak. On the other hand, I think that storing BeanManager in an enum value is a bad idea. Enum values are treated as static class members and so they're only garbage collected when the ClassLoader of the owner class is. And it seems it's not entirely clear when this happens (e.g. in Java 8 where Metaspace is not part of the heap).

CDI.current() is available in every environment (althoug CDI officialy only supports Java EE container). However, I'm not sure what the expected BeanManager is. It usually depends on the calling class name. So it really depends on your expectations ;-). As I pointed out above - using WeakReference in the enum might be sufficient.

@BalusC
Member
BalusC commented Jul 6, 2016

I retested CDI#current() over all place in OmniFaces in various modern servers and the only case where it still fails is during the close of a WildFly/Undertow websocket. However the current workaround in SocketSessionManager which obtains it as a static volatile reference during first open of a websocket still suffices.

@BalusC BalusC closed this Jul 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment