Bug 51195 - "Find leaks" reports a false positive memory/classloader leak
Summary: "Find leaks" reports a false positive memory/classloader leak
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 7
Classification: Unclassified
Component: Manager (show other bugs)
Version: 7.0.12
Hardware: PC All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-13 11:03 UTC by Joern Huxhorn
Modified: 2018-03-15 19:24 UTC (History)
0 users



Attachments
Implements the suggested change in StandardHost (4.46 KB, patch)
2011-05-19 17:19 UTC, Joern Huxhorn
Details | Diff
test (253 bytes, application/html)
2012-08-21 14:15 UTC, Larry G. Rapp
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joern Huxhorn 2011-05-13 11:03:24 UTC
If a webapp is using serialization of classes loaded by the webapp classloader then the "Find leaks" button will report a leak after undeploy of the webapp.
This is only partially true since the references that are still kept are only SoftReference instances.

This can be circumvented by adding code comparable to the changes in this commit https://github.com/huxi/logback/commit/d53e970963f84692889d438dd8a23c96137c15f2 prior to executing the garbage collection during "Find leaks".

I'd suggest to add such code to prevent the false positive warning.

See also http://jira.qos.ch/browse/LBCORE-205
Comment 1 Christopher Schultz 2011-05-16 21:54:00 UTC
The patch as written seems a bit over-reaching, possibly non-threadsafe and might even be JVM-specific.

It's over-reaching in that it purges the ObjectStreamClass's cache unconditionally which may result in a slight performance degradation for other threads as they re-populate the cache. I suspect performance is somewhat less of a concern for the thread taking the webapp out of service, so I think there's a reasonable amount of room to only clear those classes from the cache that came from the doomed webapp's ClassLoader.

From a thread-safety point of view, I'm not sure it's safe at all to clear this cache without doing a lot of checking of the type and/or method signatures, etc. This is also tied to the fact that this is a private class that may or may not exist outside of Sun/Oracle's implementation, or even in different versions of the JVM... if it's implementation-specific, we need a reliable way to test for the safety of this technique.

Obviously reflection can be used to see if the class and it's fields actually exist, but I'm skeptical about ensuring the thread-safety of the implementation.

Is it not enough to call System.gc(), here?
Comment 2 Joern Huxhorn 2011-05-17 00:17:23 UTC
Calling System.gc() is not enough at all since the caches in question contain SoftReferences that are not collected until the VM is running out of memory. This is the root of this whole issue. The references are not released, thus causing the leak-warning.

System.gc() is already executed during/before "Find leaks" if I'm not entirely mistaken.

The performance degradation is absolutely irrelevant since I'd like to see this code executed on click of the "Find leaks" button and not in case of every undeploy.
This isn't something you'd do in a production environment.
Clicking that button will result in a certain amount of performance degradation caused by the System.gc(), anyway.

The patch itself would only be non-threadsafe if
a) the private class java.io.ObjectStreamClass$Caches exists and
b) the implementation contains both fields and
c) the fields in question are not instances of ConcurrentMap but only Map

c) is not taken care of at the moment but this could easily be circumvented by changing the if condition to if(value instanceof ConcurrentMap) or even if(value instanceof ConcurrentHashMap).

Calling clear() on a ConcurrentHashMap is safe according to the documentation.
It won't interfere with any retrieval or update operations.
It's just not guaranteed, that the map is really empty with 100% certainty.

The code does nothing at all if a) or b) are not fulfilled.
Comment 3 Mark Thomas 2011-05-18 17:38:13 UTC
The code referenced above from logback is not licensed in a way that allows it to be used in an ALv2 work in source form.

The gc leaks functionality already includes a warning that it may report false positives and that a profiler should be used to determine if there really is a leak.

I'd be happy to review an appropriate licensed proposed patch.

Without a proposed patch and given the above I am leaning towards closing this as WONTFIX.
Comment 4 Joern Huxhorn 2011-05-19 17:19:01 UTC
Created attachment 27038 [details]
Implements the suggested change in StandardHost

This patch against trunk implements the suggested changes.
The code is purely my own work, apply any license you like.

Unfortunately, I was unable to perform an "ant release" build due to unrelated issues so I couldn't test this myself.

