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.
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
Created attachment 28434 [details] new threadlocal and executor classloader leak detection tests new tests for memory leak detection feature
Developers please code review and proceed with next steps --Thanks, Rohit Kelapure
Created attachment 28442 [details] reworked patch after making some modifications to loadedByThisOrChild
(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.
Fixed in trunk and 7.0.x and will be included in 7.0.27 onwards. Many thanks for the patch.
(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.
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.
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.
s/not detect/now detect/
Created attachment 28893 [details] 2012-06-05_tc6_52850_WebappClassLoader.patch Backport of this fix to 6.0, based on r1298986 and r1346519
Proposed for 6.0
Fixed in 6.0 with r1356198 and will be in 6.0.36
I have Tomcat 7.0 running. Where can I download this patch so that I can install on the server?