|Summary:||swallowOutput causes memory leak|
|Product:||Tomcat 5||Reporter:||Robert Wille <tomcat-leak>|
|Component:||Catalina||Assignee:||Tomcat Developers Mailing List <dev>|
Add addThread and removeThread to SystemLogHandler
Use ThreadPoolListener - avoid thread memory leak
Use ThreadPoolListener - avoid thread memory leak
Compiled classes and full source code
ThreadLocal Patch for Jaspers SystemLogHandler
ThreadLocal Patch for connectors SystemLogHandler
Complete Code and classes for the two patches
Description Robert Wille 2005-02-02 20:37:57 UTC
SystemLogHandler has a map called logs. The key is a ThreadWithAttributes and the value is a stack of CaptureLogs. Because threads come and go from the thread pool, this map slowly gets bigger and bigger. logs should be a ThreadLocal, not a map.
Comment 1 Rainer Jung 2005-02-04 18:59:26 UTC
I did some parallel work this week and found the same result for mod_jk and AJP/13 connector with JDK1.5 hprof. We can see the leak in production too, because we have a lot of worker thread creation and destruction. The analysis of Robert is correct. I did a quick fix by adding addThread and removeThread methods to /org/apache/tomcat/util/log/SystemLogHandler.java and then registered a ThreadPoolListener in /org/apache/jk/common/ChannelSocket.java that handles addThread and removeThread. Still need to verify, that the leak is gone. IT#s intersting that there is another class /org/apache/jasper/util/SystemLogHandler.java that is used in a similar way, but cares better about freeing the thread objects at the end of work. If you are interested I will produce a patch for the Coyote HTTP connector too, so you can check, if that would fix the problem.
Comment 2 Remy Maucherat 2005-02-04 20:13:23 UTC
Using a thread local or a weak reference seems the way of fixing the issue. Anyway, I don't quite understand why the class uses a stack and stuff, and isn't simply identical to the Jasper one. I don't see any recursion going on. Since you're actually using the class, maybe you can test this ?
Comment 3 Rainer Jung 2005-02-05 04:57:27 UTC
As a workaround for the 5.0 branch I attach a patch for - /org/apache/tomcat/util/log/SystemLogHandler.java - /org/apache/jk/common/ChannelSocket.java - /java/org/apache/coyote/http11/Http11Protocol.java It uses a ThreadPoolListener (that was already present in Http11Protocol). I tested it with creation of app. 44.000 Threads for Coyote HTTP as well as for the AJP/13 connector (and getting a bunch of non-session static content for each thread). After each creation of 2.000 threads I did a Full GC. Old Generation varies betweeen 20545K and 28517K with no noticeable increase in time (first GC 26319K, last GC 27450K).
Comment 4 Rainer Jung 2005-02-05 05:04:15 UTC
Created attachment 14184 [details] Add addThread and removeThread to SystemLogHandler
Comment 5 Rainer Jung 2005-02-05 05:05:11 UTC
Created attachment 14185 [details] Use ThreadPoolListener - avoid thread memory leak
Comment 6 Rainer Jung 2005-02-05 05:05:37 UTC
Created attachment 14186 [details] Use ThreadPoolListener - avoid thread memory leak
Comment 7 Rainer Jung 2005-02-05 05:07:39 UTC
Created attachment 14187 [details] Compiled classes and full source code For a test extract the classes from the jar to server/classes. Compatible (at least) with 5.0.27-5.0.31.
Comment 8 Remy Maucherat 2005-02-05 10:51:39 UTC
Thanks, but as I have stated, I am not interested in this way to fix it ;)
Comment 9 Rainer Jung 2005-02-05 15:20:03 UTC
Remy: I did understand, that you prefer some refactoring there. But you weren't precise about only introducing a ThreadLocal (easy) or also making analysis on probable recursion. I changed the logs HashTable inside SystemLogHandler to a ThreadLocal and I'm running a series of tests to see, if the memory leak is gone. I wil post the small patch (for both SystemLogHandlers) as soon, as the tests succeed. I would not very much like to go deeper into the question, if a stack (supporting recursion) is really necessary. In the jasper case it is not (and so not in the code), but the case of the filters and valves using /org/apache/tomcat/util/log/SystemLogHandler.java might be different (and even different in the various TC versions).
Comment 10 Rainer Jung 2005-02-05 17:54:40 UTC
Created attachment 14189 [details] ThreadLocal Patch for Jaspers SystemLogHandler
Comment 11 Rainer Jung 2005-02-05 17:55:13 UTC
Created attachment 14190 [details] ThreadLocal Patch for connectors SystemLogHandler
Comment 12 Rainer Jung 2005-02-05 18:02:21 UTC
Created attachment 14191 [details] Complete Code and classes for the two patches Test by extracting classes to server/classes. I could not reproduce the thread memory leak with this version. New version of the Jasper SystemLogHandler is compatible with Revision 1.4 (5.0 and 5.5 branches), but it seems, that it could also be used instead of 126.96.36.199 (Tomcat 4 branch). For the connectors SystemLogHandler the patch can be applied to 1.5 (5.5 branch) and 188.8.131.52 (5.0 branch), since they are the same.
Comment 13 Remy Maucherat 2005-02-06 12:30:22 UTC
The ThreadLocal solution looks good to me. I think fiding out if a stack is needed or not would be useful, since the code is significantly more complex because of that.
Comment 14 Remy Maucherat 2005-02-08 13:26:06 UTC
I have applied the patch. Thanks.
Comment 15 Mark Thomas 2005-02-14 21:31:02 UTC
This patch has also been back-ported to the TC4 branch.
Comment 16 Filip Hanik 2006-04-12 22:34:05 UTC
patch applied to 5.0