I got the following build error:
compile-non-log4j:
    [javac] [snip]/tomcat-trunk/output/extras/logging/commons-logging-1.1.1-src/build2.xml:359: warning: 'includeantruntime' was not set, defaulting to build.sysclasspath=last; set to false for repeatable builds
    [javac] Compiling 11 source files to [snip]/tomcat-trunk/output/extras/logging/commons-logging-1.1.1-src/target/classes
    [javac] [snip]/tomcat-trunk/output/extras/logging/commons-logging-1.1.1-src/src/java/org/apache/juli/logging/impl/ServletContextCleaner.java:24: package javax.servlet does not exist
    [javac] import javax.servlet.ServletContextEvent;
    [javac]                     ^
    [javac] [snip]/tomcat-trunk/output/extras/logging/commons-logging-1.1.1-src/src/java/org/apache/juli/logging/impl/ServletContextCleaner.java:25: package javax.servlet does not exist
    [javac] import javax.servlet.ServletContextListener;
    [javac]                     ^
    [javac] [snip]/tomcat-trunk/output/extras/logging/commons-logging-1.1.1-src/src/java/org/apache/juli/logging/impl/ServletContextCleaner.java:52: cannot find symbol
    [javac] symbol: class ServletContextListener
    [javac] public class ServletContextCleaner implements ServletContextListener {
    [javac]                                               ^
    [javac] [snip]/tomcat-trunk/output/extras/logging/commons-logging-1.1.1-src/src/java/org/apache/juli/logging/impl/ServletContextCleaner.java:61: cannot find symbol
    [javac] symbol  : class ServletContextEvent
    [javac] location: class org.apache.juli.logging.impl.ServletContextCleaner
    [javac]     public void contextDestroyed(ServletContextEvent sce) {
    [javac]                                  ^
    [javac] [snip]/tomcat-trunk/output/extras/logging/commons-logging-1.1.1-src/src/java/org/apache/juli/logging/impl/ServletContextCleaner.java:135: cannot find symbol
    [javac] symbol  : class ServletContextEvent
    [javac] location: class org.apache.juli.logging.impl.ServletContextCleaner
    [javac]     public void contextInitialized(ServletContextEvent sce) {
    [javac]                                    ^
    [javac] 5 errors

BUILD FAILED
Comment 5 Mark Thomas 2011-05-20 18:58:43 UTC
A couple of comments on the proposed patch.

1. This looks like clean-up code and as such is better placed in the WebappClassLoader as another ClearReferencesXXX method.

2. Rather than clearing the entire map, it would be better to iterate over the map and remove only those entries that had been loaded by the class WebappClassLoader currently being stopped.

In terms of checking the build, "ant" should suffice rather than the full "ant release". You;ll still get a working Tomcat instance you can test in output/build.
Comment 6 Joern Huxhorn 2011-05-24 11:09:57 UTC
I disagree.

The cleanup performed in the code is merely to prevent a *false* memory leak warning. A real leak does, in fact, not exist. This is the reason why I put it directly before the execution of System.gc() in the "Find Leaks" function.

Since "Find Leaks" isn't supposed to be executed in a production environment, I don't think that any further optimization (suggestion in 2.) makes sense. It would probably make sense if this code would indeed be added to the normal undeploy procedure but I really don't think that this is necessary or advisable.

The idea is to prevent false memory leak warnings so that real ones aren't ignored if they occur.
See http://en.wikipedia.org/wiki/The_Boy_Who_Cried_Wolf ;)
Comment 7 Christopher Schultz 2011-05-24 13:59:07 UTC
Joern,
(In reply to comment #6)
> The cleanup performed in the code is merely to prevent a *false* memory leak
> warning. A real leak does, in fact, not exist. This is the reason why I put it
> directly before the execution of System.gc() in the "Find Leaks" function.

One could argue that a memory leak does, in fact, exist: just because the references will be /eventually/ collected doesn't mean that they won't loiter for quite a long time... unless the GC is getting seriously low on memory, those objects will stick in memory, and so, likely, will the WebappClassLoader, and all of the java.lang.Class objects it manages.

I agree with Mark that this feature is best placed into WebappClassLoader and run during it's disposal sequence.

> Since "Find Leaks" isn't supposed to be executed in a production environment, I
> don't think that any further optimization (suggestion in 2.) makes sense.

If this will be used in WebappClassLoader, such optimizations should obviously be made.

> It
> would probably make sense if this code would indeed be added to the normal
> undeploy procedure but I really don't think that this is necessary or
> advisable.

This could be implemented as a utility method accessible to both pieces of code, and called directly during a "find leaks" operation... I would have to look at the "find leaks" feature to see if that would even be necessary... my sense is that the WCL is disposed before the leaks are detected, so I don't think any changes to the manager app would need to be made.
Comment 8 Joern Huxhorn 2011-05-24 14:35:17 UTC
(In reply to comment #7)
> Joern,
> (In reply to comment #6)
> > The cleanup performed in the code is merely to prevent a *false* memory leak
> > warning. A real leak does, in fact, not exist. This is the reason why I put it
> > directly before the execution of System.gc() in the "Find Leaks" function.
> 
> One could argue that a memory leak does, in fact, exist: just because the
> references will be /eventually/ collected doesn't mean that they won't loiter
> for quite a long time... unless the GC is getting seriously low on memory,
> those objects will stick in memory, and so, likely, will the WebappClassLoader,
> and all of the java.lang.Class objects it manages.
> 
> I agree with Mark that this feature is best placed into WebappClassLoader and
> run during it's disposal sequence.


Alright, I agree that it's better to get rid of a WebappClassLoader as soon as possible instead of waiting for a hard gc caused by low memory.


> > Since "Find Leaks" isn't supposed to be executed in a production environment, I
> > don't think that any further optimization (suggestion in 2.) makes sense.
> 
> If this will be used in WebappClassLoader, such optimizations should obviously
> be made.

It's actually pretty hard to implement those optimizations. The keys used in those maps are two static internal classes of ObjectStreamClass. 

FieldReflectorKey is private and WeakClassKey is package-private.

Performing a selective cleanup would involve serious complexity an reflection magic, including lots of assumptions about internal behavior in the hope that it won't change...
I'd leave it like it is right now to prevent errors caused by this.
My current implementation will simply do nothing if localDescs & reflectors are not available or not instances of class ConcurrentHashMap.

I don't think that the performance impact is a big thing but I haven't done any benchmarks, either. You should remember that this is exactly what would happen naturally if the memory of the JVM is running low. We are just getting rid of SoftReferences. This would be a problem if performed continuously but doing so once during undeploy shouldn't interfere with other webapps that much.

> > It
> > would probably make sense if this code would indeed be added to the normal
> > undeploy procedure but I really don't think that this is necessary or
> > advisable.
> 
> This could be implemented as a utility method accessible to both pieces of
> code, and called directly during a "find leaks" operation... I would have to
> look at the "find leaks" feature to see if that would even be necessary... my
> sense is that the WCL is disposed before the leaks are detected, so I don't
> think any changes to the manager app would need to be made.

I don't have enough know-how about the way Tomcat functionality is structured. So I just assume you are right. The manager app would likely stay untouched since "Find leaks" simply checks if the WebappClassLoader has been unloaded properly, if I remember correctly.
Comment 9 Sylvain Laurent 2011-05-24 20:17:19 UTC
just my 2cts : I'm leaning towards Joern point of view, that cleaning the ObjectStreamClass cache should be done during the Find Leaks action rather than when stopping the webapp. This way we do not impact other webapps when stopping one.
Comment 10 Konstantin Kolinko 2011-07-26 11:19:08 UTC
I think there might be additional options for the findleaks command.

E.g. make calling gc optional, or perform additional cleanup as described here.
Comment 11 Larry G. Rapp 2012-08-21 14:15:20 UTC
Created attachment 29258 [details]
test
Comment 12 Mark Thomas 2018-03-12 20:50:25 UTC
I'd argue that this is a JRE bug. Given the possibility of a memory leak, a method should be provided to enable the cache to be cleared for a given class loader.

I'm working on extending https://github.com/markt-asf/memory-leaks to add a demonstration of this leak and to test some potential clean-up code. Once I have a working test case, I'll open a JRE bug and ping our helpful contacts at Oracle.
Comment 13 Mark Thomas 2018-03-13 14:29:04 UTC
On reflection, I don't think this is a JRE bug but an enhancement request. I have submitted the RFE and been given ID 9052974.
Comment 14 Mark Thomas 2018-03-13 23:38:33 UTC
I've extended the clearReferencesXXX code in the web application class loader to remove web application classes from ObjectStreamClass$Caches. Yes, it involves reflection of internal classes but no more than Tomcat was already doing for similar issues.

If the enhancement is accepted, this code can be simplified.

Fixed in:
- trunk for 9.0.7 onwards
- 8.5.x for 8.5.30 onwards
- 8.0.x for 8.0.51 onwards
- 7.0.x for 7.0.86 onwards
Comment 16 Christopher Schultz 2018-03-15 19:24:16 UTC
(In reply to Mark Thomas from comment #13)
> On reflection

I see what you did, there. ;)