This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 9.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 633d336ace7da457628e149b48f9080d5309870d Author: opencode <[email protected]> AuthorDate: Fri Oct 30 17:05:52 2026 +0100 Keep an unchecked validation failure from stranding a connection PooledConnection.validate() caught SQLException around connection.isValid() and around the transaction termination that ends a validated session, so a driver or connection wrapper that reported a failure with an unchecked exception instead got that exception out of validate(). Commit aa2500b37d had already closed this hole for a custom Validator, for exactly this reason: a throw escaping from validate() during a borrow unwinds ConnectionPool.borrowConnection() after the connection has been polled out of the idle queue and before it is offered to the busy queue. The connection then belongs to neither queue while the pool size counter still counts it, so the abandoned and idle checks can never see it and the physical connection is never closed; every occurrence costs the pool one maxActive slot for the life of the pool. Measured on a one-slot pool, the application received an IllegalStateException where the DataSource contract promises a SQLException, the pool settled at size 1 with nothing idle and nothing busy, the server-side session stayed open, and every later borrow waited out maxWait and failed as a pool exhaustion error. The report also described the return and idle paths as exposed. They are not: dd21578f1d wraps the claimed section of returnConnection and 99bbc6e1d8 wraps the equivalent section of testAllIdle, both verified against the current sources, and silentlyCommitTransactionIfNeeded() no longer exists, so the two sites fixed here are the whole set. The claim that the validation-query branch already caught Exception is only half true, since the helper call in that branch's finally block sits outside that catch; a throw from there escaped even a successful query. validate() now treats an unchecked failure of isValid() or of the rollback that terminates a validated transaction like the checked one, which keeps validate() total and lets the existing caller behavior do the rest: the borrower gets a reconnect and revalidate followed by a checked SQLException, a returned connection is discarded, and an idle connection is evicted. Nothing is swallowed into a usable connection, because the validation fails either way and the session is destroyed, and there is one bounded retry per attempt rather than any new loop. Because the generic validation failure surfaced to the application says nothing about a driver that fails this way, and because a return that previously reached the unchecked catch in returnConnection used to log a warning naming the driver failure, the unchecked variants are now logged at warn rather than debug, and at error when logValidationErrors is set. A checked failure keeps its previous level. Add TestThrowingBuiltInValidation, driving a Connection proxy that throws RuntimeException from isValid(), from the auto-commit check and from the rollback that follows a successful validation, including the case where the query itself succeeds and only the finally-block helper throws. Each test asserts that the application gets a SQLException, that the pool does not lose the slot, that the session is closed and that the pool serves the next borrow; three further tests pin the logging levels, and the tests for the return and idle paths pin the end state those paths guarantee. --- .../apache/tomcat/jdbc/pool/PooledConnection.java | 39 +- .../jdbc/pool/TestThrowingBuiltInValidation.java | 520 +++++++++++++++++++++ 2 files changed, 558 insertions(+), 1 deletion(-) 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 01db62b72f..f04b8c3b50 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 @@ -762,9 +762,25 @@ public class PooledConnection implements PooledConnectionMBean { } return false; } - } catch (SQLException e) { + } catch (SQLException | RuntimeException e) { + // A driver or connection wrapper can also fail isValid() + // with an unchecked exception, which likewise means the + // session cannot be vouched for; like the checked failure + // it is reported as a failed validation, because a throw + // escaping from here unwinds the borrow path between the + // idle and busy queues and strands the pooled connection - + // permanently shrinking the pool if (getPoolProperties().getLogValidationErrors()) { log.error("isValid() failed.", e); + } else if (e instanceof RuntimeException) { + // Unlike the checked failure this anomaly would once + // have escaped as the driver's own throw; the caller + // is handed only a generic validation SQLException that + // says nothing about it, so it is logged at warn rather + // than debug - otherwise at the default log level a + // broken driver would fail every validation with no + // trace anywhere + log.warn("isValid() failed.", e); } else if (log.isDebugEnabled()) { log.debug("isValid() failed.", e); } @@ -827,6 +843,27 @@ public class PooledConnection implements PooledConnectionMBean { return true; } catch (SQLException e) { log.debug("Failed to rollback transaction", e); + } catch (RuntimeException e) { + // A failure to end the transaction is a failure whether the + // driver reports it checked or unchecked: the caller reads the + // false as a failed validation and takes the session out of + // service, while a throw escaping from here would escape even + // through a caller's finally block, outside the catch that + // converts it, and strand the pooled connection between the + // idle and busy queues + if (getPoolProperties().getLogValidationErrors()) { + log.error("Failed to rollback transaction", e); + } else { + // The unchecked variant is an anomaly of the same kind as + // a throwing custom Validator, and unlike the checked + // rollback failure the pool surfaces it only as the same + // generic validation failure as any other, so it is logged + // at warn rather than debug: otherwise at the default log + // level a driver that fails to end a transaction unchecked + // would destroy connection after connection with no trace + // anywhere + log.warn("Failed to rollback transaction", e); + } } return false; } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestThrowingBuiltInValidation.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestThrowingBuiltInValidation.java new file mode 100644 index 0000000000..734da7cbe3 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestThrowingBuiltInValidation.java @@ -0,0 +1,520 @@ +/* + * 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.lang.reflect.InvocationHandler; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.lang.reflect.Proxy; +import java.sql.Connection; +import java.sql.Driver; +import java.sql.DriverPropertyInfo; +import java.sql.SQLException; +import java.sql.SQLFeatureNotSupportedException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Properties; +import java.util.concurrent.atomic.AtomicReference; +import java.util.logging.Handler; +import java.util.logging.Level; +import java.util.logging.LogRecord; +import java.util.logging.Logger; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.juli.logging.LogFactory; +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; +import org.apache.tomcat.jdbc.pool.interceptor.StatementFinalizer; + +/** + * The built-in validation paths reach the database through the driver: + * the {@code isValid()} check and the transaction termination that ends + * the session the check leaves open. Both handle the checked + * {@link SQLException} a well-behaved driver throws when a call fails, + * but nothing in the JDBC contract stops a driver or a connection + * wrapper from failing one of them with an unchecked exception instead. + * Such a throw must be reported as a failed validation, not escape + * {@code validate()}: a borrow polls the connection out of the idle + * queue before validating and only offers it to the busy queue on + * success, so a throw escaping midway leaves the connection in neither + * queue while the size counter still counts it - invisible to + * checkAbandoned(), to testAllIdle() and to the close() drain alike, + * never disconnected, permanently shrinking the pool. The return path + * and the idle sweep claim their connection the same way before they + * validate, so they accept the same hazard. + */ +class TestThrowingBuiltInValidation extends JdbcPoolBaseTest { + + /* + * The method the pooled physical connections should fail from with a + * RuntimeException, suffixed with "=false" to fail by honestly + * returning false instead, or null to let everything through. Static, + * because the driver is instantiated by class name and the tests alone + * decide when the failure window opens and closes. + */ + private static final AtomicReference<String> FAIL_FROM = new AtomicReference<>(); + + /* One-slot pool on the failing driver, with no competing sweeper. */ + private DataSource oneSlotPool(boolean autoCommitOff) throws SQLException { + DataSource ds = newDataSource(); + ds.setDriverClassName(FailingDriver.class.getName()); + ds.getPoolProperties().setInitialSize(0); + // exactly one slot: a stranded connection exhausts the pool at once + ds.getPoolProperties().setMaxActive(1); + // the tests drive validation themselves; a background sweep would + // compete with the idle assertions below + ds.getPoolProperties().setTimeBetweenEvictionRunsMillis(0); + ds.getPoolProperties().setValidationInterval(0); + if (autoCommitOff) { + // an interceptor chain without the state interceptor, so + // connect() applies the autoCommit state to every (re)connect: + // the transaction termination then reaches the armed rollback + // on the new session of a reconnect as well as on the old one + ds.getPoolProperties().setJdbcInterceptors(StatementFinalizer.class.getName()); + ds.getPoolProperties().setDefaultAutoCommit(Boolean.FALSE); + } + ds.createPool(); + return ds; + } + + /* + * Park one idle connection, fail the named built-in validation call + * from the driver during the re-borrow, and require the failed borrow + * to cost the pool nothing: a checked SQLException at the application, + * the connection disconnected, its slot back in the size budget, and + * the next borrow working. This is exactly what an honest false from + * the same check already does. + */ + private void assertFailingBuiltInCheckOnBorrowDoesNotStrand(String armedMethod, + boolean autoCommitOff) throws Exception { + DataSource ds = oneSlotPool(autoCommitOff); + ds.getPoolProperties().setTestOnBorrow(true); + // the creation path is not validated (testOnConnect is off), the + // built-in check first runs when the returned connection is + // re-borrowed out of the idle queue + Connection first = ds.getConnection(); + awaitSessions(1); + first.close(); + Assertions.assertEquals(1, ds.getPool().getIdle()); + + FAIL_FROM.set(armedMethod); + SQLException sqlX; + try { + // an unchecked driver failure must reach the application as + // the checked SQLException the DataSource contract allows + sqlX = Assertions.assertThrows(SQLException.class, ds::getConnection); + } finally { + FAIL_FROM.set(null); + } + Assertions.assertTrue(sqlX.getMessage().contains("Failed to validate"), + "expected a validation failure, got: " + sqlX.getMessage()); + + // nothing stranded: neither queue holds the connection, the size + // budget got its slot back, and the physical session really was + // closed rather than left open outside both queues + awaitSessions(0); + Assertions.assertEquals(0, ds.getPool().getIdle()); + Assertions.assertEquals(0, ds.getPool().getActive()); + Assertions.assertEquals(0, ds.getPool().getSize()); + + // the single slot was not lost: the next borrow succeeds + try (Connection again = ds.getConnection()) { + Assertions.assertNotNull(again); + } + Assertions.assertEquals(0, ds.getPool().getActive()); + ds.close(); + awaitSessions(0); + } + + @Test + void uncheckedIsValidFailureOnBorrowDoesNotStrandTheSlot() throws Exception { + // the isValid() branch of validate() ran guarded only against the + // SQLException, so an unchecked throw escaped between the idle + // poll and the busy offer + assertFailingBuiltInCheckOnBorrowDoesNotStrand("isValid", false); + } + + @Test + void uncheckedAutoCommitCheckFailureOnBorrowDoesNotStrandTheSlot() throws Exception { + // the transaction termination runs even after a successful + // isValid(), through a helper that only handled the SQLException + assertFailingBuiltInCheckOnBorrowDoesNotStrand("getAutoCommit", false); + } + + @Test + void uncheckedRollbackFailureOnBorrowDoesNotStrandTheSlot() throws Exception { + // with the session in a transaction, the termination calls + // rollback() - the other failure point of the same helper + assertFailingBuiltInCheckOnBorrowDoesNotStrand("rollback", true); + } + + @Test + void uncheckedRollbackFailureAfterValidationQueryDoesNotEscapeTheFinally() + throws Exception { + // the validation-query branch catches Exception around its body, + // but the same termination runs from its finally block, outside + // that catch - an unchecked throw there escaped even the branch + // that was described as guarded + DataSource ds = oneSlotPool(true); + ds.getPoolProperties().setTestOnBorrow(true); + ds.getPoolProperties().setValidationQuery("SELECT 1"); + Connection first = ds.getConnection(); + awaitSessions(1); + first.close(); + Assertions.assertEquals(1, ds.getPool().getIdle()); + + FAIL_FROM.set("rollback"); + SQLException sqlX; + try { + sqlX = Assertions.assertThrows(SQLException.class, ds::getConnection); + } finally { + FAIL_FROM.set(null); + } + Assertions.assertTrue(sqlX.getMessage().contains("Failed to validate"), + "expected a validation failure, got: " + sqlX.getMessage()); + awaitSessions(0); + Assertions.assertEquals(0, ds.getPool().getSize()); + try (Connection again = ds.getConnection()) { + Assertions.assertNotNull(again); + } + ds.close(); + awaitSessions(0); + } + + @Test + void falseIsValidOnBorrowKeepsItsMeaning() throws Exception { + // control for the widened guards: an honest false from isValid() + // keeps its pre-existing meaning all the way through - the same + // end state the new throw-handling must reproduce + DataSource ds = oneSlotPool(false); + ds.getPoolProperties().setTestOnBorrow(true); + Connection first = ds.getConnection(); + awaitSessions(1); + first.close(); + FAIL_FROM.set("isValid=false"); + try { + Assertions.assertThrows(SQLException.class, ds::getConnection); + } finally { + FAIL_FROM.set(null); + } + awaitSessions(0); + Assertions.assertEquals(0, ds.getPool().getSize()); + try (Connection again = ds.getConnection()) { + Assertions.assertNotNull(again); + } + ds.close(); + awaitSessions(0); + } + + @Test + void uncheckedHelperFailureIsLoggedAtWarnByDefault() throws Exception { + // the pool hands the borrower only a generic validation + // SQLException, so an unchecked failure of the transaction + // termination must leave a trace in the pool's own log at the + // default settings: the helper logs it at warn with the driver's + // exception attached + DataSource ds = oneSlotPool(true); + ds.getPoolProperties().setTestOnBorrow(true); + Connection first = ds.getConnection(); + first.close(); + + try (ValidationLogCapture logs = new ValidationLogCapture()) { + FAIL_FROM.set("rollback"); + try { + Assertions.assertThrows(SQLException.class, ds::getConnection); + } finally { + FAIL_FROM.set(null); + } + logs.assertLoggedAt(Level.WARNING, "boom from rollback()"); + } + + // pinning the log must not change the outcome: the slot was not + // lost and the session really was closed + awaitSessions(0); + Assertions.assertEquals(0, ds.getPool().getSize()); + try (Connection again = ds.getConnection()) { + Assertions.assertNotNull(again); + } + ds.close(); + awaitSessions(0); + } + + @Test + void uncheckedHelperFailureIsLoggedAtErrorWhenLogValidationErrors() throws Exception { + // logValidationErrors promotes the failure the way it promotes the + // sibling validation branches', with the driver's exception + // attached either way + DataSource ds = oneSlotPool(true); + ds.getPoolProperties().setTestOnBorrow(true); + ds.getPoolProperties().setLogValidationErrors(true); + Connection first = ds.getConnection(); + first.close(); + + try (ValidationLogCapture logs = new ValidationLogCapture()) { + FAIL_FROM.set("rollback"); + try { + Assertions.assertThrows(SQLException.class, ds::getConnection); + } finally { + FAIL_FROM.set(null); + } + logs.assertLoggedAt(Level.SEVERE, "boom from rollback()"); + } + ds.close(); + awaitSessions(0); + } + + @Test + void uncheckedIsValidFailureIsLoggedAtWarnByDefault() throws Exception { + // the return path no longer reaches the caller-level guard that + // once warned about an unchecked validation throw, and the borrow + // path never had one: the widened isValid() catch itself must + // leave the driver's failure in the log at the default settings + DataSource ds = oneSlotPool(false); + ds.getPoolProperties().setTestOnBorrow(true); + Connection first = ds.getConnection(); + first.close(); + + try (ValidationLogCapture logs = new ValidationLogCapture()) { + FAIL_FROM.set("isValid"); + try { + Assertions.assertThrows(SQLException.class, ds::getConnection); + } finally { + FAIL_FROM.set(null); + } + logs.assertLoggedAt(Level.WARNING, "boom from isValid()"); + } + ds.close(); + awaitSessions(0); + } + + @Test + void uncheckedIsValidFailureOnReturnTakesTheConnectionOutOfService() throws Exception { + // pin: the return path already catches an unchecked throw around + // the whole claimed-connection section, and the fix in validate() + // now routes it through the failed validation instead; both must + // end the same way - close() complete(s) quietly and the + // connection that cannot be vouched for is discarded + DataSource ds = oneSlotPool(false); + ds.getPoolProperties().setTestOnReturn(true); + Connection first = ds.getConnection(); + awaitSessions(1); + + FAIL_FROM.set("isValid"); + try { + Assertions.assertDoesNotThrow(first::close); + } finally { + FAIL_FROM.set(null); + } + awaitSessions(0); + Assertions.assertEquals(0, ds.getPool().getIdle()); + Assertions.assertEquals(0, ds.getPool().getActive()); + Assertions.assertEquals(0, ds.getPool().getSize()); + try (Connection again = ds.getConnection()) { + Assertions.assertNotNull(again); + } + ds.close(); + awaitSessions(0); + } + + @Test + void uncheckedIsValidFailureOnIdleValidationEvictsTheConnection() throws Exception { + // pin: the idle sweep claims the connection before validating, so + // an escaped throw would strand it out of every queue and out of + // the next sweep; whether the sweep's own catch or the failed + // validation takes it out of service, it must end disconnected + DataSource ds = oneSlotPool(false); + ds.getPoolProperties().setTestWhileIdle(true); + Connection first = ds.getConnection(); + awaitSessions(1); + first.close(); + Assertions.assertEquals(1, ds.getPool().getIdle()); + + FAIL_FROM.set("isValid"); + try { + Assertions.assertDoesNotThrow(ds::testIdle); + } finally { + FAIL_FROM.set(null); + } + awaitSessions(0); + Assertions.assertEquals(0, ds.getPool().getIdle()); + Assertions.assertEquals(0, ds.getPool().getSize()); + // the evicted slot is available again + try (Connection again = ds.getConnection()) { + Assertions.assertNotNull(again); + } + ds.close(); + awaitSessions(0); + } + + /** + * Driver handing out the real H2 connections wrapped in a proxy that + * fails the named method with a RuntimeException (or, when the name + * is suffixed with "=false", by returning false). Configured through + * {@code driverClassName}, so it must stay a public static class with + * a no-arg constructor. + */ + public static class FailingDriver implements Driver { + + private final Driver delegate = new org.h2.Driver(); + + @Override + public Connection connect(String url, Properties info) throws SQLException { + Connection connection = delegate.connect(url, info); + if (connection == null) { + return null; + } + return (Connection) Proxy.newProxyInstance( + FailingDriver.class.getClassLoader(), + new Class<?>[] { Connection.class }, + new FailingHandler(connection)); + } + + @Override + public boolean acceptsURL(String url) throws SQLException { + return delegate.acceptsURL(url); + } + + @Override + public DriverPropertyInfo[] getPropertyInfo(String url, Properties info) throws SQLException { + return delegate.getPropertyInfo(url, info); + } + + @Override + public int getMajorVersion() { + return delegate.getMajorVersion(); + } + + @Override + public int getMinorVersion() { + return delegate.getMinorVersion(); + } + + @Override + public boolean jdbcCompliant() { + return delegate.jdbcCompliant(); + } + + @Override + public Logger getParentLogger() throws SQLFeatureNotSupportedException { + return delegate.getParentLogger(); + } + } + + /* Fails the armed method by name; delegates everything else to H2. */ + private static class FailingHandler implements InvocationHandler { + + private final Connection vendor; + + FailingHandler(Connection vendor) { + this.vendor = vendor; + } + + @Override + public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { + String armed = FAIL_FROM.get(); + if (armed != null) { + boolean honestFalse = armed.endsWith("=false"); + String name = honestFalse ? armed.substring(0, armed.length() - "=false".length()) : armed; + if (method.getName().equals(name)) { + if (honestFalse) { + return Boolean.FALSE; + } + throw new IllegalStateException("boom from " + name + "()"); + } + } + try { + return method.invoke(vendor, args); + } catch (InvocationTargetException x) { + throw x.getCause(); + } + } + } + + /* + * Captures what PooledConnection itself logs, following the handler- + * wiring approach TestAsyncBorrowFailureDiagnostics uses against + * ConnectionPool; the canary fails the test up front if the handler + * would observe nothing. + */ + private static final class ValidationLogCapture implements AutoCloseable { + private static final String CANARY = "pooledconnection-handler-wiring-canary"; + + private final Logger logger = Logger.getLogger(PooledConnection.class.getName()); + private final Level originalLevel = logger.getLevel(); + private final List<LogRecord> records = Collections.synchronizedList(new ArrayList<>()); + private final Handler handler = new Handler() { + @Override + public void publish(LogRecord record) { + records.add(record); + } + + @Override + public void flush() { + // no buffering + } + + @Override + public void close() { + // nothing to release + } + }; + + ValidationLogCapture() { + logger.addHandler(handler); + logger.setLevel(Level.ALL); + LogFactory.getLog(PooledConnection.class).error(CANARY); + if (records.stream().noneMatch(r -> CANARY.equals(r.getMessage()))) { + close(); + throw new AssertionError("the test handler must observe what this class logs"); + } + // the canary is wiring proof, not pinned behavior + records.clear(); + } + + /* + * Requires the driver's failure to appear at exactly the given + * level, with the driver's exception attached. It may appear more + * than once - a failed validation is retried once after a + * reconnect, and a failed termination is attempted again from the + * finally block - but every record of it must carry the level. + */ + void assertLoggedAt(Level level, String thrownMessage) { + boolean matched = false; + synchronized (records) { + for (LogRecord record : records) { + Throwable thrown = record.getThrown(); + if (thrown != null && String.valueOf(thrown.getMessage()).contains(thrownMessage)) { + Assertions.assertEquals(level, record.getLevel(), "driver failure logged at an" + + " unexpected level, so the pool's default settings would hide it: " + + record); + matched = true; + } + } + } + Assertions.assertTrue(matched, "expected the driver failure (" + thrownMessage + + ") to be logged with the exception attached, records: " + records); + } + + @Override + public void close() { + logger.removeHandler(handler); + logger.setLevel(originalLevel); + } + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
