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 5f1865ecaf362aa82f8fdc85c74ac224dcb0d3cb Author: opencode <[email protected]> AuthorDate: Tue Nov 3 16:21:29 2026 +0100 Let one validation read the validator once A custom validator is a live reconfiguration surface - setValidator answers to JMX and to the DataSource while the pool carries traffic - and the volatile field behind it was read four times per validation: once to decide whether to validate at all, once again to call, and once per log line that names the implementation. A swap landing between the first read and the second read made the second answer null, the dereference threw inside the very guard that turns a validator's throw into a failed validation, and the guard dutifully failed a healthy session's check - retiring a connection that no validator ever refused, under a log line blaming a validator that never ran. A swap to a different implementation answered the check with the replacement while the same read pattern could still name the first one in the report. The validation now reads the field once: the instance that decides the check, the instance that is called, and the instance the log lines name are one snapshot. The test installs a configuration that answers the null check with the validator and the read behind it with the replacement - the shape a swap-out takes from inside the validation - and pins that the check still runs, and runs on the validator the null check saw. --- .../apache/tomcat/jdbc/pool/PooledConnection.java | 19 +++++-- .../tomcat/jdbc/pool/TestThrowingValidator.java | 66 ++++++++++++++++++++++ 2 files changed, 81 insertions(+), 4 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 d6746f9cb2..19e993495d 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 @@ -940,10 +940,21 @@ public class PooledConnection implements PooledConnectionMBean { return true; } - if (poolProperties.getValidator() != null) { + // One read of a runtime-reconfigurable field. The validator is + // volatile and its setter is a supported live surface (JMX, the + // DataSource), so a second read could name a different validator + // than the one being called: the call would NPE on a null + // replacement and the guard below would read it as this session + // failing its own check, retiring a healthy connection that no + // validator ever refused; a swap to a different validator would + // run that one mid-borrow and name it in a log line about work + // the first one did. The snapshot decides, is called, and signs + // the log lines alike. + final Validator validator = poolProperties.getValidator(); + if (validator != null) { boolean valid; try { - valid = poolProperties.getValidator().validate(connection, validateAction); + valid = validator.validate(connection, validateAction); } catch (Throwable x) { ExceptionUtils.handleThrowable(x); // The SPI declares no checked exception, so throwing is the @@ -960,7 +971,7 @@ public class PooledConnection implements PooledConnectionMBean { // handleThrowable re-throws keep propagating. if (getPoolProperties().getLogValidationErrors()) { log.error(MessageFactory.get("pooledConnection.customValidationThrew", - poolProperties.getValidator()), x); + validator), x); } else if (log.isDebugEnabled()) { log.debug("Custom validation threw an exception.", x); } @@ -972,7 +983,7 @@ public class PooledConnection implements PooledConnectionMBean { } else { if (getPoolProperties().getLogValidationErrors()) { log.error(MessageFactory.get("pooledConnection.customValidationFailed", - poolProperties.getValidator())); + validator)); } return false; } 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 index 91edd4b710..110ae933a0 100644 --- 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 @@ -18,6 +18,7 @@ package org.apache.tomcat.jdbc.pool; import java.sql.Connection; import java.sql.SQLException; +import java.util.concurrent.atomic.AtomicInteger; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; @@ -240,6 +241,71 @@ class TestThrowingValidator extends JdbcPoolBaseTest { Assertions.assertEquals(0, ds.getPool().getIdle()); } + /** + * The shape a runtime swap of the validator takes when it races the + * call from inside: the null check is answered the validator, the + * next read is answered the replacement - here, none. + */ + private static final class SwapOnSecondRead extends PoolProperties { + private Validator real; + private int reads; + + void install(Validator validator) { + real = validator; + } + + @Override + public Validator getValidator() { + return (++reads) == 1 ? real : null; + } + } + + /** An implementation that vouches for every session it is asked about. */ + public static class CountingValidator implements Validator { + private final AtomicInteger calls = new AtomicInteger(); + + @Override + public boolean validate(Connection connection, int validateAction) { + calls.incrementAndGet(); + return true; + } + } + + @Test + void aswapOutOfTheValidatorMidCallDoesNotSpoilTheCheck() throws Exception { + CountingValidator validator = new CountingValidator(); + SwapOnSecondRead props = new SwapOnSecondRead(); + props.install(validator); + props.setUrl(getUrl()); + props.setDriverClassName("org.h2.Driver"); + props.setUsername("sa"); + props.setPassword(""); + props.setJmxEnabled(false); + props.setInitialSize(0); + props.setMinIdle(0); + props.setTestOnBorrow(true); + props.setValidationInterval(0); + props.setTimeBetweenEvictionRunsMillis(-1); + + ConnectionPool pool = new ConnectionPool(props); + PooledConnection con = new PooledConnection(props, pool); + con.connect(); + try { + // The swap races behind a validation the null check already + // admitted. A re-read null must not reach the call: validate + // would read the null dereference as its own throw, a failed + // check that retires a healthy session no validator ever + // refused - and a re-read of a replacement validator would + // answer the check while the log line blamed the first. + Assertions.assertTrue(con.validate(PooledConnection.VALIDATE_BORROW), + "the replacement read after the null check must not spoil the check"); + Assertions.assertEquals(1, validator.calls.get(), + "the validator the null check saw is the one that must have run"); + } finally { + con.release(); + } + } + @Test void rejectingValidatorOnBorrowFailsAndReleasesTheSlot() throws Exception { DataSource ds = throwingPool(); --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
