Bug 52850 - Various miscellaneous fixes to Tomcat Memory Leak Detection code
Various miscellaneous fixes to Tomcat Memory Leak Detection code
Status: RESOLVED FIXED
Product: Tomcat 7
Classification: Unclassified
Component: Catalina
trunk
PC Windows Server 2003
: P2 normal (vote)
: ---
Assigned To: Tomcat Developers Mailing List
:
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2012-03-07 14:57 UTC by Rohit Kelapure
Modified: 2012-11-12 11:19 UTC (History)
1 user (show)



Attachments
src code fixes to WebappClassLoader and new tests for memory leak detection (8.12 KB, patch)
2012-03-07 15:02 UTC, Rohit Kelapure
Details | Diff
new threadlocal and executor classloader leak detection tests (9.96 KB, patch)
2012-03-07 15:03 UTC, Rohit Kelapure
Details | Diff
reworked patch after making some modifications to loadedByThisOrChild (8.33 KB, patch)
2012-03-08 18:19 UTC, Rohit Kelapure
Details | Diff
2012-06-05_tc6_52850_WebappClassLoader.patch (7.55 KB, patch)
2012-06-05 18:59 UTC, Konstantin Kolinko
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rohit Kelapure 2012-03-07 14:57:15 UTC
Problem Description - 
----------------------
Tomcat memory leak detection has the following issues
- Most of the detection and fixing code has been tested ONLY on Sun JVMs. A lot of  the reflection based code does not work with the IBM JDK. 
- Tests in tomcat7.source\test\org\apache\catalina\loader do not seem to run successfully and are incomplete for all the protection that Tomcat provides for classloader memory leaks.
- For some categories of threadlocal memory leaks the key and value are not displayed correctly in the warning messages; particularly ones dealing with indirect references to threadlocals
-  org.apache.catalina.loader.WebappClassLoader.loadedByThisOrChild(Object) incorrectly traverses the object classloader hierarchy instead of the current (this) classloader hierarchy

Problem Conclusion - 
--------------------
- After extensive testing with the IBM JDKs, I have cleaned up and sanitized some of the reflection code to stop executors, timers, threads etc on the IBM JDK
- Added new tests for detecting threadlocal leaks and thread leaks
- Better ThreadLocal leak reporting with corrected loadedByThisOrChild method and addition of expungeStaleEntries method
- Make code more resilient in certain leak detection scenarios.
Comment 1 Rohit Kelapure 2012-03-07 15:02:53 UTC
Created attachment 28433 [details]
src code fixes to WebappClassLoader and new tests for memory leak detection

memory leak detection src code fixes to org.apache.catalina.loader.WebappClassLoader
Comment 2 Rohit Kelapure 2012-03-07 15:03:37 UTC
Created attachment 28434 [details]
new threadlocal and executor classloader leak detection tests

