Fix POOL-351 Add a configurable delay (default 10 seconds) to wait when shutting down an Evictor to allow the associated thread time to complete and current evictions and to terminate.
git-svn-id: https://svn.apache.org/repos/asf/commons/proper/pool/trunk@1767782 13f79535-47bb-0310-9956-ffa450edef68 Project: http://git-wip-us.apache.org/repos/asf/commons-pool/repo Commit: http://git-wip-us.apache.org/repos/asf/commons-pool/commit/4a20cdca Tree: http://git-wip-us.apache.org/repos/asf/commons-pool/tree/4a20cdca Diff: http://git-wip-us.apache.org/repos/asf/commons-pool/diff/4a20cdca Branch: refs/heads/master Commit: 4a20cdca923bd342360f821d7020538e985d9ec2 Parents: b15bc63 Author: Mark Thomas <ma...@apache.org> Authored: Wed Nov 2 20:53:11 2016 +0000 Committer: Mark Thomas <ma...@apache.org> Committed: Wed Nov 2 20:53:11 2016 +0000 ---------------------------------------------------------------------- src/changes/changes.xml | 5 + .../pool2/impl/BaseGenericObjectPool.java | 30 +++- .../pool2/impl/BaseObjectPoolConfig.java | 48 ++++++- .../commons/pool2/impl/EvictionTimer.java | 144 +++++++------------ .../pool2/impl/GenericKeyedObjectPool.java | 1 + .../commons/pool2/impl/GenericObjectPool.java | 1 + .../pool2/impl/TestGenericObjectPool.java | 1 + 7 files changed, 132 insertions(+), 98 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/commons-pool/blob/4a20cdca/src/changes/changes.xml ---------------------------------------------------------------------- diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 5cca806..dca51b3 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -71,6 +71,11 @@ The <action> type attribute can be add,update,fix,remove. Ensure that any class name used for evictionPolicyClassName represents a class that implements EvictionPolicy. </action> + <action dev="markt" issue="POOL-351" type="fix"> + Add a configurable delay (default 10 seconds) to wait when shutting down + an Evictor to allow the associated thread time to complete and current + evictions and to terminate. + </action> </release> <release version="2.4.2" date="2015-08-01" description= "This is a patch release, including bug fixes only."> http://git-wip-us.apache.org/repos/asf/commons-pool/blob/4a20cdca/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java b/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java index 8afa8f1..1f46811 100644 --- a/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java +++ b/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java @@ -25,6 +25,7 @@ import java.util.Arrays; import java.util.Deque; import java.util.Iterator; import java.util.TimerTask; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; import javax.management.InstanceAlreadyExistsException; @@ -87,6 +88,8 @@ public abstract class BaseGenericObjectPool<T> extends BaseObject { private volatile long softMinEvictableIdleTimeMillis = BaseObjectPoolConfig.DEFAULT_SOFT_MIN_EVICTABLE_IDLE_TIME_MILLIS; private volatile EvictionPolicy<T> evictionPolicy; + private long evictorShutdownTimeoutMillis = + BaseObjectPoolConfig.DEFAULT_EVICTOR_SHUTDOWN_TIMEOUT_MILLIS; // Internal (primarily state) attributes @@ -632,6 +635,31 @@ public abstract class BaseGenericObjectPool<T> extends BaseObject { } } + /** + * Gets the timeout that will be used when waiting for the Evictor to + * shutdown if this pool is closed and it is the only pool still using the + * the value for the Evictor. + * + * @return The timeout in milliseconds that will be used while waiting for + * the Evictor to shut down. + */ + public long getEvictorShutdownTimeoutMillis() { + return evictorShutdownTimeoutMillis; + } + + /** + * Sets the timeout that will be used when waiting for the Evictor to + * shutdown if this pool is closed and it is the only pool still using the + * the value for the Evictor. + * + * @param evictorShutdownTimeoutMillis the timeout in milliseconds that + * will be used while waiting for the + * Evictor to shut down. + */ + public void setEvictorShutdownTimeoutMillis( + final long evictorShutdownTimeoutMillis) { + this.evictorShutdownTimeoutMillis = evictorShutdownTimeoutMillis; + } /** * Closes the pool, destroys the remaining idle objects and, if registered @@ -692,7 +720,7 @@ public abstract class BaseGenericObjectPool<T> extends BaseObject { final void startEvictor(final long delay) { synchronized (evictionLock) { if (null != evictor) { - EvictionTimer.cancel(evictor); + EvictionTimer.cancel(evictor, 10, TimeUnit.SECONDS); evictor = null; evictionIterator = null; } http://git-wip-us.apache.org/repos/asf/commons-pool/blob/4a20cdca/src/main/java/org/apache/commons/pool2/impl/BaseObjectPoolConfig.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/commons/pool2/impl/BaseObjectPoolConfig.java b/src/main/java/org/apache/commons/pool2/impl/BaseObjectPoolConfig.java index 2f1a595..d635227 100644 --- a/src/main/java/org/apache/commons/pool2/impl/BaseObjectPoolConfig.java +++ b/src/main/java/org/apache/commons/pool2/impl/BaseObjectPoolConfig.java @@ -70,6 +70,15 @@ public abstract class BaseObjectPoolConfig extends BaseObject implements Cloneab public static final long DEFAULT_SOFT_MIN_EVICTABLE_IDLE_TIME_MILLIS = -1; /** + * The default value for {@code evictorShutdownTimeoutMillis} configuration + * attribute. + * @see GenericObjectPool#getEvictorShutdownTimeoutMillis() + * @see GenericKeyedObjectPool#getEvictorShutdownTimeoutMillis() + */ + public static final long DEFAULT_EVICTOR_SHUTDOWN_TIMEOUT_MILLIS = + 10L * 1000L; + + /** * The default value for the {@code numTestsPerEvictionRun} configuration * attribute. * @see GenericObjectPool#getNumTestsPerEvictionRun() @@ -163,13 +172,16 @@ public abstract class BaseObjectPoolConfig extends BaseObject implements Cloneab private long maxWaitMillis = DEFAULT_MAX_WAIT_MILLIS; private long minEvictableIdleTimeMillis = - DEFAULT_MIN_EVICTABLE_IDLE_TIME_MILLIS; + DEFAULT_MIN_EVICTABLE_IDLE_TIME_MILLIS; + + private long evictorShutdownTimeoutMillis = + DEFAULT_EVICTOR_SHUTDOWN_TIMEOUT_MILLIS; private long softMinEvictableIdleTimeMillis = DEFAULT_MIN_EVICTABLE_IDLE_TIME_MILLIS; private int numTestsPerEvictionRun = - DEFAULT_NUM_TESTS_PER_EVICTION_RUN; + DEFAULT_NUM_TESTS_PER_EVICTION_RUN; private String evictionPolicyClassName = DEFAULT_EVICTION_POLICY_CLASS_NAME; @@ -182,7 +194,7 @@ public abstract class BaseObjectPoolConfig extends BaseObject implements Cloneab private boolean testWhileIdle = DEFAULT_TEST_WHILE_IDLE; private long timeBetweenEvictionRunsMillis = - DEFAULT_TIME_BETWEEN_EVICTION_RUNS_MILLIS; + DEFAULT_TIME_BETWEEN_EVICTION_RUNS_MILLIS; private boolean blockWhenExhausted = DEFAULT_BLOCK_WHEN_EXHAUSTED; @@ -367,6 +379,36 @@ public abstract class BaseObjectPoolConfig extends BaseObject implements Cloneab } /** + * Get the value for the {@code evictorShutdownTimeoutMillis} configuration + * attribute for pools created with this configuration instance. + * + * @return The current setting of {@code evictorShutdownTimeoutMillis} for + * this configuration instance + * + * @see GenericObjectPool#getEvictorShutdownTimeoutMillis() + * @see GenericKeyedObjectPool#getEvictorShutdownTimeoutMillis() + */ + public long getEvictorShutdownTimeoutMillis() { + return evictorShutdownTimeoutMillis; + } + + /** + * Set the value for the {@code evictorShutdownTimeoutMillis} configuration + * attribute for pools created with this configuration instance. + * + * @param evictorShutdownTimeoutMillis The new setting of + * {@code evictorShutdownTimeoutMillis} for this configuration + * instance + * + * @see GenericObjectPool#getEvictorShutdownTimeoutMillis() + * @see GenericKeyedObjectPool#getEvictorShutdownTimeoutMillis() + */ + public void setEvictorShutdownTimeoutMillis( + final long evictorShutdownTimeoutMillis) { + this.evictorShutdownTimeoutMillis = evictorShutdownTimeoutMillis; + } + + /** * Get the value for the {@code testOnCreate} configuration attribute for * pools created with this configuration instance. * http://git-wip-us.apache.org/repos/asf/commons-pool/blob/4a20cdca/src/main/java/org/apache/commons/pool2/impl/EvictionTimer.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/commons/pool2/impl/EvictionTimer.java b/src/main/java/org/apache/commons/pool2/impl/EvictionTimer.java index 191dc86..b448141 100644 --- a/src/main/java/org/apache/commons/pool2/impl/EvictionTimer.java +++ b/src/main/java/org/apache/commons/pool2/impl/EvictionTimer.java @@ -18,16 +18,19 @@ package org.apache.commons.pool2.impl; import java.security.AccessController; import java.security.PrivilegedAction; -import java.util.Timer; import java.util.TimerTask; +import java.util.concurrent.ScheduledThreadPoolExecutor; +import java.util.concurrent.ThreadFactory; +import java.util.concurrent.TimeUnit; /** - * Provides a shared idle object eviction timer for all pools. This class wraps - * the standard {@link Timer} and keeps track of how many pools are using it. - * If no pools are using the timer, it is canceled. This prevents a thread - * being left running which, in application server environments, can lead to - * memory leads and/or prevent applications from shutting down or reloading - * cleanly. + * Provides a shared idle object eviction timer for all pools. This class is + * currently implemented using {@link ScheduledThreadPoolExecutor}. This + * implementation may change in any future release. This class keeps track of + * how many pools are using it. If no pools are using the timer, it is canceled. + * This prevents a thread being left running which, in application server + * environments, can lead to memory leads and/or prevent applications from + * shutting down or reloading cleanly. * <p> * This class has package scope to prevent its inclusion in the pool public API. * The class declaration below should *not* be changed to public. @@ -38,11 +41,11 @@ import java.util.TimerTask; */ class EvictionTimer { - /** Timer instance */ - private static Timer _timer; //@GuardedBy("EvictionTimer.class") + /** Executor instance */ + private static ScheduledThreadPoolExecutor executor; //@GuardedBy("EvictionTimer.class") /** Static usage count tracker */ - private static int _usageCount; //@GuardedBy("EvictionTimer.class") + private static int usageCount; //@GuardedBy("EvictionTimer.class") /** Prevent instantiation */ private EvictionTimer() { @@ -70,102 +73,55 @@ class EvictionTimer { * @param delay Delay in milliseconds before task is executed * @param period Time in milliseconds between executions */ - static synchronized void schedule(final TimerTask task, final long delay, final long period) { - if (null == _timer) { - // Force the new Timer thread to be created with a context class - // loader set to the class loader that loaded this library - final ClassLoader ccl = AccessController.doPrivileged( - new PrivilegedGetTccl()); - try { - AccessController.doPrivileged(new PrivilegedSetTccl( - EvictionTimer.class.getClassLoader())); - _timer = AccessController.doPrivileged(new PrivilegedNewEvictionTimer()); - } finally { - AccessController.doPrivileged(new PrivilegedSetTccl(ccl)); - } + static synchronized void schedule(final Runnable task, final long delay, final long period) { + if (null == executor) { + executor = new ScheduledThreadPoolExecutor(1, new EvictorThreadFactory()); } - _usageCount++; - _timer.schedule(task, delay, period); + usageCount++; + executor.scheduleWithFixedDelay(task, delay, period, TimeUnit.MILLISECONDS); } /** * Remove the specified eviction task from the timer. - * @param task Task to be scheduled + * + * @param task Task to be cancelled + * @param timeout If the associated executor is no longer required, how + * long should this thread wait for the executor to + * terminate? + * @param unit The units for the specified timeout */ - static synchronized void cancel(final TimerTask task) { + static synchronized void cancel(final TimerTask task, long timeout, TimeUnit unit) { task.cancel(); - _usageCount--; - if (_usageCount == 0) { - _timer.cancel(); - _timer = null; - } - } - - /** - * {@link PrivilegedAction} used to get the ContextClassLoader - */ - private static class PrivilegedGetTccl implements PrivilegedAction<ClassLoader> { - - /** - * {@inheritDoc} - */ - @Override - public ClassLoader run() { - return Thread.currentThread().getContextClassLoader(); - } - } - - /** - * {@link PrivilegedAction} used to set the ContextClassLoader - */ - private static class PrivilegedSetTccl implements PrivilegedAction<Void> { - - /** ClassLoader */ - private final ClassLoader classLoader; - - /** - * Create a new PrivilegedSetTccl using the given classloader - * @param classLoader ClassLoader to use - */ - PrivilegedSetTccl(final ClassLoader cl) { - this.classLoader = cl; - } - - /** - * {@inheritDoc} - */ - @Override - public Void run() { - Thread.currentThread().setContextClassLoader(classLoader); - return null; - } - - @Override - public String toString() { - final StringBuilder builder = new StringBuilder(); - builder.append("PrivilegedSetTccl [classLoader="); - builder.append(classLoader); - builder.append("]"); - return builder.toString(); + usageCount--; + if (usageCount == 0) { + executor.shutdown(); + try { + executor.awaitTermination(timeout, unit); + } catch (InterruptedException e) { + // Swallow + // Significant API changes would be required to propagate this + } + executor.setCorePoolSize(0); + executor = null; } } - /** - * {@link PrivilegedAction} used to create a new Timer. Creating the timer - * with a privileged action means the associated Thread does not inherit the - * current access control context. In a container environment, inheriting - * the current access control context is likely to result in retaining a - * reference to the thread context class loader which would be a memory - * leak. - */ - private static class PrivilegedNewEvictionTimer implements PrivilegedAction<Timer> { + private static class EvictorThreadFactory implements ThreadFactory { - /** - * {@inheritDoc} - */ @Override - public Timer run() { - return new Timer("commons-pool-EvictionTimer", true); + public Thread newThread(final Runnable r) { + final Thread t = new Thread(null, r, "commons-pool-evictor-thrreads"); + t.setName("commons-pool-evictor"); + + AccessController.doPrivileged(new PrivilegedAction<Void>() { + @Override + public Void run() { + t.setContextClassLoader(EvictorThreadFactory.class.getClassLoader()); + return null; + } + }); + + return t; } } } http://git-wip-us.apache.org/repos/asf/commons-pool/blob/4a20cdca/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java b/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java index 6976f2a..7fa21b5 100644 --- a/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java +++ b/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java @@ -255,6 +255,7 @@ public class GenericKeyedObjectPool<K,T> extends BaseGenericObjectPool<T> setTimeBetweenEvictionRunsMillis( conf.getTimeBetweenEvictionRunsMillis()); setEvictionPolicyClassName(conf.getEvictionPolicyClassName()); + setEvictorShutdownTimeoutMillis(conf.getEvictorShutdownTimeoutMillis()); } /** http://git-wip-us.apache.org/repos/asf/commons-pool/blob/4a20cdca/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java b/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java index 487eec2..be0f508 100644 --- a/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java +++ b/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java @@ -318,6 +318,7 @@ public class GenericObjectPool<T> extends BaseGenericObjectPool<T> setSoftMinEvictableIdleTimeMillis( conf.getSoftMinEvictableIdleTimeMillis()); setEvictionPolicyClassName(conf.getEvictionPolicyClassName()); + setEvictorShutdownTimeoutMillis(conf.getEvictorShutdownTimeoutMillis()); } /** http://git-wip-us.apache.org/repos/asf/commons-pool/blob/4a20cdca/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java b/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java index c9014ac..838aab4 100644 --- a/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java +++ b/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java @@ -1705,6 +1705,7 @@ public class TestGenericObjectPool extends TestBaseObjectPool { assertEquals("maxWait",expected.getMaxWaitMillis(),actual.getMaxWaitMillis()); assertEquals("minEvictableIdleTimeMillis",expected.getMinEvictableIdleTimeMillis(),actual.getMinEvictableIdleTimeMillis()); assertEquals("numTestsPerEvictionRun",expected.getNumTestsPerEvictionRun(),actual.getNumTestsPerEvictionRun()); + assertEquals("evictorShutdownTimeoutMillis",expected.getEvictorShutdownTimeoutMillis(),actual.getEvictorShutdownTimeoutMillis()); assertEquals("timeBetweenEvictionRunsMillis",expected.getTimeBetweenEvictionRunsMillis(),actual.getTimeBetweenEvictionRunsMillis()); }
