Bug 53936 - Minimize classloader leaks from stacktrace elements in long-lived Exceptions
Minimize classloader leaks from stacktrace elements in long-lived Exceptions
Status: RESOLVED WONTFIX
Product: Tomcat 7
Classification: Unclassified
Component: Catalina
unspecified
PC Windows NT
: P2 enhancement (vote)
: ---
Assigned To: Tomcat Developers Mailing List
:
: 53935 (view as bug list)
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2012-09-26 15:22 UTC by Timo Kinnunen
Modified: 2012-10-01 08:02 UTC (History)
0 users



Attachments
Cleaning Exception stacktrace by carefully calling fillInStackTrace (2.34 KB, text/plain)
2012-09-26 15:23 UTC, Timo Kinnunen
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Timo Kinnunen 2012-09-26 15:22:17 UTC
MemoryLeakProtection in the Tomcat wiki says: "Sun bug 6916498 - An exception can keep a classloader in memory if the stack trace that was recorded when it was created contains a reference to one of its classes." This bug may be considered an acceptable performance tradeoff, so detection and mitigation in the meantime is in my opinion desirable.

The leak is caused by a reference cycle that is only partially visible to the garbage collector. The method fillInStackTrace() can be called on an Exception object to alter its stacktrace, but to effectively break the cycle the method call chain up to that point must be clean of any classes that shouldn't be retained.

The attached test code contains two versions of the algorithm, for Java 1.6 and 1.7. The 1.6 version is intended to be effective only when loaded by a shared classloader, the 1.7 version should be effective even if loaded by a webapp classloader.

The methods are intended to be called before or during webapp unloading/reloading on Exception objects stored in static variables.

Please review to see if I've made any mistakes and consider for enhancement :)
Comment 1 Timo Kinnunen 2012-09-26 15:23:53 UTC
Created attachment 29421 [details]
Cleaning Exception stacktrace by carefully calling fillInStackTrace
Comment 2 Konstantin Kolinko 2012-09-30 17:40:52 UTC
(In reply to comment #0)
> The attached test code contains two versions of the algorithm, for Java 1.6
> and 1.7.

1. It is a bit hard to see, but looks that
 
1.6 version is "containerOnlyFill(Throwable)"
1.7 version is "bootclassLoaderOnlyFill(Throwable)"

Why are you using Throwable[]? It would make sense if you were returning a different value than passing it, but you are not.  Just a simple

  public static void containerOnlyFill(final Throwable throwable) {
    try {
      Runnable target = new Runnable() {
        @Override public void run() {
          throwable.fillInStackTrace();
        }
      };
(...)
  }

would suffice.


2. What is the intended use of this code?

In those several cases when Tomcat creates and holds an exception, we already solved the issue by implementing the fillInStackTrace() method as a NOOP.

See e.g. o.a.naming.resources.ImmutableNameNotFoundException.
Comment 3 Timo Kinnunen 2012-09-30 18:57:50 UTC
1) Yes, this is correct. containerOnlyFill(Throwable) is a straight-up Java 1.6 port of the Java 1.7 bootclassLoaderOnlyFill(Throwable) method. 

The 1.6 version could be simplified for production use in Tomcat. Please note, however, that the same simplification couldn't be applied to the 1.7 version, because the classes created by java.lang.invoke.MethodHandles can remain in memory for a long time. An array is used to prevent these classes from holding a direct reference to the Throwable and keeping it and its ClassLoader from being garbage-collected.

2) The intended use of this code is for the WebappClassLoader to clean up Exceptions that are created and held by third-party libraries included in a webapp when the webapp is unloaded. 

The 1.7 version is also intended to be usable from user code when the developer of the webapp is aware of one of their libraries having this issue. In this case the developer can use reflection to get the Exception object held by a library and call bootclassLoaderOnlyFill(Throwable) on it during initialization.
Comment 4 Konstantin Kolinko 2012-09-30 23:37:18 UTC
> MemoryLeakProtection in the Tomcat wiki says: "Sun bug 6916498 - An
> exception can keep a classloader in memory if the stack trace that was
> recorded when it was created contains a reference to one of its classes."

(In reply to comment #3)
> The intended use of this code is for the WebappClassLoader to clean up
> Exceptions that are created and held by third-party libraries included in a
> webapp when the webapp is unloaded. 
> 

For reference:

http://wiki.apache.org/tomcat/MemoryLeakProtection
https://issues.apache.org/bugzilla/show_bug.cgi?id=50460

1. First, I think that there will not be a leak in your scenario.

Tomcat already has code to clear static fields in classes that belong to a web application.  Note though that it is possible only because those classes are loaded through Tomcat's own WebappClassLoader class. It holds references to all classes that it loaded.


The scenario where I observed this memory leak issue was different. See bug 50460 for details. The Exception instance was being hold by a shared library, not by webapp one:

1) A webapp does its first call to a shared library or to Tomcat server code.
2) A class from the shared library is loaded and creates an Exception instance.
3) That exception holds reference to the calling webapp class.


2. If a library caches an Exception instance and has such a leak, it is a bug in that library. It is up to the authors of that library to fix their bug, e.g. by implementing fillInStackTrace() as a NOOP, like we did in ImmutableNameNotFoundException.

3. Each "class scanning" solution takes noticeable time, because usually there are a lot of classes. It is not worth here.

If there is no way to fix a library, one could implement a "targeted" solution: write a listener to perform cleanup in their specific case.

4. If the leak occurs in a shared library, I see no sane way to enumerate loaded classes (and thus to apply your solution). A good news is that there is an alternative solution: you can preload those classes.

A list of classes to preload is already configurable on JreMemoryLeakPreventionListener.


Thus I am closing this issue as WONTFIX.
Comment 5 Mark Thomas 2012-10-01 08:02:24 UTC
*** Bug 53935 has been marked as a duplicate of this bug. ***