new tests for memory leak detection feature
Comment 3 Rohit Kelapure 2012-03-07 15:05:11 UTC
Developers please code review and proceed with next steps  --Thanks, Rohit Kelapure
Comment 4 Rohit Kelapure 2012-03-08 18:19:21 UTC
Created attachment 28442 [details]
reworked patch after making some modifications to loadedByThisOrChild
Comment 5 Mark Thomas 2012-03-09 15:29:13 UTC
(In reply to comment #0)
> - Most of the detection and fixing code has been tested ONLY on Sun JVMs.
Correct.

> - Tests in tomcat7.source\test\org\apache\catalina\loader do not seem to run
> successfully and are incomplete for all the protection that Tomcat provides for
> classloader memory leaks.
The tests that are present do work. Yes, code coverage of the tests is a long way from 100%.

> - For some categories of threadlocal memory leaks the key and value are not
> displayed correctly in the warning messages; particularly ones dealing with
> indirect references to threadlocals
I don't see any changes to address this. Did I miss them?

> -  org.apache.catalina.loader.WebappClassLoader.loadedByThisOrChild(Object)
> incorrectly traverses the object classloader hierarchy instead of the current
> (this) classloader hierarchy
No. The test is not "loaded by this or parent", it is "loaded by this or child" and is correct.

I haven't looked at the tests yet. I'll commit the fixes (less the change to WebappClassLoader.loadedByThisOrChild(Object)) with the tests once I have reviewed the new tests.
Comment 6 Mark Thomas 2012-03-09 19:13:54 UTC
Fixed in trunk and 7.0.x and will be included in 7.0.27 onwards.

Many thanks for the patch.
Comment 7 Rohit Kelapure 2012-03-09 22:20:48 UTC
(In reply to comment #5)

Thanks for accepting the patch

"I don't see any changes to address this. Did I miss them?"
Ensure that key and value types ALWAYS show up for all types of threadlocal leaks (direct & indirect)
Method expungeStaleEntriesMethod = tlmClass.getDeclaredMethod("expungeStaleEntries");
expungeStaleEntriesMethod.setAccessible(true);


I had to change WebappClassLoader.loadedByThisOrChild(Object)) to get the Threadlocal leak test cases to pass.

I will be grateful if you add a check in the tests that confirms that the leak warnings have showed up in the log. I did not add junit asserts for the presence of log warnings. 

--Thanks


> (In reply to comment #0)
> > - Most of the detection and fixing code has been tested ONLY on Sun JVMs.
> Correct.
> 
> > - Tests in tomcat7.source\test\org\apache\catalina\loader do not seem to run
> > successfully and are incomplete for all the protection that Tomcat provides for
> > classloader memory leaks.
> The tests that are present do work. Yes, code coverage of the tests is a long
> way from 100%.
> 
> > - For some categories of threadlocal memory leaks the key and value are not
> > displayed correctly in the warning messages; particularly ones dealing with
> > indirect references to threadlocals
> I don't see any changes to address this. Did I miss them?
> 
> > -  org.apache.catalina.loader.WebappClassLoader.loadedByThisOrChild(Object)
> > incorrectly traverses the object classloader hierarchy instead of the current
> > (this) classloader hierarchy
> No. The test is not "loaded by this or parent", it is "loaded by this or child"
> and is correct.
> 
> I haven't looked at the tests yet. I'll commit the fixes (less the change to
> WebappClassLoader.loadedByThisOrChild(Object)) with the tests once I have
> reviewed the new tests.

(In reply to comment #5)
> (In reply to comment #0)
> > - Most of the detection and fixing code has been tested ONLY on Sun JVMs.
> Correct.
> 
> > - Tests in tomcat7.source\test\org\apache\catalina\loader do not seem to run
> > successfully and are incomplete for all the protection that Tomcat provides for
> > classloader memory leaks.
> The tests that are present do work. Yes, code coverage of the tests is a long
> way from 100%.
> 
> > - For some categories of threadlocal memory leaks the key and value are not
> > displayed correctly in the warning messages; particularly ones dealing with
> > indirect references to threadlocals
> I don't see any changes to address this. Did I miss them?
> 
> > -  org.apache.catalina.loader.WebappClassLoader.loadedByThisOrChild(Object)
> > incorrectly traverses the object classloader hierarchy instead of the current
> > (this) classloader hierarchy
> No. The test is not "loaded by this or parent", it is "loaded by this or child"
> and is correct.
> 
> I haven't looked at the tests yet. I'll commit the fixes (less the change to
> WebappClassLoader.loadedByThisOrChild(Object)) with the tests once I have
> reviewed the new tests.
Comment 8 Mark Thomas 2012-03-19 19:12:45 UTC
I've been looking at this some more.

The test cases don't do what they are meant to because they don't trigger a leak because the web application class loader isn't used to load the servlets and associated classes. That is almost certainly why you incorrectly thought you needed to rework loadedByThisOrChild.

I have re-worked the tests and direct leaks are now being created and detected as intended.

Indirect leaks appear to be being generated (although I need to double check that it is the intended leak rather than some other leak) but are not yet being logged. Work continues.

I have also have found and fixed a JMX related memory leak.
Comment 9 Mark Thomas 2012-03-19 19:38:58 UTC
The unit tests have been fixed. Indirect leaks weren't being detected. We are never going to be able to detect them all but the code does not detect anything that uses collections.
Comment 10 Mark Thomas 2012-03-19 19:40:43 UTC
s/not detect/now detect/
Comment 11 Konstantin Kolinko 2012-06-05 18:59:34 UTC
Created attachment 28893 [details]
2012-06-05_tc6_52850_WebappClassLoader.patch

Backport of this fix to 6.0, based on r1298986 and r1346519
Comment 12 Konstantin Kolinko 2012-06-05 20:10:37 UTC
Proposed for 6.0
Comment 13 Konstantin Kolinko 2012-07-02 12:07:49 UTC
Fixed in 6.0 with r1356198 and will be in 6.0.36
Comment 14 vijay 2012-11-12 11:19:06 UTC
I have Tomcat 7.0 running. Where can I download this patch so that I can install on the server?