On 23/09/2014 20:22, Bernd Eckenfels wrote: > Hello Mark, > > I am not shure about the weak reference to the classloader. What is the > idea behind asuming that the pool is dereferenced when the class loader > goes?
Dealing with the problem that the JavaEE specification provides no standard mechanism for telling a resource to clean itself up when it is no longer used. In short - fixing a memory leak when DBCP was used in Tomcat. The longer version is that when DBCP was used the following chain of references was set up: EvictorThread -> Evictor -> BaseGenericObjectPool.factoryClassLoader -> webapplication class loader -> every class loaded by the web application When a JavaEE application stops, the reference to the resource is just dropped. The problem is that DBCP/Pool has no way of stopping the Evictor thread at this point so it just keeps running, retaining a reference to the web application class loader, pinning all the web app's classes in memory and leaking a big chunk of PermGen. In a JavaEE environment when the weak reference to the web application class loader clears, that means that the web application has been stopped and that it is safe to stop the Evictor thread as the pool will already have been dereferenced. > I can imagine there are cases where the class loader for the > factory is no longer used (as it was not used to load the factory for > example) and the pool is still referenced. What sort of cases? If the pool was started with an explicit thread context class loader then expecting the pool to continue working after that class loader has stopped is not reasonable. > Would this lead to a unexpected cancel()? I don't think it is likely but I would argue that such a scenario would be a bug in the code using Pool. If a TCCL was present when the pool was started that the developer did not want to use, I'd expect them to take steps to start the pool under a different TCCL - much as Pool does inside the Evictor thread. Mark > > Gruss > Bernd > > > Am Tue, 23 Sep 2014 13:04:53 -0000 > schrieb ma...@apache.org: > >> Author: markt >> Date: Tue Sep 23 13:04:52 2014 >> New Revision: 1626998 >> >> URL: http://svn.apache.org/r1626998 >> Log: >> Prevent potential memory leaks when the Pool is dereferenced without >> being closed. >> >> Modified: >> commons/proper/pool/trunk/src/changes/changes.xml >> >> commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java >> >> Modified: commons/proper/pool/trunk/src/changes/changes.xml >> URL: >> http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/changes/changes.xml?rev=1626998&r1=1626997&r2=1626998&view=diff >> ============================================================================== >> --- commons/proper/pool/trunk/src/changes/changes.xml (original) +++ >> commons/proper/pool/trunk/src/changes/changes.xml Tue Sep 23 13:04:52 >> 2014 @@ -80,6 +80,10 @@ The <action> type attribute can be add,u >> <action dev="ggregory" type="update" issue="POOL-274"> Update >> asm-util to 5.0.3 from 4.0. </action> >> + <action dev="markt" type="fix"> >> + Prevent potential memory leaks when the Pool is dereferenced >> without being >> + closed. >> + </action> >> </release> >> <release version="2.2" date="2014-02-24" description= >> "This is a maintenance release that adds a new testOnCreate >> configuration option >> >> Modified: >> commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java >> URL: >> http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java?rev=1626998&r1=1626997&r2=1626998&view=diff >> ============================================================================== >> --- >> commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java >> (original) +++ >> commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java >> Tue Sep 23 13:04:52 2014 @@ -20,6 +20,7 @@ import >> java.io.PrintWriter; import java.io.StringWriter; import >> java.io.Writer; import java.lang.management.ManagementFactory; >> +import java.lang.ref.WeakReference; import java.util.Iterator; >> import java.util.TimerTask; import >> java.util.concurrent.atomic.AtomicLong; @@ -94,9 +95,10 @@ public >> abstract class BaseGenericObjectP /* >> * Class loader for evictor thread to use since in a J2EE or >> similar >> * environment the context class loader for the evictor thread >> may have >> - * visibility of the correct factory. See POOL-161. >> + * visibility of the correct factory. See POOL-161. Uses a weak >> reference to >> + * avoid potential memory leaks if the Pool is discarded rather >> than closed. */ >> - private final ClassLoader factoryClassLoader; >> + private final WeakReference<ClassLoader> factoryClassLoader; >> >> >> // Monitoring (primarily JMX) attributes >> @@ -137,7 +139,8 @@ public abstract class BaseGenericObjectP >> this.creationStackTrace = getStackTrace(new Exception()); >> >> // save the current CCL to be used later by the evictor >> Thread >> - factoryClassLoader = >> Thread.currentThread().getContextClassLoader(); >> + factoryClassLoader = new WeakReference<ClassLoader>( >> + Thread.currentThread().getContextClassLoader()); >> fairness = config.getFairness(); >> } >> >> @@ -251,7 +254,7 @@ public abstract class BaseGenericObjectP >> public final boolean getLifo() { >> return lifo; >> } >> - >> + >> /** >> * Returns whether or not the pool serves threads waiting to >> borrow objects fairly. >> * True means that waiting threads are served as if waiting in a >> FIFO queue. @@ -997,8 +1000,14 @@ public abstract class >> BaseGenericObjectP Thread.currentThread().getContextClassLoader(); >> try { >> // Set the class loader for the factory >> - Thread.currentThread().setContextClassLoader( >> - factoryClassLoader); >> + ClassLoader cl = factoryClassLoader.get(); >> + if (cl == null) { >> + // The pool has been dereferenced and the class >> loader GC'd. >> + // Cancel this timer so the pool can be GC'd as >> well. >> + cancel(); >> + return; >> + } >> + Thread.currentThread().setContextClassLoader(cl); >> >> // Evict from the pool >> try { >> >> > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
