GenericObjectPool.borrowObject(Duration) doesn't obey its borrowMaxWait

Duration argument when the argument is different from
GenericObjectPool.getMaxWaitDuration()

Author: garydgregory

src/changes/changes.xml

@@ -64,6 +64,18 @@ The <action> type attribute can be add,update,fix,remove.
       Add ReslientPooledObjectFactory to provide resilence against factory outages.
     </action>
   </release> 
+  <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>
+    <action type="update" dev="ggregory">Bump org.ow2.asm:asm-util from 9.5 to 9.7.</action>
+    <action type="update" dev="ggregory" due-to="Gary Gregory">[test] Bump commons-lang3 from 3.13.0 to 3.16.0.</action>
+    <action type="update" dev="ggregory" due-to="Gary Gregory">[site] Bump org.apache.bcel:bcel from 6.7.0 to 6.8.2.</action>
+    <action type="update" dev="ggregory" due-to="Gary Gregory">[test] Bump org.hamcrest:hamcrest-library from 2.2 to 3.0.</action>
+  </release>
   <release version="2.12.0" date="2023-MM-DD" description="This is a feature and maintenance release (Java 8 or above).">
     <!-- FIX -->
     <action dev="psteitz" type="fix" issue="POOL-401">

src/main/java/org/apache/commons/pool3/impl/GenericObjectPool.java

@@ -220,7 +220,7 @@ public void addObject() throws E {
         if (factory == null) {
             throw new IllegalStateException("Cannot add objects without a factory.");
         }
-        addIdleObject(create());
+        addIdleObject(create(getMaxWaitDuration()));
     }
 
     /**
@@ -274,61 +274,57 @@ public T borrowObject() throws E {
      * available instances in request arrival order.
      * </p>
      *
-     * @param borrowMaxWaitDuration The time to wait for an object
+     * @param maxWaitDuration The time to wait for an object
      *                            to become available
      *
      * @return object instance from the pool
      * @throws NoSuchElementException if an instance cannot be returned
      * @throws E if an object instance cannot be returned due to an error
      * @since 2.10.0
      */
-    public T borrowObject(final Duration borrowMaxWaitDuration) throws E {
+    public T borrowObject(final Duration maxWaitDuration) throws E {
         assertOpen();
-
+        final Instant startInstant = Instant.now();
+        final boolean negativeDuration = maxWaitDuration.isNegative();
+        Duration remainingWaitDuration = maxWaitDuration;
         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)) {
                     try {
-                        p = borrowMaxWaitDuration.isNegative() ? idleObjects.takeFirst() : idleObjects.pollFirst(borrowMaxWaitDuration);
+                        p = negativeDuration ? idleObjects.takeFirst() : idleObjects.pollFirst(maxWaitDuration);
                     } catch (final InterruptedException e) {
                         // Don't surface exception type of internal locking mechanism.
                         throw cast(e);
                     }
                 }
                 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"));
             }
             if (!p.allocate()) {
                 p = null;
             }
-
             if (!PooledObject.isNull(p)) {
                 try {
                     factory.activateObject(p);
@@ -373,9 +369,7 @@ public T borrowObject(final Duration borrowMaxWaitDuration) throws E {
                 }
             }
         }
-
-        updateStatsBorrow(p, Duration.between(waitTime, Instant.now()));
-
+        updateStatsBorrow(p, durationSince(startInstant));
         return p.getObject();
     }
 
@@ -510,19 +504,19 @@ public void close() {
      * 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 E if the object factory's {@code makeObject} fails
      */
-    private PooledObject<T> create() throws E {
+    private PooledObject<T> create(final Duration maxWaitDuration) throws E {
         int localMaxTotal = getMaxTotal();
         // This simplifies the code later in this method
         if (localMaxTotal < 0) {
             localMaxTotal = Integer.MAX_VALUE;
         }
 
         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
@@ -561,9 +555,9 @@ private PooledObject<T> create() throws E {
                 }
             }
 
-            // 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;
             }
         }
