voyent
PersistentFacesState memory leak / bloat with InheritableThreadLocal in IceFaces 1.7+  XML
Forum Index -> Contributor's Corner
Author Message
maratb

Joined: 17/Dec/2006 00:00:00
Messages: 13
Offline


Let me just make sure that the first reaction to this post will not be that this is an application bug.

You can take a look at the link bellow

http://apache-wicket.1842946.n4.nabble.com/WICKET-2846-Store-Application-in-InheritableThreadLocal-instead-of-ThreadLocal-td2222639.html

for a discussion where the app developer raised this issue already but fell on a deaf ear.

Basically I have a web app that uses straight hibernate, c3p0 database thead pool and ehcache. I'm sure that this is quite a typical setup for a web app.

I'be been experiencing a memory bloat of my web app, which would ultimatelly lead to PermGen OOM Exception. I'm still on IceFaces 1.7.2, though looking at 1.8.2 it appears that the code in question is still the same so the bloat is probably still there.

The bloat is due to PersistentFacesState use of InheritableThreadLocal AND the fact that the application code uses third party libraries that spawn child threads in the context of the jsf/servlet request processing. Those child threads inherit a reference to PersistentFacesState via Thread.inheritableThreadLocals.

Even though Thread.inheritableThreadLocals's entries is of type ThrehadLocal.ThreadLocalMap.Entry extends WeakReference<ThreadLocal> the way it is implemented seems to allow for a possibility of a memory leak in the specific scenario that I described. A child thread that keeps Thread.inheritableThreadLocals with an entiry pointing to PersistentFacesState but never uses a its ThreadLocal is not going to release Entry.value to the garbage collector because ThrehadLocal.ThreadLocalMap will only attempt to dereference its Entry on a ThrehadLocal.ThreadLocalMap.get() call.

So here is one way to fix for this bloat.

The basic idea is to allow to filter out ThreadLocal values during child thread initialization by overriding InheritableThreadLocal.childValue() method. How that decision is made can be implementation specific.

Attached code provides for the following: given a collection of class names that represent child thread classes that get spawned by third party framework, take the current stack trace and try to locate the class name from that collection. If found do not propagate this ThreadLocal's value reference to the child's thread Thread.inheritableThreadLocals

I hope people find this useful. This worked out quite nicely for me. I'm attaching a snapshot of YourKit before and afer this fix. You can see that there is no more PersistentFacesState on the second memory snapshot as the biggest in memory object.

Before the fix: http://www.flickr.com/photos/65203168@N05/5938864379/in/photostream

After the fix: http://www.flickr.com/photos/65203168@N05/5938864437/in/photostream

Here is the link to the code bellow in a readable format:

http://pastebin.com/iBqVkfL5


Code:
 ------------- PersistentFacesState.java
 
 package com.icesoft.faces.webapp.xmlhttp
 
 public class PersistentFacesState implements Serializable {
     private static InheritableThreadLocal localInstance = new InheritableThreadLocal() {
             protected Object childValue(Object parentValue) {
                 if (parentValue == null) {
                     return parentValue;
                 }
                 PersistentFacesState state = (PersistentFacesState)parentValue;
                 return state.isInheritable() ? state : null;
             }
     };
 
     public boolean isInheritable() {
         String value = 
             facesContext.getExternalContext().getInitParameter("InheritableThreadLocalExcludeFilter");
         if (value == null) {
             return true;
         }
         StackTraceElement[] elements = Thread.currentThread().getStackTrace();
         String[] classNames = value.split(";");
         String rhsClassName;
         for (int i = 0; i < classNames.length; ++i) {
             rhsClassName =  classNames[i];
             for (int j = 0; j < elements.length; ++j) {
                 StackTraceElement e = elements[j];
                 String lhsClassName = e.getClassName();
                 if (lhsClassName.equals(rhsClassName)) {
                     return false;
                 }
             }
         }
         return true;
     }
 

------------- web.xml
Code:
   <context-param>
     <param-name>InheritableThreadLocalExcludeFilter</param-name>
     <param-value>net.sf.ehcache.CacheManager$1;java.util.TimerThread;com.mchange.v2.async.ThreadPoolAsynchronousRunner$PoolThread;net.sf.ehcache.store.DiskStore$SpoolAndExpiryThread</param-value>
   </context-param>
 


Thanks,
Marat
ted.goddard

Joined: 26/Oct/2004 00:00:00
Messages: 874
Offline


Another fix would be to make the InheritableThreadLocal behavior switchable via web.xml parameter (since in retrospect the convenience of InheritableThreadLocal is not worth the other complications).
[Email]
maratb

Joined: 17/Dec/2006 00:00:00
Messages: 13
Offline


ted.goddard wrote:
Another fix would be to make the InheritableThreadLocal behavior switchable via web.xml parameter (since in retrospect the convenience of InheritableThreadLocal is not worth the other complications).
 


I thought about it too at first as well. However, won't this break the asynchronous architecture of icefaces by not letting user children threads have access to PersistentFacesState to request asych render back to the web browser?

So I think while easiest to do it would the wrong way to go about it.

Btw, you could flip what I did around and provide a way for the user to register threads that have to have PersistentFacesState in their thread local by way of a factory pattern for example:

Code:
 Thread IceFacesThreadFactory.create(Class<? inherits Thread>);
 Thread IceFacesThreadFactory.create(Class<? inherits Runnable>);
 


The implementation of these methods would register the instance of this thread with some kind of a registry controlled by IceFaces framework, which PersistentFacesState.isInheritable() will consult to see if the thread its is being executed in is already registered with the this registry.

So this way by default no thread gets its threadlocals populated with PersistentFacesState and only explicitly registered user threads get it.

Thanks,
Marat
ted.goddard

Joined: 26/Oct/2004 00:00:00
Messages: 874
Offline


InheritableThreadLocal is provided only as a convenience -- it should be possible to pass the PersistentFacesState to the Object used by the spawned Thread.
[Email]
 
Forum Index -> Contributor's Corner
Go to:   
Powered by JForum 2.1.7ice © JForum Team