You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@excalibur.apache.org by Vadim Gritsenko <va...@reverycodes.com> on 2005/08/24 01:11:27 UTC
One more change to be included into the release...
... or ThreadLocals are EVIL! If you have not heard the word yet, ThreadLocals
tend to prevent (web) application(s) from being garbage collected when thread
locals are not properly nulled out.
So in order to allow web applications using LogKit to be properly unloaded,
LogKit's ContextMap has to have one more method:
/**
* Remove current ContextMap.
* This method removes a ContextMap associated with current thread,
* if there is any.
*/
public static void removeCurrentContext()
{
c_localContext.set(null);
}
Consistent usage of this method, like:
try {
... ContextMap.getCurrentContext() ...
} finally {
ContextMap.removeCurrentContext();
}
will guarantee that ThreadLocal is properly cleaned up and web app will be
properly unloaded. I'd like to include this addition into the release; and IMHO
there is no reason to do yet another RC build just because of it :-) because it
is simple addition which does not alter any existing behavior.
WDYT?
Vadim
PS http://www.jroller.com/page/tackline?entry=fixing_threadlocal
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@excalibur.apache.org
For additional commands, e-mail: dev-help@excalibur.apache.org
Re: One more change to be included into the release...
Posted by Carsten Ziegeler <cz...@apache.org>.
Antonio Gallardo wrote:
>>
>>Yepp, we had to fix such a ThreadLocal in Xalan recently which eat up
>>3GB of memory in four hours! With the ThreadLocal properly reset, the
>>app runs at an average of 300MB...
>>
>>
>
> Wow! Is this affecting cocoon? If this is affecting cocoon code, I will
> prefer to add a patched excalibur jar into cocoon. ;-)
>
It depends - the bug is in Xalan, not excalibur, so you need to patch
Xalan - I filed this into Jira as XALANJ-2178. You're only affected by
it if you're using the Xalan extensions to write files directly from
within the stylesheet - so the usual Cocoon user does not need the
patch. I really hope that the Xalan guys will do something about it for
the next release.
Carsten
--
Carsten Ziegeler - Open Source Group, S&N AG
http://www.s-und-n.de
http://www.osoco.org/weblogs/rael/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@excalibur.apache.org
For additional commands, e-mail: dev-help@excalibur.apache.org
Re: One more change to be included into the release...
Posted by Antonio Gallardo <ag...@agssa.net>.
Carsten Ziegeler wrote:
>Vadim Gritsenko wrote:
>
>
>>... or ThreadLocals are EVIL! If you have not heard the word yet, ThreadLocals
>>tend to prevent (web) application(s) from being garbage collected when thread
>>locals are not properly nulled out.
>>
>>
>Yepp, we had to fix such a ThreadLocal in Xalan recently which eat up
>3GB of memory in four hours! With the ThreadLocal properly reset, the
>app runs at an average of 300MB...
>
>
Wow! Is this affecting cocoon? If this is affecting cocoon code, I will
prefer to add a patched excalibur jar into cocoon. ;-)
Best Regards,
Antonio Gallardo.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@excalibur.apache.org
For additional commands, e-mail: dev-help@excalibur.apache.org
Re: One more change to be included into the release...
Posted by Vadim Gritsenko <va...@reverycodes.com>.
Carsten Ziegeler wrote:
> Vadim Gritsenko wrote:
>
>>So as you can see there is a strong reference from ContextMap class to the
>>ThreadLocal which prevents GC of the WebAppClassLoader :-) See also [1] and
>>referenced BugParade entry.
>
> Ah, ok - what a crap; so actually this means that one should clear all
> ThreadLocal variables. Are only static thread locals effected or also
> instance variables using thread locals?
Unless you do something like foo.tLocal.set(foo) - which creates reference from
ThreadLocalMap.Entry.value back to tLocal - you should be ok :-) Rule is, there
should be no strong reference from value of the thread local back to thread local.
Vadim
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@excalibur.apache.org
For additional commands, e-mail: dev-help@excalibur.apache.org
Re: One more change to be included into the release...
Posted by Carsten Ziegeler <cz...@apache.org>.
Vadim Gritsenko wrote:
> Of course it is required, and of course it is a problem :-)
>
> Thread (pooled tomcat thread)
> --> ThreadLocalMap
> --> ThreadLocalMap.Entry
> --weak--> ThreadLocal (ContextMap.c_localContext)
> -strong-> ContextMap Object
> --> ContextMap Class
> --> ThreadLocal (c_localContext)
> --> WebAppClassLoader (<loader>)
>
> So as you can see there is a strong reference from ContextMap class to the
> ThreadLocal which prevents GC of the WebAppClassLoader :-) See also [1] and
> referenced BugParade entry.
>
Ah, ok - what a crap; so actually this means that one should clear all
ThreadLocal variables. Are only static thread locals effected or also
instance variables using thread locals?
Carsten
--
Carsten Ziegeler - Open Source Group, S&N AG
http://www.s-und-n.de
http://www.osoco.org/weblogs/rael/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@excalibur.apache.org
For additional commands, e-mail: dev-help@excalibur.apache.org
Re: One more change to be included into the release...
Posted by Vadim Gritsenko <va...@reverycodes.com>.
Carsten Ziegeler wrote:
> Vadim Gritsenko wrote:
>
>>Consistent usage of this method, like:
>>
>>try {
>> ... ContextMap.getCurrentContext() ...
>>} finally {
>> ContextMap.removeCurrentContext();
>>}
>>
>>will guarantee that ThreadLocal is properly cleaned up and web app will be
>>properly unloaded. I'd like to include this addition into the release; and IMHO
>>there is no reason to do yet another RC build just because of it :-) because it
>>is simple addition which does not alter any existing behavior.
>
> Hmm, is this really required? The context map has a clear method
> clearing all items stored in the map. The only object that is not
> removed is the map itself, which shouldn't be a problem.
Of course it is required, and of course it is a problem :-)
Thread (pooled tomcat thread)
--> ThreadLocalMap
--> ThreadLocalMap.Entry
--weak--> ThreadLocal (ContextMap.c_localContext)
-strong-> ContextMap Object
--> ContextMap Class
--> ThreadLocal (c_localContext)
--> WebAppClassLoader (<loader>)
So as you can see there is a strong reference from ContextMap class to the
ThreadLocal which prevents GC of the WebAppClassLoader :-) See also [1] and
referenced BugParade entry.
Vadim
[1] http://www.jroller.com/page/tackline?entry=fixing_threadlocal
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@excalibur.apache.org
For additional commands, e-mail: dev-help@excalibur.apache.org
Re: One more change to be included into the release...
Posted by Carsten Ziegeler <cz...@apache.org>.
Vadim Gritsenko wrote:
> ... or ThreadLocals are EVIL! If you have not heard the word yet, ThreadLocals
> tend to prevent (web) application(s) from being garbage collected when thread
> locals are not properly nulled out.
Yepp, we had to fix such a ThreadLocal in Xalan recently which eat up
3GB of memory in four hours! With the ThreadLocal properly reset, the
app runs at an average of 300MB...
>
> So in order to allow web applications using LogKit to be properly unloaded,
> LogKit's ContextMap has to have one more method:
>
> /**
> * Remove current ContextMap.
> * This method removes a ContextMap associated with current thread,
> * if there is any.
> */
> public static void removeCurrentContext()
> {
> c_localContext.set(null);
> }
>
>
> Consistent usage of this method, like:
>
> try {
> ... ContextMap.getCurrentContext() ...
> } finally {
> ContextMap.removeCurrentContext();
> }
>
> will guarantee that ThreadLocal is properly cleaned up and web app will be
> properly unloaded. I'd like to include this addition into the release; and IMHO
> there is no reason to do yet another RC build just because of it :-) because it
> is simple addition which does not alter any existing behavior.
>
Hmm, is this really required? The context map has a clear method
clearing all items stored in the map. The only object that is not
removed is the map itself, which shouldn't be a problem.
Carsten
--
Carsten Ziegeler - Open Source Group, S&N AG
http://www.s-und-n.de
http://www.osoco.org/weblogs/rael/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@excalibur.apache.org
For additional commands, e-mail: dev-help@excalibur.apache.org