tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From bugzi...@apache.org
Subject DO NOT REPLY [Bug 51195] "Find leaks" reports a false positive memory/classloader leak
Date Tue, 24 May 2011 14:35:17 GMT
https://issues.apache.org/bugzilla/show_bug.cgi?id=51195

--- Comment #8 from Joern Huxhorn <jhuxhorn@googlemail.com> 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.

-- 
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message