[pool] Evictor thread

[Attila Szegedi <sz...@freemail.hu>]

Hi all,

I've just been through a particularly nasty case of debugging a memory  
leak where the evictor thread of GenericObjectPool (as used by DBCP) was  
not willing to die and let go of a disposed ClassLoader it held as the  
loader of a ProtectionDomain of its inherited SecurityContext. Now, while  
GenericObjectPool.close() was being called, the Thread running the Evictor  
code didn't observe the change in the _cancelled flag. This is  
unfortunately perfectly legal under the Java memory model given that there  
is neither a synchronization surrounding reading and writing of the flag,  
nor a volatile flag on the _cancelled flag.

So, I'd ask you to fix this by adding the "volatile" flag to the "boolean  
_cancelled" field of the  
org.apache.commons.pool.impl.GenericObjectPool$Evictor class.

Also, it'd be a big improvement in terms of scalability if the  
GenericObjectPool held a reference to the Thread running the Evictor  
object, and would interrupt() it on close(), so it doesn't take  
"timeBetweenEvictionRunsMillis" after a call to close() before the thread  
actually clears up.

(All this might not be visible in a more-or-less statical environment, but  
is a problem in a system that frequently reloads classes using new class  
loaders which are a subclass of java.security.SecureClassLoader and  
therefore "decorate" threads with security contexts that ultimately hold a  
reference to the class loader. Few loitering class loaders can quickly  
escalate into an OutOfMemoryError.)

Thanks,
   Attila


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

Re: [pool] Evictor thread

[Dirk Verbeeck <di...@pandora.be>]

Hi Attila,

You have made some good points, included you will find a new evictor 
implementation with the enhancements you proposed. I didn't want to 
use interrupt() so I used a wait/notify technique with synchronized 
blocks.

Can you try this version and let me know if it works for you?

Cheers
Dirk

=================================================================

     class Evictor implements Runnable {
         private boolean _cancelled = false;
         private long _delay = 0L;

         public Evictor(long delay) {
             _delay = delay;
         }

         synchronized void cancel() {
             _cancelled = true;
             notifyAll();
         }

         synchronized boolean isCancelled() {
         	return _cancelled;
         }

         private synchronized void sleep() {
         	if (_delay > 0) {
	        	long now = System.currentTimeMillis();
	        	long wakeup = now + _delay;
	        	while (!isCancelled() && (now < wakeup)) {
		        	try {
						wait(wakeup - now);
					} catch (InterruptedException e) {
	                    // ignored
					}
					now = System.currentTimeMillis();
	        	}
         	}
         }

         public void run() {
         	sleep();
             while(!isCancelled()) {
                 try {
                     evict();
                 } catch(Exception e) {
                     // ignored
                 }
                 try {
                     ensureMinIdle();
                 } catch(Exception e) {
                     // ignored
                 }
             	sleep();
             }
             synchronized(GenericObjectPool.this) {
                 if(null != _evictionCursor) {
                     _evictionCursor.close();
                     _evictionCursor = null;
                 }
             }
         }

     }


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