The maximum wait time for GenericObjectPool.borrowObject(*) may exceed

expectations due to a spurious thread wakeup

Author: garydgregory

src/changes/changes.xml

@@ -68,6 +68,7 @@ The <action> type attribute can be add,update,fix,remove.
     <!-- 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>
+    <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.</action>
     <!-- ADD -->
     <!-- UPDATE -->
     <action type="update" dev="ggregory">Bump org.apache.commons:commons-parent from 62 to 73.</action>

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

@@ -88,7 +88,9 @@ public class GenericObjectPool<T, E extends Exception> extends BaseGenericObject
         "org.apache.commons.pool3:type=GenericObjectPool,name=";
 
     private static void wait(final Object obj, final Duration duration) throws InterruptedException {
-        obj.wait(duration.toMillis(), duration.getNano() % 1_000_000);
+        if (!duration.isNegative()) {
+            obj.wait(duration.toMillis(), duration.getNano() % 1_000_000);
+        }
     }
 
     private volatile String factoryType;
@@ -509,22 +511,23 @@ public void close() {
      * @throws E if the object factory's {@code makeObject} fails
      */
     private PooledObject<T> create(final Duration maxWaitDuration) throws E {
+        final Instant startInstant = Instant.now();
+        Duration remainingWaitDuration = maxWaitDuration.isNegative() ? Duration.ZERO : maxWaitDuration;
         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 localMaxWaitDuration = maxWaitDuration.isNegative() ? Duration.ZERO : maxWaitDuration;
-
         // Flag that indicates if create should:
         // - TRUE:  call the factory to create an object
         // - FALSE: return null
         // - null:  loop and re-test the condition that determines whether to
         //          call the factory
         Boolean create = null;
         while (create == null) {
+            // remainingWaitDuration handles spurious wakeup from wait().
+            remainingWaitDuration = remainingWaitDuration.minus(durationSince(startInstant));
             synchronized (makeObjectCountLock) {
                 final long newCreateCount = createCount.incrementAndGet();
                 if (newCreateCount > localMaxTotal) {
@@ -542,7 +545,7 @@ private PooledObject<T> create(final Duration maxWaitDuration) throws E {
                         // fail so wait until they complete and then re-test if
                         // the pool is at capacity or not.
                         try {
-                            wait(makeObjectCountLock, localMaxWaitDuration);
+                            wait(makeObjectCountLock, remainingWaitDuration);
                         } catch (final InterruptedException e) {
                             // Don't surface exception type of internal locking mechanism.
                             throw cast(e);
@@ -554,10 +557,9 @@ private PooledObject<T> create(final Duration maxWaitDuration) throws E {
                     create = Boolean.TRUE;
                 }
             }
-
-            // Do not block more if localMaxWaitDuration > 0.
-            if (create == null && localMaxWaitDuration.compareTo(Duration.ZERO) > 0 &&
-                    durationSince(localStartInstant).compareTo(localMaxWaitDuration) >= 0) {
+            // Do not block more if remainingWaitDuration > 0.
+            if (create == null && remainingWaitDuration.compareTo(Duration.ZERO) > 0 &&
+                    durationSince(localStartInstant).compareTo(remainingWaitDuration) >= 0) {
                 create = Boolean.FALSE;
             }
         }

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

@@ -1941,8 +1941,8 @@ public void testFailingFactoryDoesNotBlockThreads() throws Exception {
             assertFalse(thread1.isAlive());
             assertFalse(thread2.isAlive());
 
-            assertTrue(thread1.thrown instanceof UnsupportedCharsetException);
-            assertTrue(thread2.thrown instanceof UnsupportedCharsetException);
+            assertTrue(thread1.thrown instanceof UnsupportedCharsetException, () -> Objects.toString(thread1.thrown));
+            assertTrue(thread2.thrown instanceof UnsupportedCharsetException, () -> Objects.toString(thread2.thrown));
         }
     }