[
https://issues.apache.org/jira/browse/POOL-418?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17924260#comment-17924260
]
Gary D. Gregory edited comment on POOL-418 at 2/5/25 9:28 PM:
--------------------------------------------------------------
Hello [~sunshuai]
Please accept my apologies for the delay in replying to your question.
We should try to follow the duration limit as closely as possible. Do you think
we should recompute the duration, as in:
{code:java}
diff --git a/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java
b/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java
index 023b552..f81271f 100644
--- a/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java
+++ b/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java
@@ -246,8 +246,8 @@
* <p>
* If there are no idle instances available in the pool, behavior depends
on
* the {@link #getMaxTotal() maxTotal}, (if applicable)
- * {@link #getBlockWhenExhausted()} and the value passed in to the
- * {@code borrowMaxWaitMillis} parameter. If the number of instances
+ * {@link #getBlockWhenExhausted()} and the value passed in the
+ * {@code maxWaitDuration} parameter. If the number of instances
* checked out from the pool 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}
@@ -261,7 +261,7 @@
* {@code NoSuchElementException} (if
* {@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 borrowMaxWaitMillis}
+ * determined by the value passed in to the {@code maxWaitDuration}
* parameter.
* </p>
* <p>
@@ -271,17 +271,17 @@
* available instances in request arrival order.
* </p>
*
- * @param borrowMaxWaitDuration The time to wait for an object to become
available, not null.
+ * @param maxWaitDuration The time to wait for an object to become
available, not null.
* @return object instance from the pool
* @throws NoSuchElementException if an instance cannot be returned
* @throws Exception if an object instance cannot be returned due to an
error
* @since 2.10.0
*/
- public T borrowObject(final Duration borrowMaxWaitDuration) throws
Exception {
+ public T borrowObject(final Duration maxWaitDuration) throws Exception {
assertOpen();
final Instant startInstant = Instant.now();
- final boolean negativeDuration = borrowMaxWaitDuration.isNegative();
- Duration remainingWaitDuration = borrowMaxWaitDuration;
+ final boolean negativeDuration = maxWaitDuration.isNegative();
+ Duration remainingWaitDuration = maxWaitDuration;
final AbandonedConfig ac = this.abandonedConfig;
if (ac != null && ac.getRemoveAbandonedOnBorrow() && getNumIdle() < 2
&& getNumActive() > getMaxTotal() - 3) {
removeAbandoned(ac);
@@ -292,7 +292,7 @@
final boolean blockWhenExhausted = getBlockWhenExhausted();
boolean create;
while (p == null) {
- remainingWaitDuration =
remainingWaitDuration.minus(durationSince(startInstant));
+ remainingWaitDuration =
maxWaitDuration.minus(durationSince(startInstant));
create = false;
p = idleObjects.pollFirst();
if (p == null) {
@@ -303,11 +303,12 @@
}
if (blockWhenExhausted) {
if (PooledObject.isNull(p)) {
+ remainingWaitDuration =
maxWaitDuration.minus(durationSince(startInstant));
p = negativeDuration ? idleObjects.takeFirst() :
idleObjects.pollFirst(remainingWaitDuration);
}
if (PooledObject.isNull(p)) {
throw new NoSuchElementException(appendStats(
- "Timeout waiting for idle object,
borrowMaxWaitDuration=" + remainingWaitDuration));
+ "Timeout waiting for idle object,
maxWaitDuration=" + remainingWaitDuration));
}
} else if (PooledObject.isNull(p)) {
throw new NoSuchElementException(appendStats("Pool
exhausted"));
@@ -379,7 +380,7 @@
* If there are no idle instances available in the pool, behavior depends
on
* the {@link #getMaxTotal() maxTotal}, (if applicable)
* {@link #getBlockWhenExhausted()} and the value passed in to the
- * {@code borrowMaxWaitMillis} parameter. If the number of instances
+ * {@code maxWaitMillis} parameter. If the number of instances
* checked out from the pool 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}
@@ -393,7 +394,7 @@
* {@code NoSuchElementException} (if
* {@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 borrowMaxWaitMillis}
+ * determined by the value passed in to the {@code maxWaitMillis}
* parameter.
* </p>
* <p>
@@ -403,15 +404,15 @@
* available instances in request arrival order.
* </p>
*
- * @param borrowMaxWaitMillis The time to wait in milliseconds for an
object
+ * @param maxWaitMillis The time to wait in milliseconds for an object
* to become available
* @return object instance from the pool
* @throws NoSuchElementException if an instance cannot be returned
* @throws Exception if an object instance cannot be returned due to an
* error
*/
- public T borrowObject(final long borrowMaxWaitMillis) throws Exception {
- return borrowObject(Duration.ofMillis(borrowMaxWaitMillis));
+ public T borrowObject(final long maxWaitMillis) throws Exception {
+ return borrowObject(Duration.ofMillis(maxWaitMillis));
}
/**
@@ -512,7 +513,7 @@
Boolean create = null;
while (create == null) {
// remainingWaitDuration handles spurious wakeup from wait().
- remainingWaitDuration =
remainingWaitDuration.minus(durationSince(startInstant));
+ remainingWaitDuration =
maxWaitDuration.minus(durationSince(startInstant));
synchronized (makeObjectCountLock) {
final long newCreateCount = createCount.incrementAndGet();
if (newCreateCount > localMaxTotal) {
{code}
I think the {{remainingWaitDuration}} computation was wrong!
WDYT?
was (Author: garydgregory):
Hello [~sunshuai]
Please accept my apologies for the delay in replying to your question.
We should try to follow the duration limit as closely as possible. Do you think
we should recompute the duration, as in:
{code:java}
diff --git a/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java
b/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java
index 023b552..3bc15af 100644
--- a/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java
+++ b/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java
@@ -303,6 +303,7 @@
}
if (blockWhenExhausted) {
if (PooledObject.isNull(p)) {
+ remainingWaitDuration =
remainingWaitDuration.minus(durationSince(startInstant));
p = negativeDuration ? idleObjects.takeFirst() :
idleObjects.pollFirst(remainingWaitDuration);
}
if (PooledObject.isNull(p)) {
{code}
?
> The maximum wait time for GenericObjectPool.borrowObject(*) may exceed
> expectations due to a spurious thread wakeup
> -------------------------------------------------------------------------------------------------------------------
>
> Key: POOL-418
> URL: https://issues.apache.org/jira/browse/POOL-418
> Project: Commons Pool
> Issue Type: Bug
> Reporter: SunShuai
> Assignee: Gary D. Gregory
> Priority: Minor
> Fix For: 2.12.1
>
> Attachments: 74C00A2F-EE3A-4660-BE58-66CA37F9808A.png,
> EE4BD798-26B6-4AE1-B114-5AC7342B31A6.png
>
>
> I found an issue when using jedis to operate Redis database, Here is the
> issue link -> [https://github.com/redis/jedis/issues/4014]. we feel that it's
> a problem with the Commons pool.
>
> In situations where the connection is tight, the waiting time can be up to
> twice the maxWaitMillis consumption.
> code: org.apache.commons.pool2.impl.GenericObjectPool#borrowObject(long) ->
> org.apache.commons.pool2.impl.GenericObjectPool#create
> The thread that failed to create the connection will wake up the thread that
> is waiting to be created. Threads that do not compete for resources will wait
> again, and the waiting time will still be the maxWaitMillis.
> If the first waiting time is ( maxWaitMillis - 1ms), plus the second full
> waiting time, then the full waiting time will be twice the expected time.
>
> The waiting time we hope for this time is the total waiting time minus the
> time already waited.
>
> such as: Duration remainingWait =
> localMaxWaitDuration.minus(Duration.between(localStartInstant,
> Instant.now()));
> if (remainingWait.isNegative()) {
> create = Boolean.FALSE;
> } else {
> wait(makeObjectCountLock, remainingWait);
> }
>
> We are not sure if this issue has been addressed and look forward to your
> reply.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
