[ https://issues.apache.org/jira/browse/POOL-315?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15597287#comment-15597287 ]
Buğra Gedik edited comment on POOL-315 at 10/22/16 7:13 PM: ------------------------------------------------------------ That sounds good as a direction. How about modeling it after Java {{ExecutorService}}'s {{terminate()}}/{{awaitTermination()}}?. Perhaps it could be {{close()}} (with no change to behavior) and {waitForClose()}}. was (Author: bgedik): That sounds good as a direction. How about modeling it after Java {{ExecutorService}}'s {{terminate()}}/{{awaitTermination()}}?. Perhaps it could be {{close()}} (with no change to behavior) and {{awaitToClose()}}. > GenericObjectPool close() does not wait for the current eviction task > --------------------------------------------------------------------- > > Key: POOL-315 > URL: https://issues.apache.org/jira/browse/POOL-315 > Project: Commons Pool > Issue Type: Bug > Reporter: Buğra Gedik > > The {{close}} method is implemented as follows: > {code} > public void close() { > if(!this.isClosed()) { > Object var1 = this.closeLock; > synchronized(this.closeLock) { > if(!this.isClosed()) { > this.startEvictor(-1L); > this.closed = true; > this.clear(); > this.jmxUnregister(); > this.idleObjects.interuptTakeWaiters(); > } > } > } > } > {code} > The line {{this.startEvictor(-1L);}} calls > {{EvictionTimer.cancel(this.evictor);}} from {{BaseGenericObjectPool}} and > that in turn calls the following method in {{EvictionTimer}}: > {code} > static synchronized void cancel(TimerTask task) { > task.cancel(); > --_usageCount; > if(_usageCount == 0) { > _timer.cancel(); > _timer = null; > } > } > {code} > Here, {{_timer}} is a {{java.util.TimerTask}}. If you look at the > documentation of it, you'll see that it does not block on the currently > executing task. Even though {{task.cancel()}} is called, there is no code to > wait for it to complete. The end result is that, even if you close the pool, > there may still be an eviction thread running. Yes, it will eventually go > away, but in some rare cases it takes a bit of time to go away. > When running code under Tomcat, this results in the > {{"commons-pool-EvictionTimer"}} thread (created in the class > {{EvictionTimer}}) to be reported as leaking, despite the pool being closed. > This happens rarely, since most of the time the timer thread goes away before > Tomcat checks for leaking threads. > In my opinion a fix can be put into {{startEvictor}} in > {{BaseGenericObjectPool}}: > {code} > final void startEvictor(long delay) { > synchronized (evictionLock) { > if (null != evictor) { > EvictionTimer.cancel(evictor); > // HERE: evictor.waitForCompletion(); > evictor = null; > evictionIterator = null; > } > if (delay > 0) { > evictor = new Evictor(); > EvictionTimer.schedule(evictor, delay, delay); > } > } > } > {code} > Here is an example that illustrates the problem: > {code:collapsible=false} > import static org.junit.Assert.assertFalse; > import org.apache.commons.pool2.PooledObject; > import org.apache.commons.pool2.PooledObjectFactory; > import org.apache.commons.pool2.impl.DefaultPooledObject; > import org.apache.commons.pool2.impl.GenericObjectPool; > import org.apache.commons.pool2.impl.GenericObjectPoolConfig; > import org.junit.Test; > public class PoolTest > { > private static final CharSequence > COMMONS_POOL_EVICTIONS_TIMER_THREAD_NAME = > "commons-pool-EvictionTimer"; > private static final long EVICTION_PERIOD_IN_MILLIS = 100; > private static class Foo > { > } > private static class PooledFooFactory implements PooledObjectFactory<Foo> > { > private static final long VALIDATION_WAIT_IN_MILLIS = 1000; > @Override > public PooledObject<Foo> makeObject() throws Exception > { > return new DefaultPooledObject<>(new Foo()); > } > @Override > public void destroyObject( > PooledObject<Foo> pooledObject) throws Exception > { > } > @Override > public boolean validateObject( > PooledObject<Foo> pooledObject) > { > try > { > Thread.sleep(VALIDATION_WAIT_IN_MILLIS); > } > catch (final InterruptedException e) > { > Thread.interrupted(); > } > return false; > } > @Override > public void activateObject( > PooledObject<Foo> pooledObject) throws Exception > { > } > @Override > public void passivateObject( > PooledObject<Foo> pooledObject) throws Exception > { > } > } > @Test > public void testPool() throws Exception > { > final GenericObjectPoolConfig poolConfig = > new GenericObjectPoolConfig(); > poolConfig.setTestWhileIdle(true /* testWhileIdle */); > final PooledFooFactory pooledFooFactory = new PooledFooFactory(); > GenericObjectPool<Foo> pool = null; > try > { > pool = new GenericObjectPool<>(pooledFooFactory, poolConfig); > pool.setTimeBetweenEvictionRunsMillis(EVICTION_PERIOD_IN_MILLIS); > pool.addObject(); > try > { > Thread.sleep(EVICTION_PERIOD_IN_MILLIS); > } > catch (final InterruptedException e) > { > Thread.interrupted(); > } > } > finally > { > if (pool != null) > { > pool.close(); > } > } > final Thread[] threads = new Thread[Thread.activeCount()]; > Thread.enumerate(threads); > for (final Thread thread : threads) > { > if (thread == null) > { > continue; > } > assertFalse(thread.getName() > .contains(COMMONS_POOL_EVICTIONS_TIMER_THREAD_NAME)); > } > } > } > {code} -- This message was sent by Atlassian JIRA (v6.3.4#6332)