I will update Javadoc and add inline comments. Gary
On Sun, Jun 27, 2021, 11:49 Phil Steitz <phil.ste...@gmail.com> wrote: > It's hard to tell what the actual change is below with all of the > formatting / cosmetic changes mixed it, but AFAICT there is no sync > control to ensure consistency or currency of the stats reported. Some > note in javadoc or somewhere should be added to make it clear that stats > may not accurately reflect snapshot state at the time of the exception. > If you want to get a guaranteed accurate snapshot, you would have to > lock the pool when you take it, which we don't want to do. > > On 6/27/21 7:47 AM, ggreg...@apache.org wrote: > > This is an automated email from the ASF dual-hosted git repository. > > > > ggregory pushed a commit to branch master > > in repository https://gitbox.apache.org/repos/asf/commons-pool.git > > > > > > The following commit(s) were added to refs/heads/master by this push: > > new 26b0829 [POOL-395] Improve exception thrown in > GenericObjectPool.borrowObject when pool is exhausted. Added > BaseGenericObjectPool.setMessagesStatistics(boolean). > > 26b0829 is described below > > > > commit 26b0829fef39729df84adb1c01eb55c944aff6d7 > > Author: Gary Gregory <garydgreg...@gmail.com> > > AuthorDate: Sun Jun 27 10:47:03 2021 -0400 > > > > [POOL-395] Improve exception thrown in > GenericObjectPool.borrowObject > > when pool is exhausted. Added > > BaseGenericObjectPool.setMessagesStatistics(boolean). > > > > - Pick up Checkstyle LineLength module from Commons IO where there > was > > none before. > > - Message sentences start with a capital letter. > > - Better parameter names. > > - Javadoc. > > - Inline comments. > > --- > > checkstyle.xml | 4 + > > src/changes/changes.xml | 3 + > > .../org/apache/commons/pool2/KeyedObjectPool.java | 4 +- > > .../commons/pool2/KeyedPooledObjectFactory.java | 4 +- > > .../java/org/apache/commons/pool2/ObjectPool.java | 4 +- > > .../apache/commons/pool2/PooledObjectFactory.java | 4 +- > > .../commons/pool2/impl/BaseGenericObjectPool.java | 114 > ++++++++++++++++----- > > .../commons/pool2/impl/GenericKeyedObjectPool.java | 48 +++++---- > > .../commons/pool2/impl/GenericObjectPool.java | 46 +++++---- > > .../pool2/impl/SoftReferenceObjectPool.java | 4 +- > > .../pool2/TestBasePoolableObjectFactory.java | 4 +- > > .../pool2/impl/TestAbandonedKeyedObjectPool.java | 4 +- > > .../pool2/impl/TestAbandonedObjectPool.java | 4 +- > > .../pool2/impl/TestGenericKeyedObjectPool.java | 64 +++++++----- > > .../commons/pool2/impl/TestGenericObjectPool.java | 28 ++++- > > 15 files changed, 230 insertions(+), 109 deletions(-) > > > > diff --git a/checkstyle.xml b/checkstyle.xml > > index dca770b..3f884e1 100644 > > --- a/checkstyle.xml > > +++ b/checkstyle.xml > > @@ -74,4 +74,8 @@ > > <property name="allowLegacy" value="true"/> > > </module> > > > > + <module name="LineLength"> > > + <property name="max" value="160"/> > > + </module> > > + > > </module> > > diff --git a/src/changes/changes.xml b/src/changes/changes.xml > > index 40ef624..2368098 100644 > > --- a/src/changes/changes.xml > > +++ b/src/changes/changes.xml > > @@ -69,6 +69,9 @@ The <action> type attribute can be > add,update,fix,remove. > > <action issue="POOL-396" dev="ggregory" type="add" due-to="Jeremy > Kong, Phil Steitz"> > > Handle validation exceptions during eviction. #85. > > </action> > > + <action issue="POOL-395" dev="ggregory" type="add" due-to="Gary > Gregory, Arash Nikoo"> > > + Improve exception thrown in GenericObjectPool.borrowObject when > pool is exhausted. Added > BaseGenericObjectPool.setMessagesStatistics(boolean). > > + </action> > > <!-- FIXES --> > > <action dev="ggregory" type="fix" due-to="Gary Gregory"> > > Fix [WARNING] Old version of checkstyle detected. Consider > updating to >= v8.30. Update Checktyle to 8.43. > > diff --git a/src/main/java/org/apache/commons/pool2/KeyedObjectPool.java > b/src/main/java/org/apache/commons/pool2/KeyedObjectPool.java > > index ab52908..cac55ac 100644 > > --- a/src/main/java/org/apache/commons/pool2/KeyedObjectPool.java > > +++ b/src/main/java/org/apache/commons/pool2/KeyedObjectPool.java > > @@ -289,12 +289,12 @@ public interface KeyedObjectPool<K, V> extends > Closeable { > > * > > * @param key the key used to obtain the object > > * @param obj a {@link #borrowObject borrowed} instance to be > returned. > > - * @param mode destroy activation context provided to the factory > > + * @param destroyMode destroy activation context provided to the > factory > > * > > * @throws Exception if the instance cannot be invalidated > > * @since 2.9.0 > > */ > > - default void invalidateObject(final K key, final V obj, final > DestroyMode mode) throws Exception { > > + default void invalidateObject(final K key, final V obj, final > DestroyMode destroyMode) throws Exception { > > invalidateObject(key, obj); > > } > > > > diff --git > a/src/main/java/org/apache/commons/pool2/KeyedPooledObjectFactory.java > b/src/main/java/org/apache/commons/pool2/KeyedPooledObjectFactory.java > > index ac93114..a7f75a3 100644 > > --- > a/src/main/java/org/apache/commons/pool2/KeyedPooledObjectFactory.java > > +++ > b/src/main/java/org/apache/commons/pool2/KeyedPooledObjectFactory.java > > @@ -117,7 +117,7 @@ public interface KeyedPooledObjectFactory<K, V> { > > * > > * @param key the key used when selecting the instance > > * @param p a {@code PooledObject} wrapping the instance to be > destroyed > > - * @param mode DestroyMode providing context to the factory > > + * @param destroyMode DestroyMode providing context to the factory > > * > > * @throws Exception should be avoided as it may be swallowed by > > * the pool implementation. > > @@ -128,7 +128,7 @@ public interface KeyedPooledObjectFactory<K, V> { > > * @see DestroyMode > > * @since 2.9.0 > > */ > > - default void destroyObject(final K key, final PooledObject<V> p, > final DestroyMode mode) throws Exception { > > + default void destroyObject(final K key, final PooledObject<V> p, > final DestroyMode destroyMode) throws Exception { > > destroyObject(key, p); > > } > > > > diff --git a/src/main/java/org/apache/commons/pool2/ObjectPool.java > b/src/main/java/org/apache/commons/pool2/ObjectPool.java > > index 8225d27..8aa2d58 100644 > > --- a/src/main/java/org/apache/commons/pool2/ObjectPool.java > > +++ b/src/main/java/org/apache/commons/pool2/ObjectPool.java > > @@ -198,12 +198,12 @@ public interface ObjectPool<T> extends Closeable { > > * </p> > > * > > * @param obj a {@link #borrowObject borrowed} instance to be > disposed. > > - * @param mode destroy activation context provided to the factory > > + * @param destroyMode destroy activation context provided to the > factory > > * > > * @throws Exception if the instance cannot be invalidated > > * @since 2.9.0 > > */ > > - default void invalidateObject(final T obj, final DestroyMode mode) > throws Exception { > > + default void invalidateObject(final T obj, final DestroyMode > destroyMode) throws Exception { > > invalidateObject(obj); > > } > > > > diff --git > a/src/main/java/org/apache/commons/pool2/PooledObjectFactory.java > b/src/main/java/org/apache/commons/pool2/PooledObjectFactory.java > > index 009f341..839e866 100644 > > --- a/src/main/java/org/apache/commons/pool2/PooledObjectFactory.java > > +++ b/src/main/java/org/apache/commons/pool2/PooledObjectFactory.java > > @@ -112,7 +112,7 @@ public interface PooledObjectFactory<T> { > > * DestroyMode. > > * > > * @param p a {@code PooledObject} wrapping the instance to be > destroyed > > - * @param mode DestroyMode providing context to the factory > > + * @param destroyMode DestroyMode providing context to the factory > > * > > * @throws Exception should be avoided as it may be swallowed by > > * the pool implementation. > > @@ -123,7 +123,7 @@ public interface PooledObjectFactory<T> { > > * @see DestroyMode > > * @since 2.9.0 > > */ > > - default void destroyObject(final PooledObject<T> p, final DestroyMode > mode) throws Exception { > > + default void destroyObject(final PooledObject<T> p, final DestroyMode > destroyMode) throws Exception { > > destroyObject(p); > > } > > > > 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 000a6eb..83f37e1 100644 > > --- > a/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java > > +++ > b/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java > > @@ -26,9 +26,11 @@ import java.time.Duration; > > import java.util.Arrays; > > import java.util.Deque; > > import java.util.Iterator; > > +import java.util.List; > > import java.util.TimerTask; > > import java.util.concurrent.ScheduledFuture; > > import java.util.concurrent.atomic.AtomicLong; > > +import java.util.stream.Collectors; > > > > import javax.management.InstanceAlreadyExistsException; > > import javax.management.InstanceNotFoundException; > > @@ -233,12 +235,14 @@ public abstract class BaseGenericObjectPool<T> > extends BaseObject { > > return builder.toString(); > > } > > } > > + > > /** > > * Maintains a cache of values for a single metric and reports > > * statistics on the cached values. > > */ > > private class StatsStore { > > > > + private static final int NULL = -1; > > private final AtomicLong[] values; > > private final int size; > > private int index; > > @@ -252,7 +256,7 @@ public abstract class BaseGenericObjectPool<T> > extends BaseObject { > > this.size = size; > > values = new AtomicLong[size]; > > for (int i = 0; i < size; i++) { > > - values[i] = new AtomicLong(-1); > > + values[i] = new AtomicLong(NULL); > > } > > } > > > > @@ -275,6 +279,15 @@ public abstract class BaseGenericObjectPool<T> > extends BaseObject { > > } > > > > /** > > + * Gets the current values as a List. > > + * > > + * @return the current values as a List. > > + */ > > + synchronized List<AtomicLong> getCurrentValues() { > > + return Arrays.stream(values, 0, > index).collect(Collectors.toList()); > > + } > > + > > + /** > > * Gets the mean of the cached values. > > * > > * @return the mean of the cache, truncated to long > > @@ -284,10 +297,9 @@ public abstract class BaseGenericObjectPool<T> > extends BaseObject { > > int counter = 0; > > for (int i = 0; i < size; i++) { > > final long value = values[i].get(); > > - if (value != -1) { > > + if (value != NULL) { > > counter++; > > - result = result * ((counter - 1) / (double) > counter) + > > - value/(double) counter; > > + result = result * ((counter - 1) / (double) > counter) + value / (double) counter; > > } > > } > > return (long) result; > > @@ -296,16 +308,19 @@ public abstract class BaseGenericObjectPool<T> > extends BaseObject { > > @Override > > public String toString() { > > final StringBuilder builder = new StringBuilder(); > > - builder.append("StatsStore [values="); > > - builder.append(Arrays.toString(values)); > > - builder.append(", size="); > > + builder.append("StatsStore ["); > > + // Only append what's been filled in. > > + builder.append(getCurrentValues()); > > + builder.append("], size="); > > builder.append(size); > > builder.append(", index="); > > builder.append(index); > > builder.append("]"); > > return builder.toString(); > > } > > + > > } > > + > > // Constants > > /** > > * The size of the caches used to store historical data for some > attributes > > @@ -334,7 +349,6 @@ public abstract class BaseGenericObjectPool<T> > extends BaseObject { > > final Object closeLock = new Object(); > > volatile boolean closed; > > > > - > > final Object evictionLock = new Object(); > > private Evictor evictor = null; // @GuardedBy("evictionLock") > > EvictionIterator evictionIterator = null; // > @GuardedBy("evictionLock") > > @@ -348,21 +362,21 @@ public abstract class BaseGenericObjectPool<T> > extends BaseObject { > > // Monitoring (primarily JMX) attributes > > private final ObjectName objectName; > > private final String creationStackTrace; > > - private final AtomicLong borrowedCount = new AtomicLong(0); > > - private final AtomicLong returnedCount = new AtomicLong(0); > > - final AtomicLong createdCount = new AtomicLong(0); > > - final AtomicLong destroyedCount = new AtomicLong(0); > > - final AtomicLong destroyedByEvictorCount = new AtomicLong(0); > > - final AtomicLong destroyedByBorrowValidationCount = new > AtomicLong(0); > > + private final AtomicLong borrowedCount = new AtomicLong(); > > + private final AtomicLong returnedCount = new AtomicLong(); > > + final AtomicLong createdCount = new AtomicLong(); > > + final AtomicLong destroyedCount = new AtomicLong(); > > + final AtomicLong destroyedByEvictorCount = new AtomicLong(); > > + final AtomicLong destroyedByBorrowValidationCount = new > AtomicLong(); > > + > > private final StatsStore activeTimes = new > StatsStore(MEAN_TIMING_STATS_CACHE_SIZE); > > - > > private final StatsStore idleTimes = new > StatsStore(MEAN_TIMING_STATS_CACHE_SIZE); > > - > > private final StatsStore waitTimes = new > StatsStore(MEAN_TIMING_STATS_CACHE_SIZE); > > > > - private final AtomicLong maxBorrowWaitTimeMillis = new > AtomicLong(0L); > > + private final AtomicLong maxBorrowWaitTimeMillis = new AtomicLong(); > > > > - private volatile SwallowedExceptionListener > swallowedExceptionListener = null; > > + private volatile SwallowedExceptionListener > swallowedExceptionListener; > > + private volatile boolean messageStatistics; > > > > /** > > * Handles JMX registration (if required) and the initialization > required for > > @@ -396,6 +410,16 @@ public abstract class BaseGenericObjectPool<T> > extends BaseObject { > > } > > > > /** > > + * Appends statistics if enabled. > > + * > > + * @param string The root string. > > + * @return The root string plus statistics. > > + */ > > + String appendStats(final String string) { > > + return messageStatistics ? string + ", " + getStatsString() : > string; > > + } > > + > > + /** > > * Verifies that the pool is open. > > * @throws IllegalStateException if the pool is closed. > > */ > > @@ -680,6 +704,16 @@ public abstract class BaseGenericObjectPool<T> > extends BaseObject { > > } > > > > /** > > + * Gets whether to include statistics in exception messages. > > + * > > + * @return whether to include statistics in exception messages. > > + * @since 2.11.0 > > + */ > > + public boolean getMessageStatistics() { > > + return messageStatistics; > > + } > > + > > + /** > > * Gets the minimum amount of time an object may sit idle in the > pool > > * before it is eligible for eviction by the idle object evictor > (if any - > > * see {@link #setTimeBetweenEvictionRuns(Duration)}). When > non-positive, > > @@ -806,6 +840,22 @@ public abstract class BaseGenericObjectPool<T> > extends BaseObject { > > } > > > > /** > > + * Gets a statistics string. > > + * > > + * @return a statistics string. > > + */ > > + String getStatsString() { > > + // Simply listed in AB order. > > + return String.format( > > + "activeTimes=%s, blockWhenExhausted=%s, > borrowedCount=%,d, closed=%s, createdCount=%,d, > destroyedByBorrowValidationCount=%,d, destroyedByEvictorCount=%,d, > evictorShutdownTimeout=%s, fairness=%s, idleTimes=%s, lifo=%s, > maxBorrowWaitTimeMillis=%,d, maxTotal=%s, maxWaitDuration=%s, > minEvictableIdleTime=%s, numTestsPerEvictionRun=%s, returnedCount=%s, > softMinEvictableIdleTime=%s, testOnBorrow=%s, testOnCreate=%s, > testOnReturn=%s, testWhileIdle=%s, timeBetweenEvictionRuns=%s, [...] > > + activeTimes.getCurrentValues(), blockWhenExhausted, > borrowedCount.get(), closed, createdCount.get(), > > + destroyedByBorrowValidationCount.get(), > destroyedByEvictorCount.get(), evictorShutdownTimeout, fairness, > > + idleTimes.getCurrentValues(), lifo, > maxBorrowWaitTimeMillis.get(), maxTotal, maxWaitDuration, > > + minEvictableIdleTime, numTestsPerEvictionRun, > returnedCount, softMinEvictableIdleTime, testOnBorrow, > > + testOnCreate, testOnReturn, testWhileIdle, > timeBetweenEvictionRuns, waitTimes.getCurrentValues()); > > + } > > + > > + /** > > * The listener used (if any) to receive notifications of > exceptions > > * unavoidably swallowed by the pool. > > * > > @@ -984,8 +1034,7 @@ public abstract class BaseGenericObjectPool<T> > extends BaseObject { > > final void jmxUnregister() { > > if (objectName != null) { > > try { > > - > ManagementFactory.getPlatformMBeanServer().unregisterMBean( > > - objectName); > > + > ManagementFactory.getPlatformMBeanServer().unregisterMBean(objectName); > > } catch (final MBeanRegistrationException | > InstanceNotFoundException e) { > > swallowException(e); > > } > > @@ -997,10 +1046,9 @@ public abstract class BaseGenericObjectPool<T> > extends BaseObject { > > * @param pooledObject instance to return to the keyed pool > > */ > > protected void markReturningState(final PooledObject<T> > pooledObject) { > > - synchronized(pooledObject) { > > + synchronized (pooledObject) { > > if (pooledObject.getState() != > PooledObjectState.ALLOCATED) { > > - throw new IllegalStateException( > > - "Object has already been returned to this pool > or is invalid"); > > + throw new IllegalStateException("Object has already > been returned to this pool or is invalid"); > > } > > pooledObject.markReturning(); // Keep from being marked > abandoned > > } > > @@ -1121,9 +1169,9 @@ public abstract class BaseGenericObjectPool<T> > extends BaseObject { > > classLoader + ", " + epClassLoader + "] do not > implement " + EVICTION_POLICY_TYPE_NAME); > > } catch (final ClassNotFoundException | InstantiationException > | IllegalAccessException | > > InvocationTargetException | NoSuchMethodException e) { > > - final String exMessage = "Unable to create " + > EVICTION_POLICY_TYPE_NAME + " instance of type " + > > - evictionPolicyClassName; > > - throw new IllegalArgumentException(exMessage, e); > > + throw new IllegalArgumentException( > > + "Unable to create " + EVICTION_POLICY_TYPE_NAME + " > instance of type " + evictionPolicyClassName, > > + e); > > } > > } > > > > @@ -1223,6 +1271,16 @@ public abstract class BaseGenericObjectPool<T> > extends BaseObject { > > } > > > > /** > > + * Sets whether to include statistics in exception messages. > > + * > > + * @param messagesDetails whether to include statistics in > exception messages. > > + * @since 2.11.0 > > + */ > > + public void setMessagesStatistics(boolean messagesDetails) { > > + this.messageStatistics = messagesDetails; > > + } > > + > > + /** > > * Sets the minimum amount of time an object may sit idle in the > pool > > * before it is eligible for eviction by the idle object evictor > (if any - > > * see {@link #setTimeBetweenEvictionRuns(Duration)}). When > non-positive, > > @@ -1446,6 +1504,8 @@ public abstract class BaseGenericObjectPool<T> > extends BaseObject { > > > setTimeBetweenEvictionRuns(Duration.ofMillis(timeBetweenEvictionRunsMillis)); > > } > > > > + // Inner classes > > + > > /** > > * <p>Starts the evictor with the given delay. If there is an > evictor > > * running when this method is called, it is stopped and replaced > with a > > @@ -1478,8 +1538,6 @@ public abstract class BaseGenericObjectPool<T> > extends BaseObject { > > } > > } > > > > - // Inner classes > > - > > /** > > * Stops the evictor. > > */ > > 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 11fd633..64274bd 100644 > > --- > a/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java > > +++ > b/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java > > @@ -121,7 +121,7 @@ public class GenericKeyedObjectPool<K, T> extends > BaseGenericObjectPool<T> > > * Invariant: empty keyed pool will not be dropped unless > numInterested > > * is 0. > > */ > > - private final AtomicLong numInterested = new AtomicLong(0); > > + private final AtomicLong numInterested = new AtomicLong(); > > > > /** > > * Constructs a new ObjecDeque with the given fairness policy. > > @@ -267,7 +267,7 @@ public class GenericKeyedObjectPool<K, T> extends > BaseGenericObjectPool<T> > > > > if (factory == null) { > > jmxUnregister(); // tidy up > > - throw new IllegalArgumentException("factory may not be > null"); > > + throw new IllegalArgumentException("Factory may not be > null"); > > } > > this.factory = factory; > > this.fairness = config.getFairness(); > > @@ -449,16 +449,15 @@ public class GenericKeyedObjectPool<K, T> extends > BaseGenericObjectPool<T> > > if (borrowMaxWaitMillis < 0) { > > p = > objectDeque.getIdleObjects().takeFirst(); > > } else { > > - p = objectDeque.getIdleObjects().pollFirst( > > - borrowMaxWaitMillis, > TimeUnit.MILLISECONDS); > > + p = > objectDeque.getIdleObjects().pollFirst(borrowMaxWaitMillis, > TimeUnit.MILLISECONDS); > > } > > } > > if (p == null) { > > - throw new NoSuchElementException( > > - "Timeout waiting for idle object"); > > + throw new NoSuchElementException(appendStats( > > + "Timeout waiting for idle object, > borrowMaxWaitMillis=" + borrowMaxWaitMillis)); > > } > > } else if (p == null) { > > - throw new NoSuchElementException("Pool exhausted"); > > + throw new NoSuchElementException(appendStats("Pool > exhausted")); > > } > > if (!p.allocate()) { > > p = null; > > @@ -475,8 +474,7 @@ public class GenericKeyedObjectPool<K, T> extends > BaseGenericObjectPool<T> > > } > > p = null; > > if (create) { > > - final NoSuchElementException nsee = new > NoSuchElementException( > > - "Unable to activate object"); > > + final NoSuchElementException nsee = new > NoSuchElementException(appendStats("Unable to activate object")); > > nsee.initCause(e); > > throw nsee; > > } > > @@ -500,7 +498,7 @@ public class GenericKeyedObjectPool<K, T> extends > BaseGenericObjectPool<T> > > p = null; > > if (create) { > > final NoSuchElementException nsee = > new NoSuchElementException( > > - "Unable to validate object"); > > + appendStats("Unable to validate > object")); > > nsee.initCause(validationThrowable); > > throw nsee; > > } > > @@ -517,6 +515,13 @@ public class GenericKeyedObjectPool<K, T> extends > BaseGenericObjectPool<T> > > return p.getObject(); > > } > > > > + @Override > > + String getStatsString() { > > + // Simply listed in AB order. > > + return super.getStatsString() + > > + String.format(", fairness=%s, maxIdlePerKey%,d, > maxTotalPerKey=%,d, minIdlePerKey=%,d, numTotal=%,d", > > + fairness, maxIdlePerKey, maxTotalPerKey, > minIdlePerKey, numTotal.get()); > > + } > > > > /** > > * Calculate the number of objects that need to be created to > attempt to > > @@ -820,7 +825,7 @@ public class GenericKeyedObjectPool<K, T> extends > BaseGenericObjectPool<T> > > /** > > * De-register the use of a key by an object. > > * <p> > > - * register() and deregister() must always be used as a pair. > > + * {@link #register()} and {@link #deregister()} must always be > used as a pair. > > * </p> > > * > > * @param k The key to de-register > > @@ -858,12 +863,12 @@ public class GenericKeyedObjectPool<K, T> extends > BaseGenericObjectPool<T> > > * @param toDestroy The wrapped object to be destroyed > > * @param always Should the object be destroyed even if it is not > currently > > * in the set of idle objects for the given key > > - * @param mode DestroyMode context provided to the factory > > + * @param destroyMode DestroyMode context provided to the factory > > * > > * @return {@code true} if the object was destroyed, otherwise > {@code false} > > * @throws Exception If the object destruction failed > > */ > > - private boolean destroy(final K key, final PooledObject<T> > toDestroy, final boolean always, final DestroyMode mode) > > + private boolean destroy(final K key, final PooledObject<T> > toDestroy, final boolean always, final DestroyMode destroyMode) > > throws Exception { > > > > final ObjectDeque<T> objectDeque = register(key); > > @@ -884,7 +889,7 @@ public class GenericKeyedObjectPool<K, T> extends > BaseGenericObjectPool<T> > > toDestroy.invalidate(); > > > > try { > > - factory.destroyObject(key, toDestroy, mode); > > + factory.destroyObject(key, toDestroy, destroyMode); > > } finally { > > objectDeque.getCreateCount().decrementAndGet(); > > destroyedCount.incrementAndGet(); > > @@ -1414,7 +1419,7 @@ public class GenericKeyedObjectPool<K, T> extends > BaseGenericObjectPool<T> > > * > > * @param key pool key > > * @param obj instance to invalidate > > - * @param mode DestroyMode context provided to factory > > + * @param destroyMode DestroyMode context provided to factory > > * > > * @throws Exception if an exception occurs destroying > the > > * object > > @@ -1423,15 +1428,15 @@ public class GenericKeyedObjectPool<K, T> > extends BaseGenericObjectPool<T> > > * @since 2.9.0 > > */ > > @Override > > - public void invalidateObject(final K key, final T obj, final > DestroyMode mode) throws Exception { > > + public void invalidateObject(final K key, final T obj, final > DestroyMode destroyMode) throws Exception { > > final ObjectDeque<T> objectDeque = poolMap.get(key); > > final PooledObject<T> p = objectDeque.getAllObjects().get(new > IdentityWrapper<>(obj)); > > if (p == null) { > > - throw new IllegalStateException("Object not currently part > of this pool"); > > + throw new IllegalStateException(appendStats("Object not > currently part of this pool")); > > } > > synchronized (p) { > > if (p.getState() != PooledObjectState.INVALID) { > > - destroy(key, p, true, mode); > > + destroy(key, p, true, destroyMode); > > } > > } > > if (objectDeque.idleObjects.hasTakeWaiters()) { > > @@ -1502,7 +1507,7 @@ public class GenericKeyedObjectPool<K, T> extends > BaseGenericObjectPool<T> > > /** > > * Register the use of a key by an object. > > * <p> > > - * register() and deregister() must always be used as a pair. > > + * {@link #register()} and {@link #deregister()} must always be > used as a pair. > > * </p> > > * > > * @param k The key to register > > @@ -1579,6 +1584,7 @@ public class GenericKeyedObjectPool<K, T> extends > BaseGenericObjectPool<T> > > try { > > invalidateObject(pool.getKey(), > pooledObject.getObject(), DestroyMode.ABANDONED); > > } catch (final Exception e) { > > + // TODO handle/log? > > e.printStackTrace(); > > } > > } > > @@ -1828,9 +1834,9 @@ public class GenericKeyedObjectPool<K, T> extends > BaseGenericObjectPool<T> > > * > > * @param minIdlePerKey The minimum size of the each keyed pool > > * > > - * @see #getMinIdlePerKey > > + * @see #getMinIdlePerKey() > > * @see #getMaxIdlePerKey() > > - * @see #setTimeBetweenEvictionRunsMillis > > + * @see #setTimeBetweenEvictionRuns(Duration) > > */ > > public void setMinIdlePerKey(final int minIdlePerKey) { > > this.minIdlePerKey = minIdlePerKey; > > 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 ee27b58..c5fed24 100644 > > --- a/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java > > +++ b/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java > > @@ -115,7 +115,7 @@ public class GenericObjectPool<T> extends > BaseGenericObjectPool<T> > > * {@link #create()} will ensure that there are never more than > > * {@link #_maxActive} objects created at any one time. > > */ > > - private final AtomicLong createCount = new AtomicLong(0); > > + private final AtomicLong createCount = new AtomicLong(); > > > > private long makeObjectCount; > > > > @@ -155,7 +155,7 @@ public class GenericObjectPool<T> extends > BaseGenericObjectPool<T> > > > > if (factory == null) { > > jmxUnregister(); // tidy up > > - throw new IllegalArgumentException("factory may not be > null"); > > + throw new IllegalArgumentException("Factory may not be > null"); > > } > > this.factory = factory; > > > > @@ -214,8 +214,7 @@ public class GenericObjectPool<T> extends > BaseGenericObjectPool<T> > > public void addObject() throws Exception { > > assertOpen(); > > if (factory == null) { > > - throw new IllegalStateException( > > - "Cannot add objects without a factory."); > > + throw new IllegalStateException("Cannot add objects without > a factory."); > > } > > final PooledObject<T> p = create(); > > addIdleObject(p); > > @@ -271,7 +270,7 @@ public class GenericObjectPool<T> extends > BaseGenericObjectPool<T> > > * available instances in request arrival order. > > * </p> > > * > > - * @param borrowMaxWait The time to wait for an object > > + * @param borrowMaxWaitDuration The time to wait for an object > > * to become available > > * > > * @return object instance from the pool > > @@ -282,11 +281,11 @@ public class GenericObjectPool<T> extends > BaseGenericObjectPool<T> > > * error > > * @since 2.10.0 > > */ > > - public T borrowObject(final Duration borrowMaxWait) throws > Exception { > > + public T borrowObject(final Duration borrowMaxWaitDuration) throws > Exception { > > assertOpen(); > > > > final AbandonedConfig ac = this.abandonedConfig; > > - if (ac != null && ac.getRemoveAbandonedOnBorrow() && > (getNumIdle() < 2) && > > + if (ac != null && ac.getRemoveAbandonedOnBorrow() && > (getNumIdle() < 2) && > > (getNumActive() > getMaxTotal() - 3)) { > > removeAbandoned(ac); > > } > > @@ -311,17 +310,18 @@ public class GenericObjectPool<T> extends > BaseGenericObjectPool<T> > > } > > if (blockWhenExhausted) { > > if (p == null) { > > - if (borrowMaxWait.isNegative()) { > > + if (borrowMaxWaitDuration.isNegative()) { > > p = idleObjects.takeFirst(); > > } else { > > - p = idleObjects.pollFirst(borrowMaxWait); > > + p = > idleObjects.pollFirst(borrowMaxWaitDuration); > > } > > } > > if (p == null) { > > - throw new NoSuchElementException("Timeout waiting > for idle object"); > > + throw new NoSuchElementException(appendStats( > > + "Timeout waiting for idle object, > borrowMaxWaitMillis=" + borrowMaxWaitDuration)); > > } > > } else if (p == null) { > > - throw new NoSuchElementException("Pool exhausted"); > > + throw new NoSuchElementException(appendStats("Pool > exhausted")); > > } > > if (!p.allocate()) { > > p = null; > > @@ -338,7 +338,8 @@ public class GenericObjectPool<T> extends > BaseGenericObjectPool<T> > > } > > p = null; > > if (create) { > > - final NoSuchElementException nsee = new > NoSuchElementException("Unable to activate object"); > > + final NoSuchElementException nsee = new > NoSuchElementException( > > + appendStats("Unable to activate > object")); > > nsee.initCause(e); > > throw nsee; > > } > > @@ -361,7 +362,8 @@ public class GenericObjectPool<T> extends > BaseGenericObjectPool<T> > > } > > p = null; > > if (create) { > > - final NoSuchElementException nsee = new > NoSuchElementException("Unable to validate object"); > > + final NoSuchElementException nsee = new > NoSuchElementException( > > + appendStats("Unable to validate > object")); > > nsee.initCause(validationThrowable); > > throw nsee; > > } > > @@ -375,6 +377,14 @@ public class GenericObjectPool<T> extends > BaseGenericObjectPool<T> > > return p.getObject(); > > } > > > > + @Override > > + String getStatsString() { > > + // Simply listed in AB order. > > + return super.getStatsString() + > > + String.format(", createdCount=%,d, makeObjectCount=%,d, > maxIdle=%,d, minIdle=%,d", > > + createdCount.get(), makeObjectCount, maxIdle, > minIdle); > > + } > > + > > /** > > * Borrows an object from the pool using the specific waiting time > which only > > * applies if {@link #getBlockWhenExhausted()} is true. > > @@ -592,17 +602,17 @@ public class GenericObjectPool<T> extends > BaseGenericObjectPool<T> > > * Destroys a wrapped pooled object. > > * > > * @param toDestroy The wrapped pooled object to destroy > > - * @param mode DestroyMode context provided to the factory > > + * @param destroyMode DestroyMode context provided to the factory > > * > > * @throws Exception If the factory fails to destroy the pooled > object > > * cleanly > > */ > > - private void destroy(final PooledObject<T> toDestroy, final > DestroyMode mode) throws Exception { > > + private void destroy(final PooledObject<T> toDestroy, final > DestroyMode destroyMode) throws Exception { > > toDestroy.invalidate(); > > idleObjects.remove(toDestroy); > > allObjects.remove(new > IdentityWrapper<>(toDestroy.getObject())); > > try { > > - factory.destroyObject(toDestroy, mode); > > + factory.destroyObject(toDestroy, destroyMode); > > } finally { > > destroyedCount.incrementAndGet(); > > createCount.decrementAndGet(); > > @@ -1002,7 +1012,7 @@ public class GenericObjectPool<T> extends > BaseGenericObjectPool<T> > > * @since 2.9.0 > > */ > > @Override > > - public void invalidateObject(final T obj, final DestroyMode mode) > throws Exception { > > + public void invalidateObject(final T obj, final DestroyMode > destroyMode) throws Exception { > > final PooledObject<T> p = allObjects.get(new > IdentityWrapper<>(obj)); > > if (p == null) { > > if (isAbandonedConfig()) { > > @@ -1013,7 +1023,7 @@ public class GenericObjectPool<T> extends > BaseGenericObjectPool<T> > > } > > synchronized (p) { > > if (p.getState() != PooledObjectState.INVALID) { > > - destroy(p, mode); > > + destroy(p, destroyMode); > > } > > } > > ensureIdle(1, false); > > diff --git > a/src/main/java/org/apache/commons/pool2/impl/SoftReferenceObjectPool.java > b/src/main/java/org/apache/commons/pool2/impl/SoftReferenceObjectPool.java > > index 849629f..f7c271c 100644 > > --- > a/src/main/java/org/apache/commons/pool2/impl/SoftReferenceObjectPool.java > > +++ > b/src/main/java/org/apache/commons/pool2/impl/SoftReferenceObjectPool.java > > @@ -216,9 +216,7 @@ public class SoftReferenceObjectPool<T> extends > BaseObjectPool<T> { > > obj = null; > > } > > if (newlyCreated) { > > - throw new NoSuchElementException( > > - "Could not create a validated object, > cause: " + > > - t.getMessage()); > > + throw new NoSuchElementException("Could not > create a validated object, cause: " + t); > > } > > } > > } > > diff --git > a/src/test/java/org/apache/commons/pool2/TestBasePoolableObjectFactory.java > b/src/test/java/org/apache/commons/pool2/TestBasePoolableObjectFactory.java > > index 97c411e..43935d5 100644 > > --- > a/src/test/java/org/apache/commons/pool2/TestBasePoolableObjectFactory.java > > +++ > b/src/test/java/org/apache/commons/pool2/TestBasePoolableObjectFactory.java > > @@ -34,8 +34,8 @@ public class TestBasePoolableObjectFactory { > > return new AtomicInteger(0); > > } > > @Override > > - public void destroyObject(final PooledObject<AtomicInteger> p, > final DestroyMode mode){ > > - if (mode.equals(DestroyMode.ABANDONED)) { > > + public void destroyObject(final PooledObject<AtomicInteger> p, > final DestroyMode destroyMode){ > > + if (destroyMode.equals(DestroyMode.ABANDONED)) { > > p.getObject().incrementAndGet(); > > } > > } > > diff --git > a/src/test/java/org/apache/commons/pool2/impl/TestAbandonedKeyedObjectPool.java > b/src/test/java/org/apache/commons/pool2/impl/TestAbandonedKeyedObjectPool.java > > index 541e1ea..0d850c9 100644 > > --- > a/src/test/java/org/apache/commons/pool2/impl/TestAbandonedKeyedObjectPool.java > > +++ > b/src/test/java/org/apache/commons/pool2/impl/TestAbandonedKeyedObjectPool.java > > @@ -103,7 +103,7 @@ public class TestAbandonedKeyedObjectPool { > > } > > > > @Override > > - public void destroyObject(final Integer key, final > PooledObject<PooledTestObject> obj, final DestroyMode mode) throws > Exception { > > + public void destroyObject(final Integer key, final > PooledObject<PooledTestObject> obj, final DestroyMode destroyMode) throws > Exception { > > obj.getObject().setActive(false); > > // while destroying instances, yield control to other > threads > > // helps simulate threading errors > > @@ -111,7 +111,7 @@ public class TestAbandonedKeyedObjectPool { > > if (destroyLatency != 0) { > > Thread.sleep(destroyLatency); > > } > > - obj.getObject().destroy(mode); > > + obj.getObject().destroy(destroyMode); > > } > > > > @Override > > diff --git > a/src/test/java/org/apache/commons/pool2/impl/TestAbandonedObjectPool.java > b/src/test/java/org/apache/commons/pool2/impl/TestAbandonedObjectPool.java > > index 14b264c..9b64560 100644 > > --- > a/src/test/java/org/apache/commons/pool2/impl/TestAbandonedObjectPool.java > > +++ > b/src/test/java/org/apache/commons/pool2/impl/TestAbandonedObjectPool.java > > @@ -183,7 +183,7 @@ public class TestAbandonedObjectPool { > > } > > > > @Override > > - public void destroyObject(final PooledObject<PooledTestObject> > obj, final DestroyMode mode) throws Exception { > > + public void destroyObject(final PooledObject<PooledTestObject> > obj, final DestroyMode destroyMode) throws Exception { > > obj.getObject().setActive(false); > > // while destroying instances, yield control to other > threads > > // helps simulate threading errors > > @@ -191,7 +191,7 @@ public class TestAbandonedObjectPool { > > if (destroyLatency != 0) { > > Thread.sleep(destroyLatency); > > } > > - obj.getObject().destroy(mode); > > + obj.getObject().destroy(destroyMode); > > } > > > > @Override > > diff --git > a/src/test/java/org/apache/commons/pool2/impl/TestGenericKeyedObjectPool.java > b/src/test/java/org/apache/commons/pool2/impl/TestGenericKeyedObjectPool.java > > index 39e898c..cd36876 100644 > > --- > a/src/test/java/org/apache/commons/pool2/impl/TestGenericKeyedObjectPool.java > > +++ > b/src/test/java/org/apache/commons/pool2/impl/TestGenericKeyedObjectPool.java > > @@ -20,6 +20,7 @@ package org.apache.commons.pool2.impl; > > > > import static org.junit.jupiter.api.Assertions.assertEquals; > > import static org.junit.jupiter.api.Assertions.assertFalse; > > +import static org.junit.jupiter.api.Assertions.assertNotEquals; > > import static org.junit.jupiter.api.Assertions.assertNotNull; > > import static org.junit.jupiter.api.Assertions.assertNotSame; > > import static org.junit.jupiter.api.Assertions.assertSame; > > @@ -906,6 +907,18 @@ public class TestGenericKeyedObjectPool extends > TestKeyedObjectPool { > > } > > > > @Test > > + public void testAppendStats() { > > + assertFalse(gkoPool.getMessageStatistics()); > > + assertEquals("foo", (gkoPool.appendStats("foo"))); > > + try (final GenericKeyedObjectPool<?, ?> pool = new > GenericKeyedObjectPool<>(new SimpleFactory<>())) { > > + pool.setMessagesStatistics(true); > > + assertNotEquals("foo", (pool.appendStats("foo"))); > > + pool.setMessagesStatistics(false); > > + assertEquals("foo", (pool.appendStats("foo"))); > > + } > > + } > > + > > + @Test > > @Timeout(value = 60000, unit = TimeUnit.MILLISECONDS) > > public void testBlockedKeyDoesNotBlockPool() throws Exception { > > gkoPool.setBlockWhenExhausted(true); > > @@ -1025,6 +1038,7 @@ public class TestGenericKeyedObjectPool extends > TestKeyedObjectPool { > > gkoPool.close(); > > } > > > > + > > /** > > * Test to make sure that clearOldest does not destroy instances > that have been checked out. > > * > > @@ -1061,7 +1075,6 @@ public class TestGenericKeyedObjectPool extends > TestKeyedObjectPool { > > } > > } > > > > - > > // POOL-259 > > @Test > > public void testClientWaitStats() throws Exception { > > @@ -1483,6 +1496,27 @@ public class TestGenericKeyedObjectPool extends > TestKeyedObjectPool { > > > > @Test > > @Timeout(value = 60000, unit = TimeUnit.MILLISECONDS) > > + public void testExceptionInValidationDuringEviction() throws > Exception { > > + gkoPool.setMaxIdlePerKey(1); > > + gkoPool.setMinEvictableIdleTime(Duration.ZERO); > > + gkoPool.setTestWhileIdle(true); > > + > > + final String obj = gkoPool.borrowObject("one"); > > + gkoPool.returnObject("one", obj); > > + > > + simpleFactory.setThrowExceptionOnValidate(true); > > + try { > > + gkoPool.evict(); > > + fail("Expecting RuntimeException"); > > + } catch (final RuntimeException e) { > > + // expected > > + } > > + assertEquals(0, gkoPool.getNumActive()); > > + assertEquals(0, gkoPool.getNumIdle()); > > + } > > + > > + @Test > > + @Timeout(value = 60000, unit = TimeUnit.MILLISECONDS) > > public void testExceptionOnActivateDuringBorrow() throws Exception > { > > final String obj1 = gkoPool.borrowObject("one"); > > final String obj2 = gkoPool.borrowObject("one"); > > @@ -1514,6 +1548,7 @@ public class TestGenericKeyedObjectPool extends > TestKeyedObjectPool { > > assertEquals(0, gkoPool.getNumIdle()); > > } > > > > + > > @Test > > @Timeout(value = 60000, unit = TimeUnit.MILLISECONDS) > > public void testExceptionOnDestroyDuringBorrow() throws Exception { > > @@ -1550,7 +1585,6 @@ public class TestGenericKeyedObjectPool extends > TestKeyedObjectPool { > > assertEquals(0, gkoPool.getNumIdle()); > > } > > > > - > > @Test > > @Timeout(value = 60000, unit = TimeUnit.MILLISECONDS) > > public void testExceptionOnPassivateDuringReturn() throws > Exception { > > @@ -1563,27 +1597,6 @@ public class TestGenericKeyedObjectPool extends > TestKeyedObjectPool { > > > > @Test > > @Timeout(value = 60000, unit = TimeUnit.MILLISECONDS) > > - public void testExceptionInValidationDuringEviction() throws > Exception { > > - gkoPool.setMaxIdlePerKey(1); > > - gkoPool.setMinEvictableIdleTime(Duration.ZERO); > > - gkoPool.setTestWhileIdle(true); > > - > > - final String obj = gkoPool.borrowObject("one"); > > - gkoPool.returnObject("one", obj); > > - > > - simpleFactory.setThrowExceptionOnValidate(true); > > - try { > > - gkoPool.evict(); > > - fail("Expecting RuntimeException"); > > - } catch (final RuntimeException e) { > > - // expected > > - } > > - assertEquals(0, gkoPool.getNumActive()); > > - assertEquals(0, gkoPool.getNumIdle()); > > - } > > - > > - @Test > > - @Timeout(value = 60000, unit = TimeUnit.MILLISECONDS) > > public void testFIFO() throws Exception { > > gkoPool.setLifo(false); > > final String key = "key"; > > @@ -1600,6 +1613,11 @@ public class TestGenericKeyedObjectPool extends > TestKeyedObjectPool { > > assertEquals( "key4", gkoPool.borrowObject(key),"new-4"); > > } > > > > + @Test > > + public void testGetStatsString() { > > + assertNotNull((gkoPool.getStatsString())); > > + } > > + > > /** > > * Verify that threads waiting on a depleted pool get served when > a checked out object is > > * invalidated. > > 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 1014588..fe646a3 100644 > > --- > a/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java > > +++ > b/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java > > @@ -20,6 +20,7 @@ package org.apache.commons.pool2.impl; > > > > import static org.junit.jupiter.api.Assertions.assertEquals; > > import static org.junit.jupiter.api.Assertions.assertFalse; > > +import static org.junit.jupiter.api.Assertions.assertNotEquals; > > import static org.junit.jupiter.api.Assertions.assertNotNull; > > import static org.junit.jupiter.api.Assertions.assertNull; > > import static org.junit.jupiter.api.Assertions.assertThrows; > > @@ -58,6 +59,7 @@ import org.apache.commons.pool2.VisitTracker; > > import org.apache.commons.pool2.VisitTrackerFactory; > > import org.apache.commons.pool2.Waiter; > > import org.apache.commons.pool2.WaiterFactory; > > +import > org.apache.commons.pool2.impl.TestGenericKeyedObjectPool.SimpleFactory; > > import org.junit.jupiter.api.AfterEach; > > import org.junit.jupiter.api.BeforeEach; > > import org.junit.jupiter.api.Test; > > @@ -83,7 +85,9 @@ public class TestGenericObjectPool extends > TestBaseObjectPool { > > } else { > > genericObjectPool.evict(); > > } > > - } catch (final Exception e) { /* Ignore */} > > + } catch (final Exception e) { > > + // Ignore. > > + } > > } > > } > > > > @@ -999,6 +1003,18 @@ public class TestGenericObjectPool extends > TestBaseObjectPool { > > assertEquals( 0, genericObjectPool.getNumActive(),"should be > zero active"); > > } > > > > + @Test > > + public void testAppendStats() { > > + assertFalse(genericObjectPool.getMessageStatistics()); > > + assertEquals("foo", (genericObjectPool.appendStats("foo"))); > > + try (final GenericObjectPool<?> pool = new > GenericObjectPool<>(new SimpleFactory())) { > > + pool.setMessagesStatistics(true); > > + assertNotEquals("foo", (pool.appendStats("foo"))); > > + pool.setMessagesStatistics(false); > > + assertEquals("foo", (pool.appendStats("foo"))); > > + } > > + } > > + > > /* > > * Note: This test relies on timing for correct execution. There > *should* be > > * enough margin for this to work correctly on most (all?) systems > but be > > @@ -1185,6 +1201,7 @@ public class TestGenericObjectPool extends > TestBaseObjectPool { > > } > > } > > > > + > > /** > > * POOL-231 - verify that concurrent invalidates of the same > object do not > > * corrupt pool destroyCount. > > @@ -1238,7 +1255,6 @@ public class TestGenericObjectPool extends > TestBaseObjectPool { > > assertEquals(nIterations, > genericObjectPool.getDestroyedCount()); > > } > > > > - > > @Test > > public void testConstructorNullFactory() { > > // add dummy assert (won't be invoked because of IAE) to avoid > "unused" warning > > @@ -1916,6 +1932,14 @@ public class TestGenericObjectPool extends > TestBaseObjectPool { > > } > > } > > > > + @Test > > + public void testGetStatsString() { > > + try (final GenericObjectPool<String> pool = new > GenericObjectPool<>( > > + new > TestSynchronizedPooledObjectFactory<>(createNullPooledObjectFactory()))) { > > + assertNotNull(pool.getStatsString()); > > + } > > + } > > + > > /** > > * Verify that threads waiting on a depleted pool get served when > a checked out object is > > * invalidated. > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > >
