This is an automated email from the ASF dual-hosted git repository.
ggregory 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 e973e08b GenericObjectPool.borrowObject(Duration) doesn't obey its
borrowMaxWait Duration argument when the argument is different from
GenericObjectPool.getMaxWaitDuration()
e973e08b is described below
commit e973e08bccc005b6c9ea0d25699a5d28853588ed
Author: Gary Gregory <[email protected]>
AuthorDate: Sun Nov 24 17:50:55 2024 -0500
GenericObjectPool.borrowObject(Duration) doesn't obey its borrowMaxWait
Duration argument when the argument is different from
GenericObjectPool.getMaxWaitDuration()
---
src/changes/changes.xml | 1 +
.../commons/pool2/impl/GenericObjectPool.java | 42 +++++++++--------
.../commons/pool2/impl/TestGenericObjectPool.java | 52 +++++++++++++++++++++-
3 files changed, 71 insertions(+), 24 deletions(-)
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
index fb499780..6c7ce5d0 100644
--- a/src/changes/changes.xml
+++ b/src/changes/changes.xml
@@ -48,6 +48,7 @@ The <action> type attribute can be add,update,fix,remove.
<release version="2.12.1" date="YYYY-MM-DD" description="This is a feature
and maintenance release (Java 8 or above).">
<!-- FIX -->
<action type="fix" dev="ggregory" due-to="Gary Gregory">Use
java.time.Instant precision in
org.apache.commons.pool2.impl.ThrowableCallStack.Snapshot throwable
message.</action>
+ <action type="fix" dev="ggregory" due-to="Gary
Gregory">GenericObjectPool.borrowObject(Duration) doesn't obey its
borrowMaxWait Duration argument when the argument is different from
GenericObjectPool.getMaxWaitDuration().</action>
<!-- ADD -->
<!-- UPDATE -->
<action type="update" dev="ggregory">Bump
org.apache.commons:commons-parent from 62 to 73.</action>
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 94c97c38..f4a7c9d2 100644
--- a/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java
+++ b/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java
@@ -218,7 +218,7 @@ public class GenericObjectPool<T> extends
BaseGenericObjectPool<T>
if (factory == null) {
throw new IllegalStateException("Cannot add objects without a
factory.");
}
- addIdleObject(create());
+ addIdleObject(create(getMaxWaitDuration()));
}
/**
@@ -282,38 +282,35 @@ public class GenericObjectPool<T> extends
BaseGenericObjectPool<T>
*/
public T borrowObject(final Duration borrowMaxWaitDuration) throws
Exception {
assertOpen();
-
+ final Instant startInstant = Instant.now();
+ final boolean negativeDuration = borrowMaxWaitDuration.isNegative();
+ Duration remainingWaitDuration = borrowMaxWaitDuration;
final AbandonedConfig ac = this.abandonedConfig;
- if (ac != null && ac.getRemoveAbandonedOnBorrow() && getNumIdle() < 2
&&
- getNumActive() > getMaxTotal() - 3) {
+ if (ac != null && ac.getRemoveAbandonedOnBorrow() && getNumIdle() < 2
&& getNumActive() > getMaxTotal() - 3) {
removeAbandoned(ac);
}
-
PooledObject<T> p = null;
-
// Get local copy of current config so it is consistent for entire
// method execution
final boolean blockWhenExhausted = getBlockWhenExhausted();
-
boolean create;
- final Instant waitTime = Instant.now();
-
while (p == null) {
+ remainingWaitDuration =
remainingWaitDuration.minus(durationSince(startInstant));
create = false;
p = idleObjects.pollFirst();
if (p == null) {
- p = create();
+ p = create(remainingWaitDuration);
if (!PooledObject.isNull(p)) {
create = true;
}
}
if (blockWhenExhausted) {
if (PooledObject.isNull(p)) {
- p = borrowMaxWaitDuration.isNegative() ?
idleObjects.takeFirst() : idleObjects.pollFirst(borrowMaxWaitDuration);
+ p = negativeDuration ? idleObjects.takeFirst() :
idleObjects.pollFirst(remainingWaitDuration);
}
if (PooledObject.isNull(p)) {
throw new NoSuchElementException(appendStats(
- "Timeout waiting for idle object,
borrowMaxWaitDuration=" + borrowMaxWaitDuration));
+ "Timeout waiting for idle object,
borrowMaxWaitDuration=" + remainingWaitDuration));
}
} else if (PooledObject.isNull(p)) {
throw new NoSuchElementException(appendStats("Pool
exhausted"));
@@ -321,7 +318,6 @@ public class GenericObjectPool<T> extends
BaseGenericObjectPool<T>
if (!p.allocate()) {
p = null;
}
-
if (!PooledObject.isNull(p)) {
try {
factory.activateObject(p);
@@ -366,12 +362,14 @@ public class GenericObjectPool<T> extends
BaseGenericObjectPool<T>
}
}
}
-
- updateStatsBorrow(p, Duration.between(waitTime, Instant.now()));
-
+ updateStatsBorrow(p, durationSince(startInstant));
return p.getObject();
}
+ private Duration durationSince(final Instant startInstant) {
+ return Duration.between(startInstant, Instant.now());
+ }
+
/**
* Borrows an object from the pool using the specific waiting time which
only
* applies if {@link #getBlockWhenExhausted()} is true.
@@ -503,10 +501,11 @@ 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.
* @return The new wrapped pooled object or null.
* @throws Exception if the object factory's {@code makeObject} fails
*/
- private PooledObject<T> create() throws Exception {
+ private PooledObject<T> create(final Duration maxWaitDuration) throws
Exception {
int localMaxTotal = getMaxTotal();
// This simplifies the code later in this method
if (localMaxTotal < 0) {
@@ -514,8 +513,7 @@ public class GenericObjectPool<T> extends
BaseGenericObjectPool<T>
}
final Instant localStartInstant = Instant.now();
- final Duration maxWaitDurationRaw = getMaxWaitDuration();
- final Duration localMaxWaitDuration = maxWaitDurationRaw.isNegative()
? Duration.ZERO : maxWaitDurationRaw;
+ final Duration localMaxWaitDuration = maxWaitDuration.isNegative() ?
Duration.ZERO : maxWaitDuration;
// Flag that indicates if create should:
// - TRUE: call the factory to create an object
@@ -549,9 +547,9 @@ public class GenericObjectPool<T> extends
BaseGenericObjectPool<T>
}
}
- // Do not block more if localMaxWaitDuration is set.
+ // Do not block more if localMaxWaitDuration > 0.
if (create == null &&
localMaxWaitDuration.compareTo(Duration.ZERO) > 0 &&
- Duration.between(localStartInstant,
Instant.now()).compareTo(localMaxWaitDuration) >= 0) {
+
durationSince(localStartInstant).compareTo(localMaxWaitDuration) >= 0) {
create = Boolean.FALSE;
}
}
@@ -636,7 +634,7 @@ public class GenericObjectPool<T> extends
BaseGenericObjectPool<T>
}
while (idleObjects.size() < idleCount) {
- final PooledObject<T> p = create();
+ final PooledObject<T> p = create(getMaxWaitDuration());
if (PooledObject.isNull(p)) {
// Can't create objects, no reason to think another call to
// create will work. Give up.
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 648466f6..f12ba55f 100644
--- a/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java
+++ b/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java
@@ -50,6 +50,7 @@ import java.util.concurrent.atomic.AtomicInteger;
import javax.management.MBeanServer;
import javax.management.ObjectName;
+import org.apache.commons.lang3.time.DurationUtils;
import org.apache.commons.pool2.BasePooledObjectFactory;
import org.apache.commons.pool2.ObjectPool;
import org.apache.commons.pool2.PoolUtils;
@@ -1074,6 +1075,53 @@ public class TestGenericObjectPool extends
TestBaseObjectPool {
}
}
+ @Test/* maxWaitMillis x2 + padding */
+ @Timeout(value = 1200, unit = TimeUnit.MILLISECONDS)
+ public void testBorrowObjectOverrideMaxWaitLarge() throws Exception {
+ try (final GenericObjectPool<String> pool = new
GenericObjectPool<>(createSlowObjectFactory(60_000))) {
+ pool.setMaxTotal(1);
+ pool.setMaxWait(Duration.ofMillis(1_000)); // large
+ pool.setBlockWhenExhausted(false);
+ // thread1 tries creating a slow object to make pool full.
+ final WaitingTestThread thread1 = new WaitingTestThread(pool, 0);
+ thread1.start();
+ // Wait for thread1's reaching to create().
+ Thread.sleep(100);
+ // The test needs to make sure a wait happens in create().
+ final Duration d = DurationUtils.of(() ->
assertThrows(NoSuchElementException.class, () ->
pool.borrowObject(Duration.ofMillis(1)),
+ "borrowObject must fail quickly due to timeout
parameter"));
+ final long millis = d.toMillis();
+ final long nanos = d.toNanos();
+ assertTrue(nanos > 0, () -> "borrowObject(Duration) argument not
respected: " + nanos);
+ assertTrue(millis >= 0, () -> "borrowObject(Duration) argument not
respected: " + millis); // not > 0 to account for spurious waits
+ assertTrue(millis < 50, () -> "borrowObject(Duration) argument not
respected: " + millis);
+ }
+ }
+
+ @Test/* maxWaitMillis x2 + padding */
+ @Timeout(value = 1200, unit = TimeUnit.MILLISECONDS)
+ public void testBorrowObjectOverrideMaxWaitSmall() throws Exception {
+ try (final GenericObjectPool<String> pool = new
GenericObjectPool<>(createSlowObjectFactory(60_000))) {
+ pool.setMaxTotal(1);
+ pool.setMaxWait(Duration.ofMillis(1)); // small
+ pool.setBlockWhenExhausted(false);
+ // thread1 tries creating a slow object to make pool full.
+ final WaitingTestThread thread1 = new WaitingTestThread(pool, 0);
+ thread1.start();
+ // Wait for thread1's reaching to create().
+ Thread.sleep(100);
+ // The test needs to make sure a wait happens in create().
+ final Duration d = DurationUtils.of(() ->
assertThrows(NoSuchElementException.class, () ->
pool.borrowObject(Duration.ofMillis(500)),
+ "borrowObject must fail slowly due to timeout parameter"));
+ final long millis = d.toMillis();
+ final long nanos = d.toNanos();
+ assertTrue(nanos > 0, () -> "borrowObject(Duration) argument not
respected: " + nanos);
+ assertTrue(millis >= 0, () -> "borrowObject(Duration) argument not
respected: " + millis); // not > 0 to account for spurious waits
+ assertTrue(millis < 600, () -> "borrowObject(Duration) argument
not respected: " + millis);
+ assertTrue(millis > 490, () -> "borrowObject(Duration) argument
not respected: " + millis);
+ }
+ }
+
@Test
public void testBorrowTimings() throws Exception {
// Borrow
@@ -2648,7 +2696,7 @@ public class TestGenericObjectPool extends
TestBaseObjectPool {
Thread.sleep(100);
// another one tries borrowObject. It should return within
maxWaitMillis.
assertThrows(NoSuchElementException.class, () ->
createSlowObjectFactoryPool.borrowObject(maxWaitDuration),
- "borrowObject must fail due to timeout by maxWaitMillis");
+ "borrowObject must fail due to timeout by maxWait");
assertTrue(thread1.isAlive());
}
}
@@ -2667,7 +2715,7 @@ public class TestGenericObjectPool extends
TestBaseObjectPool {
Thread.sleep(100);
// another one tries borrowObject. It should return within
maxWaitMillis.
assertThrows(NoSuchElementException.class, () ->
createSlowObjectFactoryPool.borrowObject(maxWaitMillis),
- "borrowObject must fail due to timeout by maxWaitMillis");
+ "borrowObject must fail due to timeout by maxWait");
assertTrue(thread1.isAlive());
}
}
