Adjust tests and CPDSConnectionFactory#invalidate to accomodate behav… (#539)

* Adjust tests and CPDSConnectionFactory#invalidate to accomodate behavior change in the fix for POOL-424.

* Remove `ex.printStackTrace();`

---------

Co-authored-by: Gary Gregory <[email protected]>

Author: psteitz

src/changes/changes.xml

@@ -121,6 +121,7 @@ The <action> type attribute can be add,update,fix,remove.
       <!-- ADD -->
       <action type="add" dev="ggregory" due-to="Gary Gregory">Add org.apache.commons.dbcp2.datasources.PooledConnectionManager.setPassword(char[]).</action>
       <!-- UPDATE -->
+      <action type="update" dev="psteitz">Update tests and CPDSConnectionFactory#invalidate to accomodate changed behavior in the fix for POOL-424.</action>
       <action type="update" dev="ggregory" due-to="Gary Gregory">Bump org.apache.commons:commons-parent from 78 to 93 #521, #537, #538.</action>
       <action type="update" dev="ggregory" due-to="Gary Gregory">Bump org.apache.commons:commons-pool2 from 2.12.0 to 2.12.1 #474.</action>
       <action type="update" dev="ggregory" due-to="Gary Gregory">Port site from Doxia 1 to 2.</action>

src/main/java/org/apache/commons/dbcp2/datasources/CPDSConnectionFactory.java

@@ -192,8 +192,9 @@ public void invalidate(final PooledConnection pc) throws SQLException {
             throw new IllegalStateException(NO_KEY_MESSAGE);
         }
         try {
-            pool.invalidateObject(pci); // Destroy instance and update pool counters
             pool.close(); // Clear any other instances in this pool and kill others as they come back
+            // Calling close before invalidate ensures that invalidate will not trigger a create attempt
+            pool.invalidateObject(pci); // Destroy instance and update pool counters
         } catch (final Exception ex) {
             throw new SQLException("Error invalidating connection", ex);
         }

src/test/java/org/apache/commons/dbcp2/TestBasicDataSource.java

@@ -568,6 +568,37 @@ void testInstanceNotFoundExceptionLogSuppressed() throws Exception {
         assertNull(ds.getRegisteredJmxName());
     }
 
+    /**
+     * Cycle through idle connections and verify that they are all valid.
+     * Assumes we are the only client of the pool.
+     *
+     * @throws Exception if an error occurs
+     */
+    private void checkIdleValid() throws Exception {
+        final Set<Connection> idleConnections = new HashSet<>(); // idle connections
+        // Get idle connections by repeatedly making connection requests up to NumIdle
+        for (int i = 0; i < ds.getNumIdle(); i++) {
+            final Connection conn = ds.getConnection();
+            idleConnections.add(conn);
+        }
+        // Cycle through idle connections and verify that they are valid
+        for (final Connection conn : idleConnections) {
+            assertTrue(conn.isValid(2), "Connection should be valid");
+            conn.close();
+        }
+    }
+
+    /**
+     * Check that maxTotal and maxIdle are not exceeded
+     *
+     * @throws Exception
+     */
+    private void checkLimits() throws Exception {
+        assertTrue(ds.getNumActive() + ds.getNumIdle() <= getMaxTotal(), "Total connections exceed maxTotal");
+        assertTrue(ds.getNumIdle() <= ds.getMaxIdle(), "Idle connections exceed maxIdle");
+    }
+
+
     @Test
     void testInvalidateConnection() throws Exception {
         ds.setMaxTotal(2);
@@ -576,9 +607,11 @@ void testInvalidateConnection() throws Exception {
                 ds.invalidateConnection(conn1);
                 assertTrue(conn1.isClosed());
                 assertEquals(1, ds.getNumActive());
-                assertEquals(0, ds.getNumIdle());
+                checkIdleValid();
+                checkLimits();
                 try (final Connection conn3 = ds.getConnection()) {
                     conn2.close();
+                    conn3.close();
                 }
             }
         }

src/test/java/org/apache/commons/dbcp2/TestPoolingDriver.java

@@ -142,7 +142,6 @@ void testInvalidateConnection() throws Exception {
         driver2.invalidateConnection(conn);
 
         assertEquals(0, pool.getNumActive());
-        assertEquals(0, pool.getNumIdle());
         assertTrue(conn.isClosed());
     }
 

src/test/java/org/apache/commons/dbcp2/datasources/TestCPDSConnectionFactory.java

@@ -50,6 +50,12 @@ public void setUp() throws Exception {
         delegate.setPassword("password");
     }
 
+    private void checkPoolLimits(final GenericObjectPool<PooledConnectionAndInfo> pool) {
+        assertTrue(pool.getNumActive() + pool.getNumIdle() <= pool.getMaxTotal(),
+                "Active + Idle should be <= MaxTotal");
+        assertTrue(pool.getNumIdle() <= pool.getMaxIdle(), "Idle should be <= MaxIdle");
+    }
+
     /**
      * JIRA DBCP-216
      *
@@ -77,14 +83,14 @@ void testConnectionErrorCleanup() throws Exception {
                 // Throw connectionError event
                 pc.throwConnectionError();
 
-                // Active count should be reduced by 1 and no idle increase
+                // Active count should be reduced by 1
                 assertEquals(1, pool.getNumActive());
-                assertEquals(0, pool.getNumIdle());
+                checkPoolLimits(pool);
 
                 // Throw another one - should be ignored
                 pc.throwConnectionError();
                 assertEquals(1, pool.getNumActive());
-                assertEquals(0, pool.getNumIdle());
+                checkPoolLimits(pool);
 
                 // Ask for another connection
                 final PooledConnection pcon3 = pool.borrowObject().getPooledConnection();