This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 10.1.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 4aac37b004a72055437b11708a334991dbe5bed8 Author: opencode <[email protected]> AuthorDate: Tue Oct 27 16:40:34 2026 +0100 Treat a throwing custom Validator as a failed validation The Validator SPI declares no checked exception, so throwing is the natural way for an implementation to report its own failure, and the pool invoked poolProperties.getValidator() without any guard. The throw escaped PooledConnection.validate() into the pool machinery, where two of the call sites are unprotected: - borrowConnection() validates after the connection was polled out of the idle queue and before busy.offer(), so the RuntimeException unwound getConnection() leaving the pooled connection in neither queue but still counted in size: a permanently lost slot that removeAbandoned cannot reclaim; - returnConnection() removes the connection from busy before shouldClose() validates, so a throw stranded it the same way and escaped the application's close(); - in testAllIdle() the outer catch survived the throw but aborted the pass, so the broken connection was never evicted and connections later in the iteration were starved. Meanwhile the two built-in validation paths in the very same method - isValid() and the validation query - convert any Exception into a false return, and the create/reconnect paths already catch, release and wrap. The throw was the anomaly. Catch RuntimeException around the validator call only, log it the way the sibling failure branches log, and return false; every path then behaves exactly as it already does for an honest false: borrow falls into the reconnect-and-revalidate branch and ends in a checked SQLException with the slot released, return discards the connection that cannot be vouched for, and the idle sweep evicts and continues. Spell out the exception contract in the Validator javadoc, since the guard alone would leave the SPI under-specified. Add TestThrowingValidator: three pins for the borrow, return and idle paths, verified to fail at the pre-fix commit, plus controls for the create path (already guarded, unchanged behavior) and a validator returning plain false (the new throw-handling must be exactly as the false path). --- .../apache/tomcat/jdbc/pool/PooledConnection.java | 19 ++- .../org/apache/tomcat/jdbc/pool/Validator.java | 20 ++- .../tomcat/jdbc/pool/TestThrowingValidator.java | 164 +++++++++++++++++++++ 3 files changed, 201 insertions(+), 2 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java index 4390a8afe3..a350d069cb 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java @@ -595,7 +595,24 @@ public class PooledConnection implements PooledConnectionMBean { } if (poolProperties.getValidator() != null) { - if (poolProperties.getValidator().validate(connection, validateAction)) { + boolean valid; + try { + valid = poolProperties.getValidator().validate(connection, validateAction); + } catch (RuntimeException x) { + // the SPI declares no checked exception, so throwing is the + // only way an implementation can report its own failure; the + // pool must treat that as a failed validation, because a + // throw escaping from here unwinds the borrow or return path + // between the idle and busy queues and strands the pooled + // connection - permanently shrinking the pool + if (getPoolProperties().getLogValidationErrors()) { + log.error("Custom validation through "+poolProperties.getValidator()+" threw an exception.", x); + } else if (log.isDebugEnabled()) { + log.debug("Custom validation threw an exception.", x); + } + return false; + } + if (valid) { this.lastValidated = now; return true; } else { diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/Validator.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/Validator.java index af2e395f99..5cf8075ca4 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/Validator.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/Validator.java @@ -24,8 +24,26 @@ import java.sql.Connection; public interface Validator { /** * Validate a connection and return a boolean to indicate if it's valid. + * <p> + * The pool calls this method with the underlying JDBC connection, outside + * any interceptor chain, and treats any runtime exception thrown by an + * implementation as a validation failure. Implementations that invoke JDBC + * APIs must catch {@link java.sql.SQLException} and return {@code false} + * themselves, since this method does not declare a checked exception. + * A {@code false} result (or a thrown runtime exception) causes the pool + * to discard the connection. + * <p> + * The pool may skip validation entirely for a given action - it only + * validates when the corresponding {@code testOnBorrow}, + * {@code testOnReturn}, {@code testWhileIdle} or {@code testOnConnect} + * flag is set - and it may short-circuit repeated validations, see + * {@link PoolConfiguration#setValidationInterval(long)}. This method is + * therefore not guaranteed to run for every borrow, and it must not be + * relied on for per-borrow session state. Implementations are invoked + * concurrently from borrowing and returning application threads and from + * the pool cleaner thread and must be thread-safe. * - * @param connection the Connection object to test + * @param connection the underlying (non-proxied) {@link Connection} object to test * @param validateAction the action used. One of {@link PooledConnection#VALIDATE_BORROW}, * {@link PooledConnection#VALIDATE_IDLE}, {@link PooledConnection#VALIDATE_INIT} or * {@link PooledConnection#VALIDATE_RETURN} diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestThrowingValidator.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestThrowingValidator.java new file mode 100644 index 0000000000..db922762bc --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestThrowingValidator.java @@ -0,0 +1,164 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomcat.jdbc.pool; + +import java.sql.Connection; +import java.sql.SQLException; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; + +/** + * A custom {@link Validator} cannot declare a checked exception, so + * throwing is the only way it can report its own failure. The pool must + * treat such a runtime exception as a failed validation: it must not + * escape {@code getConnection()}/{@code close()}, must not strand the + * pooled connection between the idle and busy queues (a permanently lost + * slot), and must not silently disable idle eviction. + */ +class TestThrowingValidator extends JdbcPoolBaseTest { + + /** The natural broken implementation: reports its failure by throwing. */ + public static class ThrowingValidator implements Validator { + @Override + public boolean validate(Connection connection, int validateAction) { + throw new IllegalStateException("validator boom"); + } + } + + /** A validator that only fails, the way the throwing one now fails. */ + public static class RejectingValidator implements Validator { + @Override + public boolean validate(Connection connection, int validateAction) { + return false; + } + } + + /* One-slot pool with a throwing validator, gated on by the caller's test flag. */ + private DataSource throwingPool() { + DataSource ds = newDataSource(); + ds.getPoolProperties().setInitialSize(0); + // exactly one slot: any leaked slot exhausts the pool immediately + ds.getPoolProperties().setMaxActive(1); + ds.getPoolProperties().setValidationInterval(0); + // the tests drive validation themselves; with the guard in place a + // background sweep would now be a working competitor for the idle + // assertions below + ds.getPoolProperties().setTimeBetweenEvictionRunsMillis(0); + ds.getPoolProperties().setValidator(new ThrowingValidator()); + return ds; + } + + @Test + void throwingValidatorOnBorrowDoesNotLeakTheSlot() throws Exception { + DataSource ds = throwingPool(); + ds.getPoolProperties().setTestOnBorrow(true); + // the creating borrow is not validated (testOnConnect is off), the + // validator first runs when the returned connection is re-borrowed + Connection first = ds.getConnection(); + awaitSessions(1); + first.close(); + Assertions.assertEquals(1, ds.getPool().getIdle()); + + // the borrow must fail, but as the checked SQLException the + // DataSource contract allows - not as the validator's throw + SQLException sqlX = Assertions.assertThrows(SQLException.class, + ds::getConnection); + Assertions.assertTrue(sqlX.getMessage().contains("Failed to validate"), + "expected a validation failure, got: " + sqlX.getMessage()); + // and the attempt must not strand the connection: it was polled out + // of idle, so a throw escaping the validation would leave it in + // neither idle nor busy while still counted in size + awaitSessions(0); + Assertions.assertEquals(0, ds.getPool().getIdle()); + Assertions.assertEquals(0, ds.getPool().getActive()); + Assertions.assertEquals(0, ds.getPool().getSize()); + } + + @Test + void throwingValidatorOnReturnDoesNotLeakTheSlot() throws Exception { + DataSource ds = throwingPool(); + ds.getPoolProperties().setTestOnReturn(true); + Connection first = ds.getConnection(); + awaitSessions(1); + + // close() completes normally: the throwing validation counts as a + // failure, so the connection is discarded rather than returned + Assertions.assertDoesNotThrow(first::close); + awaitSessions(0); + Assertions.assertEquals(0, ds.getPool().getIdle()); + Assertions.assertEquals(0, ds.getPool().getActive()); + Assertions.assertEquals(0, ds.getPool().getSize()); + + // the pool has not permanently lost capacity: the discarded slot + // was returned to the size budget, and the next borrow succeeds + ds.getPoolProperties().setValidator(null); + Assertions.assertEquals(0, ds.getPool().getSize()); + try (Connection again = ds.getConnection()) { + Assertions.assertNotNull(again); + } + Assertions.assertEquals(0, ds.getPool().getActive()); + // the discarded connection's physical session is gone, and the + // rebuilt one is released when the pool closes + ds.close(); + awaitSessions(0); + } + + @Test + void throwingValidatorOnIdleValidationDoesNotBlockEviction() throws Exception { + DataSource ds = throwingPool(); + ds.getPoolProperties().setTestWhileIdle(true); + Connection first = ds.getConnection(); + first.close(); + awaitSessions(1); + Assertions.assertEquals(1, ds.getPool().getIdle()); + + // the throw must not abort the sweep with the broken connection + // still parked in idle forever + Assertions.assertDoesNotThrow(ds::testIdle); + awaitSessions(0); + Assertions.assertEquals(0, ds.getPool().getIdle()); + } + + @Test + void throwingValidatorOnConnectFailsAsCheckedException() throws Exception { + DataSource ds = throwingPool(); + ds.getPoolProperties().setTestOnConnect(true); + // the creation path was already guarded by a catch(Exception) that + // releases the half-built connection; the behavior must not regress + Assertions.assertThrows(SQLException.class, ds::getConnection); + awaitSessions(0); + Assertions.assertEquals(0, ds.getPool().getSize()); + } + + @Test + void rejectingValidatorOnBorrowFailsAndReleasesTheSlot() throws Exception { + DataSource ds = throwingPool(); + ds.getPoolProperties().setTestOnBorrow(true); + ds.getPoolProperties().setValidator(new RejectingValidator()); + // control for the new guard shape: a plain false from the validator + // keeps its pre-existing meaning all the way through + Connection first = ds.getConnection(); + awaitSessions(1); + first.close(); + Assertions.assertThrows(SQLException.class, ds::getConnection); + awaitSessions(0); + Assertions.assertEquals(0, ds.getPool().getSize()); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