@@ -600,10 +594,14 @@ private PooledObject<T> create() throws E {
         }
 
         createdCount.incrementAndGet();
-        allObjects.put(IdentityWrapper.on(p), p);
+        allObjects.put(new IdentityWrapper<>(p.getObject()), p);
         return p;
     }
 
+    private Duration durationSince(final Instant startInstant) {
+        return Duration.between(startInstant, Instant.now());
+    }
+
     /**
      * Destroys a wrapped pooled object.
      *
@@ -648,7 +646,7 @@ private void ensureIdle(final int idleCount, final boolean always) throws E {
         }
 
         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.

src/test/java/org/apache/commons/pool3/impl/TestGenericObjectPool.java

@@ -51,6 +51,7 @@
 import javax.management.MBeanServer;
 import javax.management.ObjectName;
 
+import org.apache.commons.lang3.time.DurationUtils;
 import org.apache.commons.pool3.BasePooledObjectFactory;
 import org.apache.commons.pool3.ObjectPool;
 import org.apache.commons.pool3.PoolUtils;
@@ -1070,6 +1071,52 @@ public void testBorrowObjectFairness() throws Exception {
         }
     }
 
+    @Test/* maxWaitMillis x2 + padding */
+    @Timeout(value = 1200, unit = TimeUnit.MILLISECONDS)
+    public void testBorrowObjectOverrideMaxWaitLarge() throws Exception {
+        try (final GenericObjectPool<String, InterruptedException> 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<InterruptedException> 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, InterruptedException> 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<InterruptedException> 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
@@ -2642,28 +2689,40 @@ public void testPreparePool() throws Exception {
         assertEquals(1, genericObjectPool.getNumIdle());
     }
 
+    @Test/* maxWaitMillis x2 + padding */
+    @Timeout(value = 1200, unit = TimeUnit.MILLISECONDS)
+    public void testReturnBorrowObjectWithingMaxWaitDuration() throws Exception {
+        final Duration maxWaitDuration = Duration.ofMillis(500);
+        try (final GenericObjectPool<String, InterruptedException> createSlowObjectFactoryPool = new GenericObjectPool<>(createSlowObjectFactory(60_000))) {
+            createSlowObjectFactoryPool.setMaxTotal(1);
+            createSlowObjectFactoryPool.setMaxWait(maxWaitDuration);
+            // thread1 tries creating a slow object to make pool full.
+            final WaitingTestThread<InterruptedException> thread1 = new WaitingTestThread<>(createSlowObjectFactoryPool, 0);
+            thread1.start();
+            // Wait for thread1's reaching to create().
+            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");
+            assertTrue(thread1.isAlive());
+        }
+    }
+    
     @Test /* maxWaitMillis x2 + padding */
     @Timeout(value = 1200, unit = TimeUnit.MILLISECONDS)
     public void testReturnBorrowObjectWithingMaxWaitMillis() throws Exception {
         final long maxWaitMillis = 500;
-
-        try (final GenericObjectPool<String, InterruptedException> createSlowObjectFactoryPool = new GenericObjectPool<>(
-                createSlowObjectFactory(60000))) {
+        try (final GenericObjectPool<String, InterruptedException> createSlowObjectFactoryPool = new GenericObjectPool<>(createSlowObjectFactory(60000))) {
             createSlowObjectFactoryPool.setMaxTotal(1);
             createSlowObjectFactoryPool.setMaxWait(Duration.ofMillis(maxWaitMillis));
-
             // thread1 tries creating a slow object to make pool full.
-            final WaitingTestThread<InterruptedException> thread1 = new WaitingTestThread<>(createSlowObjectFactoryPool,
-                    0);
+            final WaitingTestThread<InterruptedException> thread1 = new WaitingTestThread<>(createSlowObjectFactoryPool, 0);
             thread1.start();
-
             // Wait for thread1's reaching to create().
             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");
-
             assertTrue(thread1.isAlive());
         }
     }