This is an automated email from the ASF dual-hosted git repository.
psteitz pushed a commit to branch POOL_2_X
in repository https://gitbox.apache.org/repos/asf/commons-pool.git
The following commit(s) were added to refs/heads/POOL_2_X by this push:
new 1c4b1deb JIRA: POOL-420 Correct time-keeping in GKOP create to
eliminate possibility of configured maxWait on borrow to be exceeded. Also add
Duration-based borrow method and change addObject to return immediately (rather
than potentially waiting up to maxWaitDuration) if the pool has no capacity to
create when this method is invoked.
new 5769a311 Merge branch 'POOL_2_X' of
https://github.com/apache/commons-pool into POOL_2_X
1c4b1deb is described below
commit 1c4b1deb3656e611e07161b22ec6ea14cf6ea70a
Author: Phil Steitz <phil.ste...@gmail.com>
AuthorDate: Mon May 19 16:44:18 2025 -0700
JIRA: POOL-420
Correct time-keeping in GKOP create to eliminate possibility of configured
maxWait on borrow to be exceeded.
Also add Duration-based borrow method and change addObject to return
immediately
(rather than potentially waiting up to maxWaitDuration) if the pool has no
capacity
to create when this method is invoked.
---
src/changes/changes.xml | 3 +
.../commons/pool2/impl/BaseGenericObjectPool.java | 26 +++++
.../commons/pool2/impl/GenericKeyedObjectPool.java | 124 ++++++++++++++++++---
.../commons/pool2/impl/GenericObjectPool.java | 11 +-
.../pool2/impl/TestGenericKeyedObjectPool.java | 52 ++++++++-
5 files changed, 183 insertions(+), 33 deletions(-)
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
index d56cd543..c978ef83 100644
--- a/src/changes/changes.xml
+++ b/src/changes/changes.xml
@@ -47,6 +47,9 @@ The <action> type attribute can be add,update,fix,remove.
<body>
<release version="2.12.2" date="YYYY-MM-DD" description="This is a feature
and maintenance release. Java 8 or later is required.">
<!-- FIX -->
+ <action type="fix" issue="POOL-420" dev="psteitz" due-to="Phil Steitz">The
maximum wait time for GenericKeyedObjectPool.borrowObject(*) may exceed
configured maximum wait time. This is the same issue as POOL-418, but for GKOP.
+ Also included in this fix is a change to addObject that prevents it
from waiting for capacity to create. That method now returns immediately when
there is no capcity to add to the pool under the given key.
+ </action>
<action type="fix" dev="ggregory" due-to="Gary Gregory">Remove -nouses
directive from maven-bundle-plugin. OSGi package imports now state 'uses'
definitions for package imports, this doesn't affect JPMS (from
org.apache.commons:commons-parent:80).</action>
<action type="fix" issue="POOL-418" dev="ggregory" due-to="Gary
Gregory">The maximum wait time for GenericObjectPool.borrowObject(*) may exceed
expectations due to a spurious thread wakeup.
The remaining duration was incorrectly calculated and the method did not
end up waiting long enough.
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 4efde590..573bf006 100644
--- a/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java
+++ b/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java
@@ -1938,6 +1938,32 @@ public abstract class BaseGenericObjectPool<T> extends
BaseObject implements Aut
}
}
+ /**
+ * Returns the duration since the given start time.
+ *
+ * @param startInstant the start time
+ * @return the duration since the given start time
+ */
+ final Duration durationSince(final Instant startInstant) {
+ return Duration.between(startInstant, Instant.now());
+ }
+
+ /**
+ * Waits for notification on the given object for the specified duration.
+ * Duration.ZERO causes the thread to wait indefinitely.
+ *
+ * @param obj the object to wait on
+ * @param duration the duration to wait
+ * @throws InterruptedException if interrupted while waiting
+ * @throws IllegalArgumentException if the duration is negative
+ */
+ final void wait(final Object obj, final Duration duration) throws
InterruptedException {
+ if (!duration.isNegative()) {
+ obj.wait(duration.toMillis(), duration.getNano() % 1_000_000);
+ }
+ }
+
+
/**
* 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 df367e32..c6ce6f62 100644
--- a/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
+++ b/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
@@ -29,7 +29,6 @@ import java.util.NoSuchElementException;
import java.util.Objects;
import java.util.TreeMap;
import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicLong;
@@ -330,9 +329,18 @@ public class GenericKeyedObjectPool<K, T> extends
BaseGenericObjectPool<T>
@Override
public void addObject(final K key) throws Exception {
assertOpen();
- register(key);
+ final ObjectDeque<T> objectDeque = register(key);
try {
- addIdleObject(key, create(key));
+ // Attempt create and add only if there is capacity to add
+ // > to the overall instance count
+ // > to the pool under the key
+ final int maxtTotalPerKey = getMaxTotalPerKey();
+ final int maxTotal = getMaxTotal();
+ if ((maxTotal < 0 || getNumActive() + getNumIdle() < maxTotal)
+ && (maxtTotalPerKey < 0 || objectDeque.allObjects.size() <
maxtTotalPerKey)) {
+ // Attempt to create and add a new instance under key
+ addIdleObject(key, create(key, getMaxWaitDuration()));
+ }
} finally {
deregister(key);
}
@@ -400,8 +408,8 @@ public class GenericKeyedObjectPool<K, T> extends
BaseGenericObjectPool<T>
* </p>
*
* @param key pool key
- * @param borrowMaxWaitMillis The time to wait in milliseconds for an
object
- * to become available
+ * @param maxWaitDuration The time to wait for an object to become
+ * available
*
* @return object instance from the keyed pool
* @throws NoSuchElementException if a keyed object instance cannot be
@@ -409,10 +417,12 @@ public class GenericKeyedObjectPool<K, T> extends
BaseGenericObjectPool<T>
*
* @throws Exception if a keyed object instance cannot be returned due to
an
* error
+ * @since 2.12.2
*/
- public T borrowObject(final K key, final long borrowMaxWaitMillis) throws
Exception {
+ public T borrowObject(final K key, final Duration maxWaitDuration) throws
Exception {
assertOpen();
-
+ final Instant startInstant = Instant.now();
+ Duration remainingWaitDuration = maxWaitDuration;
final AbandonedConfig ac = this.abandonedConfig;
if (ac != null && ac.getRemoveAbandonedOnBorrow() && getNumIdle() < 2
&&
getNumActive() > getMaxTotal() - 3) {
@@ -426,27 +436,28 @@ public class GenericKeyedObjectPool<K, T> extends
BaseGenericObjectPool<T>
final boolean blockWhenExhausted = getBlockWhenExhausted();
boolean create;
- final Instant waitTime = Instant.now();
final ObjectDeque<T> objectDeque = register(key);
try {
while (p == null) {
+ remainingWaitDuration =
maxWaitDuration.minus(durationSince(startInstant));
create = false;
p = objectDeque.getIdleObjects().pollFirst();
if (p == null) {
- p = create(key);
+ p = create(key, remainingWaitDuration);
if (!PooledObject.isNull(p)) {
create = true;
}
+ remainingWaitDuration =
maxWaitDuration.minus(durationSince(startInstant));
}
if (blockWhenExhausted) {
if (PooledObject.isNull(p)) {
- p = borrowMaxWaitMillis < 0 ?
objectDeque.getIdleObjects().takeFirst()
- :
objectDeque.getIdleObjects().pollFirst(borrowMaxWaitMillis,
TimeUnit.MILLISECONDS);
+ p = maxWaitDuration.isNegative() ?
objectDeque.getIdleObjects().takeFirst()
+ :
objectDeque.getIdleObjects().pollFirst(remainingWaitDuration);
}
if (PooledObject.isNull(p)) {
throw new NoSuchElementException(appendStats(
- "Timeout waiting for idle object,
borrowMaxWaitMillis=" + borrowMaxWaitMillis));
+ "Timeout waiting for idle object,
borrowMaxWaitMillis=" + maxWaitDuration.toMillis()));
}
} else if (PooledObject.isNull(p)) {
throw new NoSuchElementException(appendStats("Pool
exhausted"));
@@ -466,7 +477,8 @@ public class GenericKeyedObjectPool<K, T> extends
BaseGenericObjectPool<T>
}
p = null;
if (create) {
- final NoSuchElementException nsee = new
NoSuchElementException(appendStats("Unable to activate object"));
+ final NoSuchElementException nsee = new
NoSuchElementException(
+ appendStats("Unable to activate object"));
nsee.initCause(e);
throw nsee;
}
@@ -502,11 +514,78 @@ public class GenericKeyedObjectPool<K, T> extends
BaseGenericObjectPool<T>
deregister(key);
}
- updateStatsBorrow(p, Duration.between(waitTime, Instant.now()));
+ updateStatsBorrow(p, Duration.between(startInstant, Instant.now()));
return p.getObject();
}
+
+ /**
+ * Borrows an object from the sub-pool associated with the given key using
+ * the specified waiting time which only applies if
+ * {@link #getBlockWhenExhausted()} is true.
+ * <p>
+ * If there is one or more idle instances available in the sub-pool
+ * associated with the given key, then an idle instance will be selected
+ * based on the value of {@link #getLifo()}, activated and returned. If
+ * activation fails, or {@link #getTestOnBorrow() testOnBorrow} is set to
+ * {@code true} and validation fails, the 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 sub-pool associated with
+ * the given key, behavior depends on the {@link #getMaxTotalPerKey()
+ * maxTotalPerKey}, {@link #getMaxTotal() maxTotal}, and (if applicable)
+ * {@link #getBlockWhenExhausted()} and the value passed in to the
+ * {@code borrowMaxWaitMillis} parameter. If the number of instances
checked
+ * out from the sub-pool under the given key is less than
+ * {@code maxTotalPerKey} and the total number of instances in
+ * circulation (under all keys) is less than {@code maxTotal}, a new
+ * instance is created, activated and (if applicable) validated and
returned
+ * to the caller. If validation fails, a {@code NoSuchElementException}
+ * will be thrown. If the factory returns null when creating an instance,
+ * a {@code NullPointerException} is thrown.
+ * </p>
+ * <p>
+ * If the associated sub-pool is exhausted (no available idle instances and
+ * no capacity to create new ones), this method will either block
+ * ({@link #getBlockWhenExhausted()} is true) or throw a
+ * {@code NoSuchElementException}
+ * ({@link #getBlockWhenExhausted()} is false).
+ * The length of time that this method will block when
+ * {@link #getBlockWhenExhausted()} is true is determined by the value
+ * passed in to the {@code borrowMaxWait} parameter.
+ * </p>
+ * <p>
+ * When {@code maxTotal} is set to a positive value and this method is
+ * invoked when at the limit with no idle instances available under the
requested
+ * key, an attempt is made to create room by clearing the oldest 15% of the
+ * elements from the keyed sub-pools.
+ * </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 key pool key
+ * @param maxWaitMillis The time to wait in milliseconds for an object to
become
+ * available
+ *
+ * @return object instance from the keyed pool
+ * @throws NoSuchElementException if a keyed object instance cannot be
+ * returned because the pool is exhausted.
+ *
+ * @throws Exception if a keyed object instance cannot be returned due to
an
+ * error
+ */
+
+ public T borrowObject(final K key, final long maxWaitMillis) throws
Exception {
+ return borrowObject(key, Duration.ofMillis(maxWaitMillis));
+ }
+
/**
* Calculate the number of objects that need to be created to attempt to
* maintain the minimum number of idle objects while not exceeded the
limits
@@ -708,11 +787,16 @@ public class GenericKeyedObjectPool<K, T> extends
BaseGenericObjectPool<T>
/**
* Creates a new pooled object or null.
*
- * @param key Key associated with new pooled object.
+ * @param key Key associated with new pooled object.
+ * @param maxWaitDuration The time to wait in this method. If negative or
ZERO,
+ * this method may wait indefinitely.
* @return The new, wrapped pooled object. May return null.
* @throws Exception If the objection creation fails.
*/
- private PooledObject<T> create(final K key) throws Exception {
+ private PooledObject<T> create(final K key, Duration maxWaitDuration)
throws Exception {
+ final Instant startInstant = Instant.now();
+ Duration remainingWaitDuration = maxWaitDuration.isNegative() ?
Duration.ZERO : maxWaitDuration;
+
int maxTotalPerKeySave = getMaxTotalPerKey(); // Per key
if (maxTotalPerKeySave < 0) {
maxTotalPerKeySave = Integer.MAX_VALUE;
@@ -744,6 +828,7 @@ public class GenericKeyedObjectPool<K, T> extends
BaseGenericObjectPool<T>
// call the factory
Boolean create = null;
while (create == null) {
+ remainingWaitDuration = maxWaitDuration.isNegative() ?
Duration.ZERO : maxWaitDuration.minus(durationSince(startInstant));
synchronized (objectDeque.makeObjectCountLock) {
final long newCreateCount =
objectDeque.getCreateCount().incrementAndGet();
// Check against the per key limit
@@ -762,7 +847,10 @@ public class GenericKeyedObjectPool<K, T> extends
BaseGenericObjectPool<T>
// bring the pool to capacity. Those calls might also
// fail so wait until they complete and then re-test if
// the pool is at capacity or not.
- objectDeque.makeObjectCountLock.wait();
+ if (!remainingWaitDuration.isNegative()) {
+
objectDeque.makeObjectCountLock.wait(remainingWaitDuration.toMillis(),
+ remainingWaitDuration.getNano() %
1_000_000);
+ }
}
} else {
// The pool is not at capacity. Create a new object.
@@ -1571,7 +1659,7 @@ public class GenericKeyedObjectPool<K, T> extends
BaseGenericObjectPool<T>
try {
// If there is no capacity to add, create will return null
// and addIdleObject will no-op.
- addIdleObject(mostLoadedKey, create(mostLoadedKey));
+ addIdleObject(mostLoadedKey, create(mostLoadedKey,
Duration.ZERO));
} catch (final Exception e) {
swallowException(e);
} finally {
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 8421062a..3a88431a 100644
--- a/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java
+++ b/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java
@@ -84,12 +84,6 @@ public class GenericObjectPool<T> extends
BaseGenericObjectPool<T>
private static final String ONAME_BASE =
"org.apache.commons.pool2:type=GenericObjectPool,name=";
- private static void wait(final Object obj, final Duration duration) throws
InterruptedException {
- if (!duration.isNegative()) {
- obj.wait(duration.toMillis(), duration.getNano() % 1_000_000);
- }
- }
-
private volatile String factoryType;
private volatile int maxIdle = GenericObjectPoolConfig.DEFAULT_MAX_IDLE;
@@ -491,7 +485,7 @@ public class GenericObjectPool<T> extends
BaseGenericObjectPool<T>
* If the factory makeObject returns null, this method throws a
NullPointerException.
* </p>
*
- * @param maxWaitDuration The time to wait for an object to become
available.
+ * @param maxWaitDuration The time to wait for capacity to create
* @return The new wrapped pooled object or null.
* @throws Exception if the object factory's {@code makeObject} fails
*/
@@ -600,9 +594,6 @@ public class GenericObjectPool<T> extends
BaseGenericObjectPool<T>
}
}
- private Duration durationSince(final Instant startInstant) {
- return Duration.between(startInstant, Instant.now());
- }
/**
* Tries to ensure that {@code idleCount} idle instances exist in the pool.
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 52f58646..82aff064 100644
---
a/src/test/java/org/apache/commons/pool2/impl/TestGenericKeyedObjectPool.java
+++
b/src/test/java/org/apache/commons/pool2/impl/TestGenericKeyedObjectPool.java
@@ -28,6 +28,7 @@ import static org.junit.jupiter.api.Assertions.fail;
import java.lang.management.ManagementFactory;
import java.time.Duration;
+import java.time.Instant;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashSet;
@@ -315,11 +316,11 @@ public class TestGenericKeyedObjectPool extends
AbstractTestKeyedObjectPool {
@Override
public void run() {
- try {
- pool.returnObject(key, pool.borrowObject(key));
- } catch (final Exception e) {
- // Ignore
- }
+ try {
+ pool.returnObject(key, pool.borrowObject(key));
+ } catch (Exception e) {
+ // Ignore
+ }
}
}
@@ -2045,6 +2046,47 @@ public class TestGenericKeyedObjectPool extends
AbstractTestKeyedObjectPool {
assertThrows(NoSuchElementException.class, () ->
gkoPool.borrowObject("a"));
}
+ /**
+ * JIRA: POOL-420 (clone of POOL-418 for GKOP)
+ *
+ * Test to make sure that a client thread that triggers a create that
fails does
+ * not wait longer than the maxWait time.
+ *
+ * Bug was that the time spent waiting for the create to complete was not
being
+ * counted against the maxWait time.
+ */
+ @Test
+ public void testMaxWaitTimeOutOnTime() throws Exception {
+ final Duration maxWaitDuration = Duration.ofSeconds(1);
+ final SimpleFactory<String> factory = new SimpleFactory<>();
+ factory.makeLatency = 500;
+ factory.setValidationEnabled(true); // turn on factory-level validatiom
+ factory.setValid(false); // make validation fail uniformly
+ final GenericKeyedObjectPool<String, String> pool = new
GenericKeyedObjectPool<>(factory);
+
+ pool.setBlockWhenExhausted(true);
+ pool.setMaxWait(maxWaitDuration);
+ pool.setMaxTotalPerKey(1);
+ pool.setMaxTotal(1);
+ pool.setTestOnCreate(true);
+ final Instant startTime = Instant.now();
+
+ // Try to borrow an object. Validation will fail.
+ // Then we will wait on the pool.
+ try {
+ pool.borrowObject("a");
+ } catch (NoSuchElementException ex) {
+ // expected
+ }
+
+ // Should have timeed out after 1000 ms from the start time
+ final Duration duration = Duration.between(startTime, Instant.now());
+ assertTrue(duration.toMillis() < maxWaitDuration.toMillis() + 10, //
allow for some timing delay
+ "Thread A should have timed out after " +
maxWaitDuration.toMillis() + " ms, but took " + duration.toMillis() + " ms");
+ pool.close();
+ }
+
+
/*
* Test multi-threaded pool access. Multiple keys, multiple threads, but
maxActive only allows half the threads to succeed.
*
