expression-language
expression-language copied to clipboard
jakartaee
Add BeanPropertiesCache + FactoryFinderCache
https://github.com/jboss/jboss-jakarta-el-api_spec/blob/4.0.x/api/src/main/java/org/jboss/el/cache/BeanPropertiesCache.java + https://github.com/jboss/jboss-jakarta-el-api_spec/blob/4.0.x/api/src/main/java/org/jboss/el/cache/FactoryFinderCache.java have worked well for WildFly/JBoss application servers. We haven't ported these to Expression Language 6.0 yet but could if there is interest in adding those.
Question, should we target adding these changes in 6.0? If no, how about the next release?
There are (SPEC-API) integration changes that we would include in the form of a pull request to add the ^ caching classes if there is interest in improving the ^ caching classes.
Note that there are public methods to clear the cache for a specified classloader.
Note that what this does addresses the concern raised in #214. While it's true that if something is gc'd as a result of memory pressure it's not a memory leak (as stated by @markt-asf in #215), in WildFly we prefer not to have deployment classloaders survive undeployment and thus we prefer to clear the cache. That's one of the reasons why we've used our own variant of the EL API for many releases.
The current BeanELResolver cache is at the instance level, not the class level. That was changed in #171. The entire cache should be GC eligible once the web application is undeployed as nothing should be retaining a reference to the BeanELResolver instance.
We can't use the proposed BeanPropertiesCache as is since it would re-introduce the dependency on java.beans.* that we just removed.
BeanPropertiesCache also uses a static cache (hence why it needs an explicit clear() method) which goes against the work undertaken in #171.
I don't see any other differences so I think that is a "no thank you" to using BeanPropertiesCache in the EL API.
I'll note that EL 6.0 was released at the start of this month so any changes being discussed would be for 6.1.
Tomcat has a similar approach to caching the factory implementation. The key difference is that is uses WeakReferences rather than requiring a clear() method. I think I prefer the WeakReference approach as that doesn't create a potential memory leak that users need to take care of by calling clear().
I'd have no objection to a PR that introduced a WeakReference based cache to FactoryFinder or any other approach on broadly the same lines.
~I believe that WeakReference instead of SoftReference should resolve the issue without a specific clean() method, however without testing, cannot be sure.~
WeakReference will not work as BeanProperties will be cleared out on every GC
The entire cache should be GC eligible once the web application is undeployed as nothing should be retaining a reference to the BeanELResolver instance
Unfortunately that isn't true. com.sun.faces.el.ELUtils holds a reference to BeanELResolver, and since it's loaded outside Application's ClassLoader, it will not be GC eligible.
See https://github.com/eclipse-ee4j/mojarra/blob/410b7eab83ae3709668cd05642b8ccc01b17e598/impl/src/main/java/com/sun/faces/el/ELUtils.java#L109
That behaviour of com.sun.faces.el.ELUtils is a memory leak waiting to happen. Whether that is an EL API issue or a mojarra implementation issue is debatable. We can probably protect the BeanELResolver cache against memory leaks caused by that sort of usage but it is going to require a new cache implementation. None of the ones I am aware of would work for that usage without leaking.
I think FactoryFinderCache can be merged "as-is"
and #215 should take care of BeanPropertiesCache since linked implementation depends on java.beans and is non-starter.
Let me know if I am missing something
It seems the main issue is in Mojarra holding a reference to the BeanELResolver in a static variable. I believe that should be definitely addressed. In application servers that support app redeployment, no component should use static variables as it can easily introduce leaked classloaders after an app was undeployed. I saw this behavior in Mojarra and I was really suprised about it.
To address beanProperties and factoryFinder caches, isn't it better to add an SPI to plug in custom caching mechanisms? It would additional dependencies in the API, the API would contain only the necessary functionality and would leave caching to implementations. It would also allow implementers clean the cache if needed, without adding new methods in the API or maintaining forks.
better to add an SPI to plug in custom caching mechanisms?
That's a fantastic idea. Way to think in Jakarta EE-stic ways