This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 11.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 9241075801de4a3512c3b0db985a14943dfb8271 Author: opencode <[email protected]> AuthorDate: Tue Nov 3 17:19:18 2026 +0100 State the validator contract the way the pool honors it The SPI javadoc told implementers three things the pool does not agree to. It named the four test flags as the complete set of triggers, but a non-blank configured init SQL arms the init validation with no flag at all, and a configured validator then replaces that statement, so the configured SQL never runs. It presented a false as terminal for the session, while a borrow answers a false by re-establishing the session and validating the replacement inside the same getConnection() call - under the init action when new connections are initialized - so an implementation can be asked twice for one borrow, under two different actions. And it never disclosed that the pool ends no transaction as part of a custom validation: the built-in check terminates whatever its own query leaves open, but a validator replaces that check and owns the state its own statements open. A fourth sentence had gone stale behind the recent widening of the guard: the pool now reads any non-fatal Error, not only a runtime exception, as a failed validation, re-throwing only a fatal JVM error. The amended claims are pinned by a new test built so that a pool executing the init SQL tolerantly beside the validator cannot pass it unseen. The configuration side carried two roots of the same silence. A validator that failed to load or to instantiate was demoted to a warn line, after which the pool validated on the query path while the configuration kept reporting the validator's class name; the consequence is now stated once more, at the error level an operator sees without enabling anything. And the validator instance is transient, so a deserialized configuration read back the class name without the instance and validated on the query path under the same quiet disagreement between the reported and the run configuration; a readObject now replays the name through the setter that built it the first time, which rebuilds the instance in the reading JVM, or reports - as the original call did - why it could not be built there. A serialization round trip pins that the validator is neither lost nor faked by the round trip. --- .../apache/tomcat/jdbc/pool/PoolProperties.java | 35 ++++++++++++++++++ .../org/apache/tomcat/jdbc/pool/Validator.java | 42 +++++++++++++++------- .../tomcat/jdbc/pool/util/LocalStrings.properties | 1 + .../tomcat/jdbc/pool/TestPoolProperties.java | 39 ++++++++++++++++++++ .../apache/tomcat/jdbc/pool/TestValidation.java | 27 ++++++++++++++ 5 files changed, 132 insertions(+), 12 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolProperties.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolProperties.java index 5dfc98303f..dd200e0779 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolProperties.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolProperties.java @@ -17,6 +17,7 @@ package org.apache.tomcat.jdbc.pool; import java.io.IOException; +import java.io.ObjectInputStream; import java.io.ObjectOutputStream; import java.io.Serializable; import java.io.StringReader; @@ -873,6 +874,16 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl } catch (ReflectiveOperationException | IllegalArgumentException | SecurityException e) { log.warn(MessageFactory.get("poolProperties.validatorCannotInstantiate", className), e); } + + if (validator == null) { + // The line above names the cause; this names the consequence, + // at the level an operator sees without enabling anything: the + // configuration keeps reporting the validator's class name, + // while validation in fact runs on the query path. Left at the + // warning alone, that disagreement is invisible in the default + // log. + log.error(MessageFactory.get("poolProperties.validatorNotInEffect", className)); + } } @@ -2079,4 +2090,28 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl } + /** + * Replays the validator configuration after deserialization. The validator + * instance is transient - it is runtime state, an object loaded through + * some class loader and carrying whatever that loader can reach - but the + * class name it was built from is serialized, and a configuration that + * read back the name without rebuilding the instance would validate on the + * query path while reporting the validator as configured, which is the + * disagreement {@link #setValidatorClassName(String)} was made to log. + * Replaying the setter rebuilds the instance in the reading JVM, or + * reports - as the original setter did - why it could not be built there. + * + * @param ois the stream to read the object graph from + * + * @throws IOException if the stream fails + * @throws ClassNotFoundException if a serialized class cannot be found + */ + private void readObject(ObjectInputStream ois) throws IOException, ClassNotFoundException { + ois.defaultReadObject(); + if (validatorClassName != null) { + setValidatorClassName(validatorClassName); + } + } + + } 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 f77821252f..04f78f377f 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 @@ -19,26 +19,44 @@ package org.apache.tomcat.jdbc.pool; import java.sql.Connection; /** - * Interface to be implemented by custom validator classes. + * Interface to be implemented by custom validator classes. An implementation configured through + * {@link PoolConfiguration#setValidatorClassName(String)} is instantiated reflectively and therefore must have a + * public no-arg constructor. */ 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. + * runtime exception or non-fatal {@link Error} thrown by an implementation as a validation failure; only a + * {@link VirtualMachineError} is re-thrown. 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 or error) causes the pool to discard the + * connection; on a borrow that discard happens within the same {@code getConnection()} call, as the pool + * immediately re-establishes the session and validates the replacement, reporting it as + * {@link PooledConnection#VALIDATE_INIT} when new connections are initialized. * <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. + * The pool performs no transaction management as part of validating. The built-in, query-based check ends any + * transaction open on the session when it completes - its own, or one a previous borrower left behind - but a + * validator replaces that check and owns the session state it observes: an implementation that executes + * statements on a session whose auto-commit mode is off must commit or roll back the transaction those + * statements open, itself. + * <p> + * The pool may skip validation entirely for a given action - it validates when the corresponding + * {@code testOnBorrow}, {@code testOnReturn}, {@code testWhileIdle} or {@code testOnConnect} flag is set (the + * idle action additionally requires a running pool sweeper, see + * {@link PoolConfiguration#setTimeBetweenEvictionRunsMillis(int)}), and for + * {@link PooledConnection#VALIDATE_INIT} also when a non-blank {@link PoolConfiguration#getInitSQL() initSQL} is + * configured - and it may short-circuit repeated validations, see + * {@link PoolConfiguration#setValidationInterval(long)}. The validator replaces the query-based validation + * entirely: while it is set, neither the {@link PoolConfiguration#getValidationQuery() validation query} nor a + * configured init SQL is executed. 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 any application, + * management or pool-internal thread - including a sweeper timer thread shared by every pool in the JVM - and + * must be thread-safe. * - * @param connection the underlying (non-proxied) {@link Connection} object to test + * @param connection the underlying {@link Connection} to test - not the pool's proxied handle, though the + * object itself may be a driver or third-party wrapper * @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/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties index b7d49bb1d3..fb85f6978f 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties @@ -164,6 +164,7 @@ poolProperties.validatorCannotInstantiate=An object of class {0} cannot be insta poolProperties.validatorClassNotFound=The class {0} cannot be found. poolProperties.validatorInaccessible=The class {0} or its no-arg constructor are inaccessible. poolProperties.validatorNotValidator=The class {0} does not implement the Validator interface. +poolProperties.validatorNotInEffect=A custom validator was configured as class {0} but is not in effect. Validation will use the configured validation query instead. pooledConnection.cannotReestablish=A connection once released, can't be reestablished. pooledConnection.commitFailed=Failed to commit transaction diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPoolProperties.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPoolProperties.java index 52dd0c3365..6068cb857f 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPoolProperties.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPoolProperties.java @@ -16,6 +16,11 @@ */ package org.apache.tomcat.jdbc.pool; +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.ObjectInputStream; +import java.io.ObjectOutputStream; +import java.sql.Connection; import java.util.ArrayList; import java.util.List; import java.util.Properties; @@ -914,4 +919,38 @@ class TestPoolProperties { Assertions.assertTrue(none.toString().contains("jdbcInterceptors=null"), "a null string must print as null, was: " + none); } + + /** The implementation a configured validator must be rebuilt to. */ + public static class RebuiltValidator implements Validator { + @Override + public boolean validate(Connection connection, int validateAction) { + return true; + } + } + + @Test + void serializationRebuildsTheValidatorConfiguredByName() throws Exception { + PoolProperties props = new PoolProperties(); + props.setValidatorClassName(RebuiltValidator.class.getName()); + Assertions.assertNotNull(props.getValidator(), + "configuring by class name must build the instance"); + + ByteArrayOutputStream bytes = new ByteArrayOutputStream(); + try (ObjectOutputStream out = new ObjectOutputStream(bytes)) { + out.writeObject(props); + } + PoolProperties read; + try (ObjectInputStream in = + new ObjectInputStream(new ByteArrayInputStream(bytes.toByteArray()))) { + read = (PoolProperties) in.readObject(); + } + + // The instance is transient and the name alone does not validate: + // without the read-side replay, the deserialized configuration + // reports the validator by name while the query path does the work. + Assertions.assertNotNull(read.getValidator(), + "the deserialized configuration must not report a validator it does not run"); + Assertions.assertTrue(read.getValidator() instanceof RebuiltValidator, + "the replay must rebuild the configured class"); + } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestValidation.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestValidation.java index 1a06bbb407..209c052e45 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestValidation.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestValidation.java @@ -206,6 +206,33 @@ class TestValidation extends JdbcPoolBaseTest { "the pool must serve again once validation passes"); } + @Test + void aConfiguredValidatorReplacesTheInitSql() throws Exception { + // The contract the Validator javadoc states: a configured init SQL + // arms the init validation even with no test flag set, and the + // validator replaces the statement rather than running beside it. + // The statement is executable and observable - it stamps the + // session variable the suite uses as a session probe - so a pool + // that ran it next to the validator, tolerantly or not, would be + // caught by the marker staying null. + CountingValidator validator = new CountingValidator(); + DataSource ds = newDataSource(); + ds.getPoolProperties().setValidator(validator); + ds.getPoolProperties().setInitSQL("SET @POOL_TEST_MARKER = 7"); + ds.getPoolProperties().setInitialSize(0); + ds.createPool(); + + try (Connection c = ds.getConnection()) { + Assertions.assertNull(sessionMarker(c), + "the configured init SQL must not run while a validator replaces it"); + } + Assertions.assertEquals(1, validator.actions.size(), + "the validator must be consulted exactly once, on the new session"); + Assertions.assertEquals(Integer.valueOf(PooledConnection.VALIDATE_INIT), + validator.actions.get(0), + "a configured init SQL must arm the init action with no test flag set"); + } + @Test void validationTerminatesTheTransactionOpenOnTheSession() throws Exception { // Corrected contract (legacy TestValidation pinned "no transaction --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
