[POOL-419] Fix for POOL-419. Before adding the returned object to the pool, we must check whether it is invalid.

Author: Raju Kumar Gupta

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

@@ -1091,6 +1091,12 @@ public void returnObject(final T obj) {
                 swallowException(e);
             }
         } else {
+
+            if (!p.deallocate()) {
+                throw new IllegalStateException(
+                    "Object has already been returned to this pool or is invalid");
+            }
+
             if (getLifo()) {
                 idleObjects.addFirst(p);
             } else {

src/test/java/org/apache/commons/pool3/TestPoolUtils.java

@@ -21,7 +21,6 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
-import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
 
 import java.lang.reflect.InvocationHandler;
@@ -34,16 +33,10 @@
 import java.util.List;
 import java.util.Map;
 import java.util.TimerTask;
-import java.util.concurrent.CountDownLatch;
-import java.util.concurrent.ExecutorService;
-import java.util.concurrent.Executors;
-import java.util.concurrent.FutureTask;
-import java.util.concurrent.TimeUnit;
 
 import org.apache.commons.pool3.impl.DefaultPooledObject;
 import org.apache.commons.pool3.impl.GenericKeyedObjectPool;
 import org.apache.commons.pool3.impl.GenericObjectPool;
-import org.apache.commons.pool3.impl.GenericObjectPoolConfig;
 import org.junit.jupiter.api.Test;
 import org.opentest4j.AssertionFailedError;
 
@@ -691,60 +684,4 @@ public void testTimerHolder() {
         assertNotNull(h);
         assertNotNull(PoolUtils.TimerHolder.MIN_IDLE_TIMER);
     }
-
-    /*
-    * Test for POOL-419.
-    * https://issues.apache.org/jira/browse/POOL-419
-    */
-    @Test
-    void testPool419() throws Exception{
-
-        ExecutorService executor = Executors.newFixedThreadPool(100);
-
-        final List<String> calledMethods = new ArrayList<>();
-
-        final GenericObjectPoolConfig<Object> config = new GenericObjectPoolConfig<>();
-
-        config.setMaxTotal(10000);
-        config.setMaxIdle(10000);
-        config.setMinIdle(10000);
-
-        @SuppressWarnings("unchecked")
-        final PooledObjectFactory<Object, RuntimeException> pof = createProxy(PooledObjectFactory.class, calledMethods);
-        try (final ObjectPool<Object, RuntimeException> connectionPool = new GenericObjectPool<>(pof, config)) {
-            assertNotNull(connectionPool);
-
-            CountDownLatch startLatch = new CountDownLatch(1);
-
-            for (int i = 0; i < 10000; i++) {
-                Object poolObject = connectionPool.borrowObject();
-
-                FutureTask<Boolean> invalidateObject = new FutureTask<>(() -> {
-                    startLatch.await();
-                    connectionPool.invalidateObject(poolObject);
-                    return true;
-                });
-
-                FutureTask<Boolean> returnObject = new FutureTask<>(() -> {
-                    startLatch.await();
-                    connectionPool.returnObject(poolObject);
-                    return true;
-                });
-
-                executor.submit(returnObject);
-                executor.submit(invalidateObject);
-
-            }
-
-            startLatch.countDown(); // Start all tasks simultaneously
-
-            executor.shutdown();
-            assertTrue(executor.awaitTermination(Long.MAX_VALUE, TimeUnit.NANOSECONDS));
-
-            assertEquals(0, connectionPool.getNumActive(), "getNumActive() must not return a negative value");
-
-        }
-    }
-
 }
-

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

@@ -41,6 +41,10 @@
 import java.util.Set;
 import java.util.Timer;
 import java.util.TimerTask;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.FutureTask;
 import java.util.concurrent.Semaphore;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;
@@ -3150,4 +3154,78 @@ public void testWhenExhaustedFail() throws Exception {
         genericObjectPool.close();
     }
 
+    private BasePooledObjectFactory<Object, RuntimeException> createPooledObjectFactory() {
+        return new BasePooledObjectFactory<>() {
+            @Override
+            public Object create() {
+                return new Object();
+            }
+
+            @Override
+            public PooledObject<Object> wrap(final Object obj) {
+                return new DefaultPooledObject<>(obj);
+            }
+        };
+    }
+
+
+    /*
+     * Test for POOL-419.
+     * https://issues.apache.org/jira/browse/POOL-419
+     */
+    @Test
+    void testPool419() throws Exception{
+
+        ExecutorService executor = Executors.newFixedThreadPool(100);
+
+        final GenericObjectPoolConfig<Object> config = new GenericObjectPoolConfig<>();
+
+        final int maxConnections = 10000;
+
+        config.setMaxTotal(maxConnections);
+        config.setMaxIdle(maxConnections);
+        config.setMinIdle(1);
+
+        final BasePooledObjectFactory<Object, RuntimeException> pof = createPooledObjectFactory();
+
+        try (final ObjectPool<Object, RuntimeException> connectionPool = new GenericObjectPool<>(pof, config)) {
+            assertNotNull(connectionPool);
+
+            CountDownLatch startLatch = new CountDownLatch(1);
+
+            List<Object> poolObjects = new ArrayList<>();
+
+            List<FutureTask<Boolean>> tasks = new ArrayList<>();
+
+            for (int i = 0; i < maxConnections; i++) {
+                poolObjects.add(connectionPool.borrowObject());
+            }
+
+            for(Object poolObject : poolObjects) {
+
+                tasks.add(new FutureTask<>(() -> {
+                    startLatch.await();
+                    connectionPool.invalidateObject(poolObject);
+                    return true;
+                }));
+
+                tasks.add(new FutureTask<>(() -> {
+                    startLatch.await();
+                    connectionPool.returnObject(poolObject);
+                    return true;
+                }));
+            }
+
+            tasks.forEach(executor::submit);
+
+            startLatch.countDown(); // Start all tasks simultaneously
+
+            executor.shutdown();
+            assertTrue(executor.awaitTermination(Long.MAX_VALUE, TimeUnit.NANOSECONDS));
+
+            assertEquals(0, connectionPool.getNumActive(), "getNumActive() must not return a negative value");
+
+        }
+    }
+
 }