[DBCP-600] Honor positive sub-second validation timeouts

src/changes/changes.xml

@@ -64,6 +64,7 @@ The <action> type attribute can be add,update,fix,remove.
   <body>
     <release version="2.14.1" date="YYYY-MM-DD" description="This is a minor release, including bug fixes and enhancements.">
       <!-- FIX -->
+      <action type="fix" issue="DBCP-600" due-to="Aryan Ganjoo">Apply a positive sub-second validation timeout as one JDBC second instead of disabling the timeout.</action>
       <action type="fix" dev="ggregory" due-to="Gary Gregory, Mark Thomas">Handle SQLException on connection initialization with SQL statements (#540).</action> 
       <!-- ADD -->
       <!-- UPDATE -->

src/main/java/org/apache/commons/dbcp2/PoolableConnection.java

@@ -404,21 +404,22 @@ public void setLastUsed() {
      * @param sql
      *            The validation SQL query.
      * @param timeoutDuration
-     *            The validation timeout in seconds.
+     *            The validation timeout, applied in whole seconds: zero or negative means no timeout, and a positive
+     *            sub-second value is rounded up to one second.
      * @throws SQLException
      *             Thrown when validation fails or an SQLException occurs during validation
      * @since 2.10.0
      */
-    public void validate(final String sql, Duration timeoutDuration) throws SQLException {
+    public void validate(final String sql, final Duration timeoutDuration) throws SQLException {
         if (fastFailValidation && fatalSqlExceptionThrown.get()) {
             throw new SQLException(Utils.getMessage("poolableConnection.validate.fastFail"));
         }
 
+        // JDBC timeouts are whole seconds where 0 means no timeout, so a positive sub-second timeout becomes 1 second.
+        final int timeoutSeconds = timeoutDuration.isNegative() || timeoutDuration.isZero() ? 0
+                : (int) Math.max(1, Math.min(Integer.MAX_VALUE, timeoutDuration.getSeconds()));
         if (sql == null || sql.isEmpty()) {
-            if (timeoutDuration.isNegative()) {
-                timeoutDuration = Duration.ZERO;
-            }
-            if (!isValid(timeoutDuration)) {
+            if (!isValid(Duration.ofSeconds(timeoutSeconds))) {
                 throw new SQLException("isValid() returned false");
             }
             return;
@@ -431,8 +432,8 @@ public void validate(final String sql, Duration timeoutDuration) throws SQLExcep
             validationPreparedStatement = getInnermostDelegateInternal().prepareStatement(sql);
         }
 
-        if (timeoutDuration.compareTo(Duration.ZERO) > 0) {
-            validationPreparedStatement.setQueryTimeout((int) timeoutDuration.getSeconds());
+        if (timeoutSeconds > 0) {
+            validationPreparedStatement.setQueryTimeout(timeoutSeconds);
         }
 
         try (ResultSet rs = validationPreparedStatement.executeQuery()) {

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

@@ -1131,12 +1131,21 @@ void testValidationQueryTimeoutNegative() throws Exception {
     void testValidationQueryTimeoutSucceed() throws Exception {
         ds.setTestOnBorrow(true);
         ds.setTestOnReturn(true);
-        ds.setValidationQueryTimeout(Duration.ofMillis(100)); // Works for TesterStatement
+        ds.setValidationQueryTimeout(Duration.ofSeconds(5)); // Works for TesterStatement
         try (final Connection con = ds.getConnection()) {
             // close right away.
         }
     }
 
+    @Test
+    void testValidationQueryTimeoutSubSecondFail() {
+        ds.setTestOnBorrow(true);
+        // A positive sub-second timeout must not be truncated to 0 (no timeout); it becomes 1 second, too fast for TesterStatement.
+        ds.setValidationQueryTimeout(Duration.ofMillis(100));
+        final SQLException e = assertThrows(SQLException.class, ds::getConnection);
+        assertTrue(e.toString().contains("timeout"));
+    }
+
     @Test
     void testValidationQueryTimeoutZero() throws Exception {
         ds.setTestOnBorrow(true);

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

@@ -21,9 +21,14 @@
 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.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 
 import java.sql.Connection;
 import java.sql.SQLException;
+import java.time.Duration;
 import java.util.ArrayList;
 import java.util.Arrays;
 
@@ -213,4 +218,18 @@ void testPoolableConnectionLeak() throws Exception {
 
         assertEquals(0, pool.getNumActive(), "The pool should have no active connections");
     }
+
+    /**
+     * Without a validation query, a positive sub-second timeout must reach {@link Connection#isValid(int)} as 1 second,
+     * not 0 (no timeout).
+     */
+    @Test
+    void testValidateWithoutQuerySubSecondTimeout() throws Exception {
+        final Connection nativeConnection = mock(Connection.class);
+        when(nativeConnection.isValid(1)).thenReturn(true);
+        final PoolableConnection conn = new PoolableConnection(nativeConnection, null, null);
+        conn.validate(null, Duration.ofMillis(100));
+        verify(nativeConnection).isValid(1);
+        verify(nativeConnection, never()).isValid(0);
+    }
 }