This is an automated email from the ASF dual-hosted git repository. markt pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/master by this push: new d676a1e Update internal fork of Apache Commons Pool 2 d676a1e is described below commit d676a1eb64c3d3c93abe90daa3226bfa448f68ec Author: Mark Thomas <ma...@apache.org> AuthorDate: Fri May 3 16:57:22 2019 +0100 Update internal fork of Apache Commons Pool 2 --- MERGE.txt | 2 +- java/org/apache/tomcat/dbcp/pool2/PoolUtils.java | 47 +++++++++++++--------- .../dbcp/pool2/impl/BaseGenericObjectPool.java | 8 ++-- .../tomcat/dbcp/pool2/impl/CallStackUtils.java | 8 +--- .../tomcat/dbcp/pool2/impl/EvictionTimer.java | 35 +++++++++------- .../dbcp/pool2/impl/GenericKeyedObjectPool.java | 25 +++++++----- .../tomcat/dbcp/pool2/impl/GenericObjectPool.java | 26 ++++++++++++ webapps/docs/changelog.xml | 6 ++- 8 files changed, 97 insertions(+), 60 deletions(-) diff --git a/MERGE.txt b/MERGE.txt index e28ca94..7d24a98 100644 --- a/MERGE.txt +++ b/MERGE.txt @@ -69,4 +69,4 @@ Pool2 Sub-tree src/main/java/org/apache/commons/pool2 The SHA1 ID for the most recent commit to be merged to Tomcat is: -d4e0e88227ad91d8c8ef36ba01d656f71c770f83 +0664f4dac9ef653703624cbe67272134cf0151cb (2019-04-30) diff --git a/java/org/apache/tomcat/dbcp/pool2/PoolUtils.java b/java/org/apache/tomcat/dbcp/pool2/PoolUtils.java index 4fb0aba..2494351 100644 --- a/java/org/apache/tomcat/dbcp/pool2/PoolUtils.java +++ b/java/org/apache/tomcat/dbcp/pool2/PoolUtils.java @@ -36,6 +36,13 @@ import java.util.concurrent.locks.ReentrantReadWriteLock.WriteLock; */ public final class PoolUtils { + private static final String MSG_FACTOR_NEGATIVE = "factor must be positive."; + private static final String MSG_MIN_IDLE = "minIdle must be non-negative."; + private static final String MSG_NULL_KEY = "key must not be null."; + private static final String MSG_NULL_KEYED_POOL = "keyedPool must not be null."; + private static final String MSG_NULL_KEYS = "keys must not be null."; + private static final String MSG_NULL_POOL = "pool must not be null."; + /** * Timer used to periodically check pools idle object count. Because a * {@link Timer} creates a {@link Thread}, an IODH is used. @@ -101,10 +108,10 @@ public final class PoolUtils { final int minIdle, final long period) throws IllegalArgumentException { if (pool == null) { - throw new IllegalArgumentException("keyedPool must not be null."); + throw new IllegalArgumentException(MSG_NULL_KEYED_POOL); } if (minIdle < 0) { - throw new IllegalArgumentException("minIdle must be non-negative."); + throw new IllegalArgumentException(MSG_MIN_IDLE); } final TimerTask task = new ObjectPoolMinIdleTimerTask<>(pool, minIdle); getMinIdleTimer().schedule(task, 0L, period); @@ -142,13 +149,13 @@ public final class PoolUtils { final int minIdle, final long period) throws IllegalArgumentException { if (keyedPool == null) { - throw new IllegalArgumentException("keyedPool must not be null."); + throw new IllegalArgumentException(MSG_NULL_KEYED_POOL); } if (key == null) { - throw new IllegalArgumentException("key must not be null."); + throw new IllegalArgumentException(MSG_NULL_KEY); } if (minIdle < 0) { - throw new IllegalArgumentException("minIdle must be non-negative."); + throw new IllegalArgumentException(MSG_MIN_IDLE); } final TimerTask task = new KeyedObjectPoolMinIdleTimerTask<>( keyedPool, key, minIdle); @@ -188,7 +195,7 @@ public final class PoolUtils { final int minIdle, final long period) throws IllegalArgumentException { if (keys == null) { - throw new IllegalArgumentException("keys must not be null."); + throw new IllegalArgumentException(MSG_NULL_KEYS); } final Map<K, TimerTask> tasks = new HashMap<>(keys.size()); final Iterator<K> iter = keys.iterator(); @@ -217,7 +224,7 @@ public final class PoolUtils { public static <T> void prefill(final ObjectPool<T> pool, final int count) throws Exception, IllegalArgumentException { if (pool == null) { - throw new IllegalArgumentException("pool must not be null."); + throw new IllegalArgumentException(MSG_NULL_POOL); } for (int i = 0; i < count; i++) { pool.addObject(); @@ -246,10 +253,10 @@ public final class PoolUtils { final K key, final int count) throws Exception, IllegalArgumentException { if (keyedPool == null) { - throw new IllegalArgumentException("keyedPool must not be null."); + throw new IllegalArgumentException(MSG_NULL_KEYED_POOL); } if (key == null) { - throw new IllegalArgumentException("key must not be null."); + throw new IllegalArgumentException(MSG_NULL_KEY); } for (int i = 0; i < count; i++) { keyedPool.addObject(key); @@ -281,7 +288,7 @@ public final class PoolUtils { final Collection<K> keys, final int count) throws Exception, IllegalArgumentException { if (keys == null) { - throw new IllegalArgumentException("keys must not be null."); + throw new IllegalArgumentException(MSG_NULL_KEYS); } final Iterator<K> iter = keys.iterator(); while (iter.hasNext()) { @@ -308,7 +315,7 @@ public final class PoolUtils { */ public static <T> ObjectPool<T> synchronizedPool(final ObjectPool<T> pool) { if (pool == null) { - throw new IllegalArgumentException("pool must not be null."); + throw new IllegalArgumentException(MSG_NULL_POOL); } /* * assert !(pool instanceof GenericObjectPool) : @@ -436,10 +443,10 @@ public final class PoolUtils { public static <T> ObjectPool<T> erodingPool(final ObjectPool<T> pool, final float factor) { if (pool == null) { - throw new IllegalArgumentException("pool must not be null."); + throw new IllegalArgumentException(MSG_NULL_POOL); } if (factor <= 0f) { - throw new IllegalArgumentException("factor must be positive."); + throw new IllegalArgumentException(MSG_FACTOR_NEGATIVE); } return new ErodingObjectPool<>(pool, factor); } @@ -538,10 +545,10 @@ public final class PoolUtils { final KeyedObjectPool<K, V> keyedPool, final float factor, final boolean perKey) { if (keyedPool == null) { - throw new IllegalArgumentException("keyedPool must not be null."); + throw new IllegalArgumentException(MSG_NULL_KEYED_POOL); } if (factor <= 0f) { - throw new IllegalArgumentException("factor must be positive."); + throw new IllegalArgumentException(MSG_FACTOR_NEGATIVE); } if (perKey) { return new ErodingPerKeyKeyedObjectPool<>(keyedPool, factor); @@ -587,7 +594,7 @@ public final class PoolUtils { ObjectPoolMinIdleTimerTask(final ObjectPool<T> pool, final int minIdle) throws IllegalArgumentException { if (pool == null) { - throw new IllegalArgumentException("pool must not be null."); + throw new IllegalArgumentException(MSG_NULL_POOL); } this.pool = pool; this.minIdle = minIdle; @@ -665,7 +672,7 @@ public final class PoolUtils { final K key, final int minIdle) throws IllegalArgumentException { if (keyedPool == null) { throw new IllegalArgumentException( - "keyedPool must not be null."); + MSG_NULL_KEYED_POOL); } this.keyedPool = keyedPool; this.key = key; @@ -747,7 +754,7 @@ public final class PoolUtils { SynchronizedObjectPool(final ObjectPool<T> pool) throws IllegalArgumentException { if (pool == null) { - throw new IllegalArgumentException("pool must not be null."); + throw new IllegalArgumentException(MSG_NULL_POOL); } this.pool = pool; } @@ -924,7 +931,7 @@ public final class PoolUtils { throws IllegalArgumentException { if (keyedPool == null) { throw new IllegalArgumentException( - "keyedPool must not be null."); + MSG_NULL_KEYED_POOL); } this.keyedPool = keyedPool; } @@ -1605,7 +1612,7 @@ public final class PoolUtils { final ErodingFactor erodingFactor) { if (keyedPool == null) { throw new IllegalArgumentException( - "keyedPool must not be null."); + MSG_NULL_KEYED_POOL); } this.keyedPool = keyedPool; this.erodingFactor = erodingFactor; diff --git a/java/org/apache/tomcat/dbcp/pool2/impl/BaseGenericObjectPool.java b/java/org/apache/tomcat/dbcp/pool2/impl/BaseGenericObjectPool.java index 0de61d6..448c4a7 100644 --- a/java/org/apache/tomcat/dbcp/pool2/impl/BaseGenericObjectPool.java +++ b/java/org/apache/tomcat/dbcp/pool2/impl/BaseGenericObjectPool.java @@ -772,11 +772,9 @@ public abstract class BaseGenericObjectPool<T> extends BaseObject { */ final void startEvictor(final long delay) { synchronized (evictionLock) { - if (null != evictor) { - EvictionTimer.cancel(evictor, evictorShutdownTimeoutMillis, TimeUnit.MILLISECONDS); - evictor = null; - evictionIterator = null; - } + EvictionTimer.cancel(evictor, evictorShutdownTimeoutMillis, TimeUnit.MILLISECONDS); + evictor = null; + evictionIterator = null; if (delay > 0) { evictor = new Evictor(); EvictionTimer.schedule(evictor, delay, delay); diff --git a/java/org/apache/tomcat/dbcp/pool2/impl/CallStackUtils.java b/java/org/apache/tomcat/dbcp/pool2/impl/CallStackUtils.java index d5089c3..871f311 100644 --- a/java/org/apache/tomcat/dbcp/pool2/impl/CallStackUtils.java +++ b/java/org/apache/tomcat/dbcp/pool2/impl/CallStackUtils.java @@ -25,12 +25,6 @@ import java.security.AccessControlException; */ public final class CallStackUtils { - private static final boolean CAN_CREATE_SECURITY_MANAGER; - - static { - CAN_CREATE_SECURITY_MANAGER = canCreateSecurityManager(); - } - /** * @return {@code true} if it is able to create a security manager in the current environment, {@code false} * otherwise. @@ -76,7 +70,7 @@ public final class CallStackUtils { public static CallStack newCallStack(final String messageFormat, final boolean useTimestamp, final boolean requireFullStackTrace) { - return CAN_CREATE_SECURITY_MANAGER && !requireFullStackTrace + return canCreateSecurityManager() && !requireFullStackTrace ? new SecurityManagerCallStack(messageFormat, useTimestamp) : new ThrowableCallStack(messageFormat, useTimestamp); } diff --git a/java/org/apache/tomcat/dbcp/pool2/impl/EvictionTimer.java b/java/org/apache/tomcat/dbcp/pool2/impl/EvictionTimer.java index b483dc3..f034c38 100644 --- a/java/org/apache/tomcat/dbcp/pool2/impl/EvictionTimer.java +++ b/java/org/apache/tomcat/dbcp/pool2/impl/EvictionTimer.java @@ -24,18 +24,20 @@ import java.util.concurrent.ThreadFactory; import java.util.concurrent.TimeUnit; /** - * 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 cancelled. - * 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. * <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. + * 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 + * cancelled. 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> + * <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. + * </p> * <p> * This class is intended to be thread-safe. + * </p> * * @since 2.0 */ @@ -66,6 +68,7 @@ class EvictionTimer { * call to this method *must* call {@link #cancel(BaseGenericObjectPool.Evictor,long,TimeUnit)} * to cancel the task to prevent memory and/or thread leaks in application * server environments. + * * @param task Task to be scheduled * @param delay Delay in milliseconds before task is executed * @param period Time in milliseconds between executions @@ -84,16 +87,18 @@ class EvictionTimer { /** * Remove the specified eviction task from the timer. * - * @param task Task to be cancelled + * @param evictor 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 BaseGenericObjectPool<?>.Evictor task, final long timeout, final TimeUnit unit) { - task.cancel(); - if (executor.getQueue().size() == 0) { + final BaseGenericObjectPool<?>.Evictor evictor, final long timeout, final TimeUnit unit) { + if (evictor != null) { + evictor.cancel(); + } + if (executor != null && executor.getQueue().isEmpty()) { executor.shutdown(); try { executor.awaitTermination(timeout, unit); @@ -107,14 +112,14 @@ class EvictionTimer { } /** - * Thread factory that creates a thread, with the context class loader from this class. + * Thread factory that creates a daemon thread, with the context class loader from this class. */ private static class EvictorThreadFactory implements ThreadFactory { @Override public Thread newThread(final Runnable runnable) { final Thread thread = new Thread(null, runnable, "commons-pool-evictor-thread"); - + thread.setDaemon(true); // POOL-363 - Required for applications using Runtime.addShutdownHook(). --joshlandin 03.27.2019 AccessController.doPrivileged(new PrivilegedAction<Void>() { @Override public Void run() { diff --git a/java/org/apache/tomcat/dbcp/pool2/impl/GenericKeyedObjectPool.java b/java/org/apache/tomcat/dbcp/pool2/impl/GenericKeyedObjectPool.java index d9c9c8f..7de46a3 100644 --- a/java/org/apache/tomcat/dbcp/pool2/impl/GenericKeyedObjectPool.java +++ b/java/org/apache/tomcat/dbcp/pool2/impl/GenericKeyedObjectPool.java @@ -1145,26 +1145,29 @@ public class GenericKeyedObjectPool<K, T> extends BaseGenericObjectPool<T> * @param k The key to de-register */ private void deregister(final K k) { + Lock lock = keyLock.readLock(); ObjectDeque<T> objectDeque; - objectDeque = poolMap.get(k); - final long numInterested = objectDeque.getNumInterested().decrementAndGet(); - if (numInterested == 0 && objectDeque.getCreateCount().get() == 0) { - // Potential to remove key - final Lock writeLock = keyLock.writeLock(); - writeLock.lock(); - try { - if (objectDeque.getCreateCount().get() == 0 && - objectDeque.getNumInterested().get() == 0) { + try { + lock.lock(); + objectDeque = poolMap.get(k); + final long numInterested = objectDeque.getNumInterested().decrementAndGet(); + if (numInterested == 0 && objectDeque.getCreateCount().get() == 0) { + // Potential to remove key + // Upgrade to write lock + lock.unlock(); + lock = keyLock.writeLock(); + lock.lock(); + if (objectDeque.getCreateCount().get() == 0 && objectDeque.getNumInterested().get() == 0) { // NOTE: Keys must always be removed from both poolMap and // poolKeyList at the same time while protected by // keyLock.writeLock() poolMap.remove(k); poolKeyList.remove(k); } - } finally { - writeLock.unlock(); } + } finally { + lock.unlock(); } } diff --git a/java/org/apache/tomcat/dbcp/pool2/impl/GenericObjectPool.java b/java/org/apache/tomcat/dbcp/pool2/impl/GenericObjectPool.java index 15b6d6d..04fdd79 100644 --- a/java/org/apache/tomcat/dbcp/pool2/impl/GenericObjectPool.java +++ b/java/org/apache/tomcat/dbcp/pool2/impl/GenericObjectPool.java @@ -181,6 +181,7 @@ public class GenericObjectPool<T> extends BaseGenericObjectPool<T> * <p> * If the configured value of minIdle is greater than the configured value * for maxIdle then the value of maxIdle will be used instead. + * </p> * * @param minIdle * The minimum number of objects. @@ -202,6 +203,7 @@ public class GenericObjectPool<T> extends BaseGenericObjectPool<T> * <p> * If the configured value of minIdle is greater than the configured value * for maxIdle then the value of maxIdle will be used instead. + * </p> * * @return The minimum number of objects. * @@ -339,6 +341,7 @@ public class GenericObjectPool<T> extends BaseGenericObjectPool<T> * borrowObject}({@link #getMaxWaitMillis()})</code>. * <p> * {@inheritDoc} + * </p> */ @Override public T borrowObject() throws Exception { @@ -356,6 +359,7 @@ public class GenericObjectPool<T> extends BaseGenericObjectPool<T> * instance is destroyed and the next available instance is examined. This * continues until either a valid instance is returned or there are no more * idle instances available. + * </p> * <p> * If there are no idle instances available in the pool, behavior depends on * the {@link #getMaxTotal() maxTotal}, (if applicable) @@ -365,6 +369,7 @@ public class GenericObjectPool<T> extends BaseGenericObjectPool<T> * instance is created, activated and (if applicable) validated and returned * to the caller. If validation fails, a <code>NoSuchElementException</code> * is thrown. + * </p> * <p> * If the pool is exhausted (no available idle instances and no capacity to * create new ones), this method will either block (if @@ -374,11 +379,13 @@ public class GenericObjectPool<T> extends BaseGenericObjectPool<T> * method will block when {@link #getBlockWhenExhausted()} is true is * determined by the value passed in to the <code>borrowMaxWaitMillis</code> * parameter. + * </p> * <p> * When the pool is exhausted, multiple calling threads may be * simultaneously blocked waiting for instances to become available. A * "fairness" algorithm has been implemented to ensure that threads receive * available instances in request arrival order. + * </p> * * @param borrowMaxWaitMillis The time to wait in milliseconds for an object * to become available @@ -496,14 +503,17 @@ public class GenericObjectPool<T> extends BaseGenericObjectPool<T> * If {@link #getMaxIdle() maxIdle} is set to a positive value and the * number of idle instances has reached this value, the returning instance * is destroyed. + * </p> * <p> * If {@link #getTestOnReturn() testOnReturn} == true, the returning * instance is validated before being returned to the idle instance pool. In * this case, if validation fails, the instance is destroyed. + * </p> * <p> * Exceptions encountered destroying objects for any reason are swallowed * but notified via a * {@link org.apache.tomcat.dbcp.pool2.SwallowedExceptionListener}. + * </p> */ @Override public void returnObject(final T obj) { @@ -587,6 +597,7 @@ public class GenericObjectPool<T> extends BaseGenericObjectPool<T> * <p> * Activation of this method decrements the active count and attempts to * destroy the instance. + * </p> * * @throws Exception if an exception occurs destroying the * object @@ -617,6 +628,7 @@ public class GenericObjectPool<T> extends BaseGenericObjectPool<T> * idle instance. * <p> * Implementation notes: + * </p> * <ul> * <li>This method does not destroy or effect in any way instances that are * checked out of the pool when it is invoked.</li> @@ -659,6 +671,7 @@ public class GenericObjectPool<T> extends BaseGenericObjectPool<T> * objects destroyed on return. * <p> * Destroys idle instances in the pool by invoking {@link #clear()}. + * </p> */ @Override public void close() { @@ -691,6 +704,7 @@ public class GenericObjectPool<T> extends BaseGenericObjectPool<T> * <p> * Successive activations of this method examine objects in sequence, * cycling through objects in oldest-to-youngest order. + * </p> */ @Override public void evict() throws Exception { @@ -811,6 +825,7 @@ public class GenericObjectPool<T> extends BaseGenericObjectPool<T> * <p> * If there are {@link #getMaxTotal()} objects already in circulation * or in process of being created, this method returns null. + * </p> * * @return The new wrapped pooled object * @@ -915,6 +930,15 @@ public class GenericObjectPool<T> extends BaseGenericObjectPool<T> destroyedCount.incrementAndGet(); createCount.decrementAndGet(); } + + if (idleObjects.isEmpty() && idleObjects.hasTakeWaiters()) { + // POOL-356. + // In case there are already threads waiting on something in the pool + // (e.g. idleObjects.takeFirst(); then we need to provide them a fresh instance. + // Otherwise they will be stuck forever (or until timeout) + final PooledObject<T> freshPooled = create(); + idleObjects.put(freshPooled); + } } @Override @@ -929,6 +953,7 @@ public class GenericObjectPool<T> extends BaseGenericObjectPool<T> * or the total number of objects (idle, checked out, or being created) reaches * {@link #getMaxTotal()}. If {@code always} is false, no instances are created unless * there are threads waiting to check out instances from the pool. + * </p> * * @param idleCount the number of idle instances desired * @param always true means create instances even if the pool has no threads waiting @@ -1115,6 +1140,7 @@ public class GenericObjectPool<T> extends BaseGenericObjectPool<T> * JMX. That means it won't be invoked unless the explicitly requested * whereas all attributes will be automatically requested when viewing the * attributes for an object in a tool like JConsole. + * </p> * * @return Information grouped on all the objects in the pool */ diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 7c77575..e75f345 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -215,9 +215,13 @@ (2019-04-24) pick up some enhancements. (markt) </update> <update> - Update the internal fork of Apache Commons DBCP 2 to + Update the internal fork of Apache Commons DBCP 2 to dcdbc72 (2019-04-24) to pick up some clean-up and enhancements. (markt) </update> + <update> + Update the internal fork of Apache Commons Pool 2 to 0664f4d + (2019-04-30) to pick up some enhancements and bug fixes. (markt) + </update> </changelog> </subsection> </section> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org