Bug 33368

Summary: swallowOutput causes memory leak
Product: Tomcat 5 Reporter: Robert Wille <tomcat-leak>
Component: CatalinaAssignee: Tomcat Developers Mailing List <dev>
Status: RESOLVED FIXED    
Severity: normal CC: jefft
Priority: P2    
Version: 5.0.28   
Target Milestone: ---   
Hardware: All   
OS: All   
Attachments: 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 1.4 (5.0
and 5.5 branches), but it seems, that it could also be used instead of 1.1.2.2
(Tomcat 4 branch).

For the connectors SystemLogHandler the patch can be applied to 1.5 (5.5
branch) and 1.4.2.1 (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