MyFaces Core
  1. MyFaces Core
  2. MYFACES-2942

Memory Leak in MyFaces 2.0.1 probably as well in 2.0.2

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0.1, 2.0.2
    • Fix Version/s: 2.0.3
    • Component/s: None
    • Labels:
      None
    • Environment:
      JBOSS AS

      Description

      Stan Silvert from JBoss reports:

      I'm pretty sure 2.0.1 has a memory leak on undeploy. Mojarra had an undeploy leak and it took a long time to track it down. The same test I was using on Mojarra also failed on MyFaces but I haven't had time to track down the leak in MyFaces.

      Maybe this is fixed in 2.0.2? If not maybe someone can go ahead and take a look? The mem leak keeps MyFaces from passing TCK on JBoss AS. To test, all you need to do is create a small exploaded JSF app. Then have a script that touches web.xml every 10 seconds. That will cause the app to redeploy. You will get a PermGen error in about an hour.

        Issue Links

          Activity

          Hide
          Bruno Aranda added a comment -

          Using tomcat 7 I get this warning...

          SEVERE: The web application [/editor-2.0-SNAPSHOT] created a ThreadLocal with key of type [java.lang.ThreadLocal] (value [java.lang.ThreadLocal@41649a55]) and a value of type [org.apache.myfaces.config.RuntimeConfig] (value [org.apache.myfaces.config.RuntimeConfig@33d063fd]) but failed to remove it when the web application was stopped. This is very likely to create a memory leak.

          I don't know if the RuntimeConfig could be the one responsible of the leak in Jboss?

          Show
          Bruno Aranda added a comment - Using tomcat 7 I get this warning... SEVERE: The web application [/editor-2.0-SNAPSHOT] created a ThreadLocal with key of type [java.lang.ThreadLocal] (value [java.lang.ThreadLocal@41649a55] ) and a value of type [org.apache.myfaces.config.RuntimeConfig] (value [org.apache.myfaces.config.RuntimeConfig@33d063fd] ) but failed to remove it when the web application was stopped. This is very likely to create a memory leak. I don't know if the RuntimeConfig could be the one responsible of the leak in Jboss?
          Hide
          Martin Kočí added a comment -

          after 10 deploy-undeploy of same app at tomcat 6 (after each redeploy I did one request to a view)

          1 instance of RuntimeConfig (keeps live instances of NavigationCase and NavigationRule and many others)

          362 instances of org.apache.myfaces.view.facelets.tag.MetadataTargetImpl - for those Memory Analyzer (http://www.eclipse.org/mat/) shows that immediate dominator is org.apache.catalina.loader.WebappClassLoader.

          RuntimeConfig is not GCed but next deploy replaces instance -> only one instance remain. MetadataTargetImpl is another story - number of instances grows after every redeploy. Each 39 instances of MetadataTargetImpl are loaded with different WebappClassLoader instance (17 live instances of WebappClassLoader in time of snapshot).

          It looks like a cumulative issue: something (probably RuntimeConfig) prevents WebappClassLoader instance to be GCed. Next deploy create new WebappClassLoader which loads classes again and consumes perm gen space.

          Show
          Martin Kočí added a comment - after 10 deploy-undeploy of same app at tomcat 6 (after each redeploy I did one request to a view) 1 instance of RuntimeConfig (keeps live instances of NavigationCase and NavigationRule and many others) 362 instances of org.apache.myfaces.view.facelets.tag.MetadataTargetImpl - for those Memory Analyzer ( http://www.eclipse.org/mat/ ) shows that immediate dominator is org.apache.catalina.loader.WebappClassLoader. RuntimeConfig is not GCed but next deploy replaces instance -> only one instance remain. MetadataTargetImpl is another story - number of instances grows after every redeploy. Each 39 instances of MetadataTargetImpl are loaded with different WebappClassLoader instance (17 live instances of WebappClassLoader in time of snapshot). It looks like a cumulative issue: something (probably RuntimeConfig) prevents WebappClassLoader instance to be GCed. Next deploy create new WebappClassLoader which loads classes again and consumes perm gen space.
          Hide
          Jakob Korherr added a comment -

          Thanks a lot for this info, Martin!

          I'll try to fix the issue with RuntimeConfig, then we'll see if the MetadataTargetImpl problem still exists..

          Show
          Jakob Korherr added a comment - Thanks a lot for this info, Martin! I'll try to fix the issue with RuntimeConfig, then we'll see if the MetadataTargetImpl problem still exists..
          Hide
          Jakob Korherr added a comment -

          Digging into this, I found out that in MyFaces 2.0 we actually do not need the ThreadLocal for the RuntimeConfig anymore, because we have a FacesContext and a ExternalContext available at startup (and shutdown). Thus setting the RuntimeConfig is not necessary, because we can simply get it via getCurrentInstance().

          By the way: the ThreadLocal in ApplicationImpl was introduced in revision 511514 (MyFaces 1.2), and of course there was no FacesContext available at initialisation time. Now, it is!

          Attached is a patch, which removes the ThreadLocal from ApplicationImpl and uses RuntimeConfig.getCurrentInstance() instead. I tested it and could not find any problems with this approach, since it returns the exact same instance of RuntimeConfig.

          If there are no objections, I will commit this patch soon.

          @Martin, Bruno: could you please test this patch and see if the memory problems are still there? Thanks!

          Show
          Jakob Korherr added a comment - Digging into this, I found out that in MyFaces 2.0 we actually do not need the ThreadLocal for the RuntimeConfig anymore, because we have a FacesContext and a ExternalContext available at startup (and shutdown). Thus setting the RuntimeConfig is not necessary, because we can simply get it via getCurrentInstance(). By the way: the ThreadLocal in ApplicationImpl was introduced in revision 511514 (MyFaces 1.2), and of course there was no FacesContext available at initialisation time. Now, it is! Attached is a patch, which removes the ThreadLocal from ApplicationImpl and uses RuntimeConfig.getCurrentInstance() instead. I tested it and could not find any problems with this approach, since it returns the exact same instance of RuntimeConfig. If there are no objections, I will commit this patch soon. @Martin, Bruno: could you please test this patch and see if the memory problems are still there? Thanks!
          Hide
          Jakob Korherr added a comment -

          Committed patch for ApplicationImpl.

          Now I am working on the MetadataTargetImpl problem mentioned by Martin. I found out that this is exactly the WeakHashMap-issue that Stan mentioned.

          In that issue the value of the WeakHashMap directly references the key and thus the key cannot be GCed (see javadoc of WeakHashMap for more details). Before revision 962933 we were using the Class name (=String) as the key for the WeakHashMap and thus did not have this value-key-references. However, this turned out to cause problems if there are multiple versions of the "same" Class available (e.g. in a shared classloader scenario, see MYFACES-2806).

          A possible solution to this problem is to use a WeakReference for the value (suggested by the javadoc of WeakHashMap). Another one would be to re-introduce the Class name (=String) as the key for the WeakHashMap instead of the Class instance itself, but to also query for the current ClassLoader (thus having a Map<ClassLoader<Map<String, MetadataTarget>).

          Show
          Jakob Korherr added a comment - Committed patch for ApplicationImpl. Now I am working on the MetadataTargetImpl problem mentioned by Martin. I found out that this is exactly the WeakHashMap-issue that Stan mentioned. In that issue the value of the WeakHashMap directly references the key and thus the key cannot be GCed (see javadoc of WeakHashMap for more details). Before revision 962933 we were using the Class name (=String) as the key for the WeakHashMap and thus did not have this value-key-references. However, this turned out to cause problems if there are multiple versions of the "same" Class available (e.g. in a shared classloader scenario, see MYFACES-2806 ). A possible solution to this problem is to use a WeakReference for the value (suggested by the javadoc of WeakHashMap). Another one would be to re-introduce the Class name (=String) as the key for the WeakHashMap instead of the Class instance itself, but to also query for the current ClassLoader (thus having a Map<ClassLoader<Map<String, MetadataTarget>).
          Hide
          Jakob Korherr added a comment -

          I tested a lot of scenarios locally to find out why the WebappClassLoader is not GCed, but I did not find a result.

          I locally removed all ThreadLocal instances from the MyFaces code and also fixed the problem with the WeakHashMap mentioned by Stan, but the WebappClassLoader still isn't GCed.

          Suggestions are welcome...

          Show
          Jakob Korherr added a comment - I tested a lot of scenarios locally to find out why the WebappClassLoader is not GCed, but I did not find a result. I locally removed all ThreadLocal instances from the MyFaces code and also fixed the problem with the WeakHashMap mentioned by Stan, but the WebappClassLoader still isn't GCed. Suggestions are welcome...
          Hide
          Leonardo Uribe added a comment -

          I think the problem is relate to the WeakHashMap is never released. In theory to use the WebappClassLoader as key you should do something similar to the hack done in javax.faces.FactoryFinder. So in this case when the application is undeployed, we should ensure we remove the key and after that all the values will be GCed.

          Show
          Leonardo Uribe added a comment - I think the problem is relate to the WeakHashMap is never released. In theory to use the WebappClassLoader as key you should do something similar to the hack done in javax.faces.FactoryFinder. So in this case when the application is undeployed, we should ensure we remove the key and after that all the values will be GCed.
          Hide
          Jakob Korherr added a comment -

          I locally changed this scenario to use the ApplicationMap instead of a static WeakHashMap and also replaced some ThreadLocals with FacesContext.getAttributes(), but the WebappClassLoader still isn't GCed, however I actually cannot find a (hard-)reference to it via the memory analyzer. The commit for this will follow tomorrow - I have to cleanup some things first.

          Any other suggestions why WebappClassLoader just won't be undeployed? It does not seem to be caused by the ThreadLocals or the WeakHashMap in MetaRulesetImpl.

          Show
          Jakob Korherr added a comment - I locally changed this scenario to use the ApplicationMap instead of a static WeakHashMap and also replaced some ThreadLocals with FacesContext.getAttributes(), but the WebappClassLoader still isn't GCed, however I actually cannot find a (hard-)reference to it via the memory analyzer. The commit for this will follow tomorrow - I have to cleanup some things first. Any other suggestions why WebappClassLoader just won't be undeployed? It does not seem to be caused by the ThreadLocals or the WeakHashMap in MetaRulesetImpl.
          Hide
          Martin Kočí added a comment -

          What application (.war) do you use to reproduce this problem with WebappClassLoader? It can be also problem in tomcat - did you try tomcat version 7 ?

          Show
          Martin Kočí added a comment - What application (.war) do you use to reproduce this problem with WebappClassLoader? It can be also problem in tomcat - did you try tomcat version 7 ?
          Hide
          Mark Struberg added a comment -

          Hi!

          I remember that I did put that in because it was the easiest way to get a shared MyFaces (in the geronimo lib classpath) running with multiple WebApps. Another option would be to introduce an outer Map<ClassLoader, Map<yourconfig..>> to get the proper config for each WebAppClassLoader.

          Show
          Mark Struberg added a comment - Hi! I remember that I did put that in because it was the easiest way to get a shared MyFaces (in the geronimo lib classpath) running with multiple WebApps. Another option would be to introduce an outer Map<ClassLoader, Map<yourconfig..>> to get the proper config for each WebAppClassLoader.
          Hide
          Jakob Korherr added a comment -

          @Martin: I am using the sample app from the myfaces archetypes (really, really small) and tomcat 7.0.2, 6.0.29 and 6.0.20.

          I just committed a solution which removes all ThreadLocals, which we can't really guarantee to be removed(), from the code and uses FacesContext.getAttributes() instead, because this is also local for the current Thread, but will certainly be released.

          Furthermore I put the MetadataTarget cache from MetaRulesetImpl on the Application Map, thus these instances will certainly be removed upon undeploy, regardless of the WebappClassLoader beeing GCed. Furthermore this also gets rid of the problems with multiple webapps in a shared classloader scenario (Application Maps are unique per webapp).

          However, tomcat (all versions) still tells me that there is a possible memory leak in my mini-app. But again, if I use the profiler I can't find any non-weak non-soft references to the WebappClassLoader, but it really is still there. One good thing though is that the WebappClassLoader is now a lot smaller (not even 1 MB), because all MetadataTarget instances are gone.

          Further suggestions are appreciated! I think we are really near to an acceptable solution.

          Show
          Jakob Korherr added a comment - @Martin: I am using the sample app from the myfaces archetypes (really, really small) and tomcat 7.0.2, 6.0.29 and 6.0.20. I just committed a solution which removes all ThreadLocals, which we can't really guarantee to be removed(), from the code and uses FacesContext.getAttributes() instead, because this is also local for the current Thread, but will certainly be released. Furthermore I put the MetadataTarget cache from MetaRulesetImpl on the Application Map, thus these instances will certainly be removed upon undeploy, regardless of the WebappClassLoader beeing GCed. Furthermore this also gets rid of the problems with multiple webapps in a shared classloader scenario (Application Maps are unique per webapp). However, tomcat (all versions) still tells me that there is a possible memory leak in my mini-app. But again, if I use the profiler I can't find any non-weak non-soft references to the WebappClassLoader, but it really is still there. One good thing though is that the WebappClassLoader is now a lot smaller (not even 1 MB), because all MetadataTarget instances are gone. Further suggestions are appreciated! I think we are really near to an acceptable solution.
          Hide
          Leonardo Uribe added a comment -

          Hi

          It is good to use FacesContext attribute map instead (anyway FacesContext.getCurrentInstance uses a ThreadLocal var, but it is correctly cleaned after request ends), but I'm not agree with remove all ThreadLocal instances, because that's not the problem. The problem is add code that "clean" those instances where it is required.

          There are valid cases where ThreadLocal usage is required. For example, in FlashELResolver it is possible that an EL expression could be resolved outside normal JSF lifecycle, where there is no FacesContext set, so the code committed will not work as expected. The same applies for VariableResolverToELResolver, so that code must be reverted to its original form.

          I see that my suggestion about "clear" the map was not very clear. I was thinking on add some code inside org.apache.myfaces.webapp.AbstractFacesInitializer.destroyFaces(ServletContext) to do proper cleanup (if you have a WeakHashMap somewhere, call a method that removes the key).

          For the case of MetaRulesetImpl, I think use a WeakHashMap and do proper cleanup on AbstractFacesInitializer.destroyFaces is better. There is no reason to call FacesContext.getCurrentInstance(), since the returned instances are "application scope" and once initialized does not change.

          Show
          Leonardo Uribe added a comment - Hi It is good to use FacesContext attribute map instead (anyway FacesContext.getCurrentInstance uses a ThreadLocal var, but it is correctly cleaned after request ends), but I'm not agree with remove all ThreadLocal instances, because that's not the problem. The problem is add code that "clean" those instances where it is required. There are valid cases where ThreadLocal usage is required. For example, in FlashELResolver it is possible that an EL expression could be resolved outside normal JSF lifecycle, where there is no FacesContext set, so the code committed will not work as expected. The same applies for VariableResolverToELResolver, so that code must be reverted to its original form. I see that my suggestion about "clear" the map was not very clear. I was thinking on add some code inside org.apache.myfaces.webapp.AbstractFacesInitializer.destroyFaces(ServletContext) to do proper cleanup (if you have a WeakHashMap somewhere, call a method that removes the key). For the case of MetaRulesetImpl, I think use a WeakHashMap and do proper cleanup on AbstractFacesInitializer.destroyFaces is better. There is no reason to call FacesContext.getCurrentInstance(), since the returned instances are "application scope" and once initialized does not change.
          Hide
          Martin Kočí added a comment -

          With EL 2.2 implementation from Glassfish, tomcat 6.0.X and myfaces helloworld20 archetype, instance of https://uel.dev.java.net/svn/uel/trunk/api/src/main/java/javax/el/BeanELResolver.java holds reference to HelloWorldController.class in:

          private static final ConcurrentHashMap<Class, BeanProperties> properties = new ConcurrentHashMap<Class, BeanProperties>(CACHE_SIZE);

          and keeps the classloader alive. This is cleary problem in this particular EL implementation. After I commented out that caching, classloader is GCed.

          Tomcat uses better solution:
          http://svn.apache.org/viewvc/tomcat/trunk/java/javax/el/BeanELResolver.java?revision=1022623&view=markup

          so this problem should not be general.

          Show
          Martin Kočí added a comment - With EL 2.2 implementation from Glassfish, tomcat 6.0.X and myfaces helloworld20 archetype, instance of https://uel.dev.java.net/svn/uel/trunk/api/src/main/java/javax/el/BeanELResolver.java holds reference to HelloWorldController.class in: private static final ConcurrentHashMap<Class, BeanProperties> properties = new ConcurrentHashMap<Class, BeanProperties>(CACHE_SIZE); and keeps the classloader alive. This is cleary problem in this particular EL implementation. After I commented out that caching, classloader is GCed. Tomcat uses better solution: http://svn.apache.org/viewvc/tomcat/trunk/java/javax/el/BeanELResolver.java?revision=1022623&view=markup so this problem should not be general.
          Hide
          Leonardo Uribe added a comment -

          I reverted the code on FlashELResolver and commit some enhancements to UIComponent shared StringBuilder.

          Show
          Leonardo Uribe added a comment - I reverted the code on FlashELResolver and commit some enhancements to UIComponent shared StringBuilder.
          Hide
          Jakob Korherr added a comment -

          Hi Leo,

          Sorry for the late answer.

          > I see that my suggestion about "clear" the map was not very clear. I was thinking on add some code inside org.apache.myfaces.webapp.AbstractFacesInitializer.destroyFaces(ServletContext) to do proper cleanup (if you have a WeakHashMap somewhere, call a method that removes the key).
          > For the case of MetaRulesetImpl, I think use a WeakHashMap and do proper cleanup on AbstractFacesInitializer.destroyFaces is better. There is no reason to call FacesContext.getCurrentInstance(), since the returned instances are "application scope" and once initialized does not change.

          You're right, Leo. In this case it might be better to use the static WeakHashMap<ClassLoader, WeakHashMap<String, MetadataTarget>> and ensure a cleanup on application shutdown. This will be faster. The reason I put this on ApplicationMap was simply to use the advantage of ApplicationMap beeing cleared automatically. I'll change this.

          > There are valid cases where ThreadLocal usage is required. For example, in FlashELResolver it is possible that an EL expression could be resolved outside normal JSF lifecycle, where there is no FacesContext set, so the code committed will not work as expected. The same applies for VariableResolverToELResolver, so that code must be reverted to its original form.

          I disagree, because the FlashELResolver only makes sence for JSF and it actually really does use the FacesContext, however indirectly, because it gets the ExternalContext via the FacesContext. So if you have no FacesContext at all the FlashElResolver will fail no matter what (no FacesContext means no ExternalContext means no Flash). Furthermore you can't really remove() the ThreadLocal in FlashELResolver in a consistent way other than using an additional request-listener. I think that using the FacesContext here instead of the ThreadLocal is a better alternative, but please feel free to convince me otherwise!

          Jakob

          Show
          Jakob Korherr added a comment - Hi Leo, Sorry for the late answer. > I see that my suggestion about "clear" the map was not very clear. I was thinking on add some code inside org.apache.myfaces.webapp.AbstractFacesInitializer.destroyFaces(ServletContext) to do proper cleanup (if you have a WeakHashMap somewhere, call a method that removes the key). > For the case of MetaRulesetImpl, I think use a WeakHashMap and do proper cleanup on AbstractFacesInitializer.destroyFaces is better. There is no reason to call FacesContext.getCurrentInstance(), since the returned instances are "application scope" and once initialized does not change. You're right, Leo. In this case it might be better to use the static WeakHashMap<ClassLoader, WeakHashMap<String, MetadataTarget>> and ensure a cleanup on application shutdown. This will be faster. The reason I put this on ApplicationMap was simply to use the advantage of ApplicationMap beeing cleared automatically. I'll change this. > There are valid cases where ThreadLocal usage is required. For example, in FlashELResolver it is possible that an EL expression could be resolved outside normal JSF lifecycle, where there is no FacesContext set, so the code committed will not work as expected. The same applies for VariableResolverToELResolver, so that code must be reverted to its original form. I disagree, because the FlashELResolver only makes sence for JSF and it actually really does use the FacesContext, however indirectly, because it gets the ExternalContext via the FacesContext. So if you have no FacesContext at all the FlashElResolver will fail no matter what (no FacesContext means no ExternalContext means no Flash). Furthermore you can't really remove() the ThreadLocal in FlashELResolver in a consistent way other than using an additional request-listener. I think that using the FacesContext here instead of the ThreadLocal is a better alternative, but please feel free to convince me otherwise! Jakob
          Hide
          Leonardo Uribe added a comment -

          Hi Jakob

          Yes, you're right. Since Flash scope was introduced in JSF 2.0 and is an object that only can be retrieved through ExternalContext/FacesContext, with the current code such evaluation will fail too, so there will be no problem if we use FacesContext instead. Please apply your change again.

          Leonardo

          Show
          Leonardo Uribe added a comment - Hi Jakob Yes, you're right. Since Flash scope was introduced in JSF 2.0 and is an object that only can be retrieved through ExternalContext/FacesContext, with the current code such evaluation will fail too, so there will be no problem if we use FacesContext instead. Please apply your change again. Leonardo
          Hide
          Jakob Korherr added a comment -

          I just committed the last discussed point for this issue.

          Does anyone still see memory leaks or problems with ThreadLocals or something similar? If not, I will resolve this one.

          Show
          Jakob Korherr added a comment - I just committed the last discussed point for this issue. Does anyone still see memory leaks or problems with ThreadLocals or something similar? If not, I will resolve this one.
          Hide
          Jakob Korherr added a comment -

          Noone seems to see a memory leak anymore - resolving this one.

          Show
          Jakob Korherr added a comment - Noone seems to see a memory leak anymore - resolving this one.
          Hide
          Andrei Zagorneanu added a comment -

          Is it resolved for MyFaces 1.2.x also? I'm using MyFaces 1.2.9 and I'm getting the following when I'm stopping Tomcat:

          11.12.2010 12:55:04 org.apache.catalina.loader.WebappClassLoader clearThreadLocalMap
          SEVERE: The web application [/foe-web-app] created a ThreadLocal with key of type [java.lang.ThreadLocal] (value [java.lang.ThreadLocal@9eca0a]) and a value of type [org.apache.myfaces.config.RuntimeConfig] (value [org.apache.myfaces.config.RuntimeConfig@e28500]) but failed to remove it when the web application was stopped. This is very likely to create a memory leak.
          11.12.2010 12:55:04 org.apache.catalina.loader.WebappClassLoader clearThreadLocalMap
          SEVERE: The web application [/foe-web-app] created a ThreadLocal with key of type [null] (value [org.apache.myfaces.el.convert.VariableResolverToELResolver$1@1414d1e]) and a value of type [java.util.HashSet] (value [[]]) but failed to remove it when the web application was stopped. This is very likely to create a memory leak.

          Show
          Andrei Zagorneanu added a comment - Is it resolved for MyFaces 1.2.x also? I'm using MyFaces 1.2.9 and I'm getting the following when I'm stopping Tomcat: 11.12.2010 12:55:04 org.apache.catalina.loader.WebappClassLoader clearThreadLocalMap SEVERE: The web application [/foe-web-app] created a ThreadLocal with key of type [java.lang.ThreadLocal] (value [java.lang.ThreadLocal@9eca0a] ) and a value of type [org.apache.myfaces.config.RuntimeConfig] (value [org.apache.myfaces.config.RuntimeConfig@e28500] ) but failed to remove it when the web application was stopped. This is very likely to create a memory leak. 11.12.2010 12:55:04 org.apache.catalina.loader.WebappClassLoader clearThreadLocalMap SEVERE: The web application [/foe-web-app] created a ThreadLocal with key of type [null] (value [org.apache.myfaces.el.convert.VariableResolverToELResolver$1@1414d1e] ) and a value of type [java.util.HashSet] (value [[]]) but failed to remove it when the web application was stopped. This is very likely to create a memory leak.
          Hide
          Jakob Korherr added a comment -

          Reopened, because it must be fixed for MyFaces core 1.2.x too.

          Show
          Jakob Korherr added a comment - Reopened, because it must be fixed for MyFaces core 1.2.x too.
          Hide
          Jakob Korherr added a comment -

          add 1.2.9 as affected version.

          Show
          Jakob Korherr added a comment - add 1.2.9 as affected version.
          Hide
          Leonardo Uribe added a comment -

          Created MYFACES-3001 to handle this one for 1.2 branch

          Show
          Leonardo Uribe added a comment - Created MYFACES-3001 to handle this one for 1.2 branch

            People

            • Assignee:
              Jakob Korherr
              Reporter:
              Werner Punz
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development