jakartaee / expression-language

Jakarta Expression Language
https://eclipse.org/ee4j/el
Other
60 stars 49 forks source link

BeanELResolver needs cleanup method #171

Closed joakime closed 2 years ago

joakime commented 2 years ago

If the BeanELResolver is loaded from the Server/System classpath, then the BeanELResolver.properties field holds onto references that lock the WebApp classloaders from being able to be shutdown / reloaded properly.

Containers like Jetty have to work around this with classes like ELContextCleaner.

https://github.com/eclipse/jetty.project/blob/jetty-11.0.6/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/listener/ELContextCleaner.java

Can we get a proper method in BeanELResolver to allow us to cleanup what is being held in the BeanELResolver on demand?

A quick-n-dirty example method.

public void cleanup(Predicate<Map.Entry<Class<?>,BeanProperties>> removePredicate) {
    properties.entrySet().removeIf(removePredicate);
}
markt-asf commented 2 years ago

It looks like there has been an attempt to fix this in BeanELResolver with soft references. Is this solution not working? (I'll do some testing but I'm asking the question as it may get the answer faster). My preference would be to fix the cache in BeanELresolver so it doesn't leak rather than add a new method.

Ah. I see that the cache is static. We could just remove the static. Tomcat's EL API has been that way since it was first written and I'm not aware of any issues being reported.

joakime commented 2 years ago

The soft references rely on a GC run, which isn't a reliable method to cleanup the resources that are being held to prevent cleanup of the resources.

WebApp(/foo) -> BeanELResolver -> WebApp(/foo)

It's a cycle, unfortunately.

Looks like this change in BeanELResolver (around version 2.1??) was due to https://github.com/eclipse-ee4j/glassfish/issues/1649

markt-asf commented 2 years ago

So switch the cache to non-static then?

markt-asf commented 2 years ago

And/or switch to weak references?

joakime commented 2 years ago

I'm leaning towards removing static.

olamy commented 2 years ago

voting for non static

janbartel commented 2 years ago

Non static