This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 8926ef07a75304f2f9c775c9136da07d0a45dcaa Author: remm <[email protected]> AuthorDate: Wed Oct 28 18:36:31 2026 +0100 Terminate a pending transaction by rollback when validating A validation that reached the database ended whatever transaction was open on the session by committing it. That made a health check commit work the previous borrower had left uncommitted, which silently defeated the guarantee that pending work is terminated by being discarded: validation runs before the state restoring interceptor sees the connection on borrow, on return and from the evictor, so by the time the restore rolled the transaction back there was nothing left to roll back, and with a validation interval the commit came and went intermittently. Terminate the transaction with a rollback instead, on both built-in check paths, and treat a rollback that fails as a failed validation: a session whose transaction cannot be ended is in an unknown state and the pool takes it out of service. The property the commit-on-validate behavior was introduced for, that a transaction opened by the validation itself must not leak into the next borrow, is kept: the rollback ends it just as well, and a read-only check has nothing to commit of its own. On return, the configured commit or rollback of the returned connection now runs before the return validation, so the explicit choice of commitOnReturn is honored rather than being overridden by the validation, and rollbackOnReturn is no longer defeated by a validation that reached the database first. A pool using a custom validator is unchanged: the validator replaces the check and owns the session state it observes. Cover it with tests that borrow, return and idle validation leave the previous borrower's work discarded on the same physical session, that return validation does not override either configured termination, and that the transaction is still left terminated in place after validation. --- .../apache/tomcat/jdbc/pool/ConnectionPool.java | 25 +++- .../apache/tomcat/jdbc/pool/PooledConnection.java | 54 +++++---- .../pool/TestTerminateTransactionOnReturn.java | 52 ++++++++ .../apache/tomcat/jdbc/pool/TestValidation.java | 64 ++++++---- .../TestPendingTransactionAcrossBorrowers.java | 133 +++++++++++++++++++++ webapps/docs/changelog.xml | 11 ++ 6 files changed, 293 insertions(+), 46 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java index 0bf15edc5f..1803275c18 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java @@ -1065,11 +1065,19 @@ public class ConnectionPool { boolean doRollback = getPoolProperties().getRollbackOnReturn(); boolean doCommit = !doRollback && getPoolProperties().getCommitOnReturn(); if (doRollback || doCommit) { - if (!con.getConnection().getAutoCommit()) { + Connection raw = con.getConnection(); + if (raw == null) { + // defensive: the pool's call path reaches here only on + // a session that is not closed; with nothing to + // terminate there is nothing to fail, and the caller's + // own closed checks take the connection out of service + return true; + } + if (!raw.getAutoCommit()) { if (doRollback) { - con.getConnection().rollback(); + raw.rollback(); } else { - con.getConnection().commit(); + raw.commit(); } } } @@ -1105,10 +1113,17 @@ public class ConnectionPool { if (isClosed()) { return true; } - if (!con.validate(action)) { + // Terminate the transaction before validating. When the pool is + // configured to commit or roll back on return, that is the explicit + // choice for the pending work, and a validation that runs first + // would end the transaction with its own rollback and silently + // override a configured commitOnReturn. With the termination first, + // a validation below only ever sees a transaction the return path + // deliberately left open. + if (!terminateTransaction(con)) { return true; } - if (!terminateTransaction(con)) { + if (!con.validate(action)) { return true; } return false; 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 a350d069cb..c2e92943b3 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 @@ -568,6 +568,18 @@ public class PooledConnection implements PooledConnectionMBean { * false if the validation failed. The caller should close the connection if false is returned since a session could have been left in * an unknown state during initialization. A connection that was never connected, or has already been disconnected, is always reported * as invalid. + * <p> + * A validation that runs its built-in check against the database also + * ends any transaction that is open on the session when the check + * completes - a pool configured with a custom {@link Validator} is not, + * as the validator replaces the check and owns the session state it + * observes. The transaction ended is the one the check itself may have + * started, or one a previous borrower left open. It is ended with a + * rollback, never with a commit: the check is read-only and a pool + * health check must not commit application work the application itself + * never committed. When that rollback fails the session state is + * unknown, so the validation is reported as failed and the pool takes + * the connection out of service. */ public boolean validate(int validateAction,String sql) { if (connection == null) { @@ -634,7 +646,7 @@ public class PooledConnection implements PooledConnectionMBean { } if (query == null) { - boolean transactionCommitted = false; + boolean transactionTerminated = false; int validationQueryTimeout = poolProperties.getValidationQueryTimeout(); if (validationQueryTimeout < 0) { validationQueryTimeout = 0; @@ -642,8 +654,15 @@ public class PooledConnection implements PooledConnectionMBean { try { if (connection.isValid(validationQueryTimeout)) { this.lastValidated = now; - transactionCommitted = silentlyCommitTransactionIfNeeded(); - return true; + // Terminate - by rollback, never by commit - any + // transaction the isValid() call opened, or that a + // previous borrower left open: a read-only health check + // must not commit application work. A session whose + // transaction cannot be terminated is in an unknown + // state, so it fails the validation and the pool takes + // the connection out of service. + transactionTerminated = silentlyRollbackTransactionIfNeeded(); + return transactionTerminated; } else { if (getPoolProperties().getLogValidationErrors()) { log.error("isValid() returned false."); @@ -658,13 +677,13 @@ public class PooledConnection implements PooledConnectionMBean { } return false; } finally { - if (!transactionCommitted) { + if (!transactionTerminated) { silentlyRollbackTransactionIfNeeded(); } } } - boolean transactionCommitted = false; + boolean transactionTerminated = false; Statement stmt = null; try { stmt = connection.createStatement(); @@ -677,8 +696,14 @@ public class PooledConnection implements PooledConnectionMBean { stmt.execute(query); stmt.close(); this.lastValidated = now; - transactionCommitted = silentlyCommitTransactionIfNeeded(); - return true; + // Terminate - by rollback, never by commit - any transaction + // the validation query opened, or that a previous borrower left + // open: a read-only health check must not commit application + // work. A session whose transaction cannot be terminated is in + // an unknown state, so it fails the validation and the pool + // takes the connection out of service. + transactionTerminated = silentlyRollbackTransactionIfNeeded(); + return transactionTerminated; } catch (Exception ex) { if (getPoolProperties().getLogValidationErrors()) { log.error("SQL Validation error", ex); @@ -690,7 +715,7 @@ public class PooledConnection implements PooledConnectionMBean { } } finally { - if (!transactionCommitted) { + if (!transactionTerminated) { silentlyRollbackTransactionIfNeeded(); } } @@ -698,19 +723,6 @@ public class PooledConnection implements PooledConnectionMBean { } //validate - private boolean silentlyCommitTransactionIfNeeded() { - try { - if (!connection.getAutoCommit()) { - connection.commit(); - } - return true; - } catch (SQLException e) { - log.debug("Failed to commit transaction", e); - } - return false; - } - - private boolean silentlyRollbackTransactionIfNeeded() { try { if (!connection.getAutoCommit()) { diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestTerminateTransactionOnReturn.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestTerminateTransactionOnReturn.java index 32fd7808c2..1dcc50d1fc 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestTerminateTransactionOnReturn.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestTerminateTransactionOnReturn.java @@ -134,4 +134,56 @@ class TestTerminateTransactionOnReturn extends JdbcPoolBaseTest { "when both flags are set, rollbackOnReturn takes " + "precedence"); } + + /* + * With testOnReturn enabled, shouldClose() runs the validation and the + * transaction termination on the same return. The validation ends any + * open transaction with a rollback, so the configured termination has + * to run first: otherwise the validation would silently override both + * flags - committing when rollbackOnReturn is set, and discarding work + * when commitOnReturn is set. + */ + private DataSource validatingOnReturn() { + DataSource ds = newDataSource(); + ds.getPoolProperties().setTestOnReturn(true); + ds.getPoolProperties().setValidationQuery("SELECT 1"); + ds.getPoolProperties().setValidationInterval(0); + return ds; + } + + @Test + void returnValidationDoesNotOverrideRollbackOnReturn() throws Exception { + DataSource ds = validatingOnReturn(); + ds.getPoolProperties().setRollbackOnReturn(true); + String t = createTable(ds); + + Connection c1 = ds.getConnection(); + c1.setAutoCommit(false); + try (Statement st = c1.createStatement()) { + st.executeUpdate("INSERT INTO " + t + " VALUES (1)"); + } + c1.close(); // terminate first, then validate + + Assertions.assertEquals(0, adminRowCount(t), + "the return validation must not turn a configured rollback " + + "into a commit"); + } + + @Test + void returnValidationDoesNotOverrideCommitOnReturn() throws Exception { + DataSource ds = validatingOnReturn(); + ds.getPoolProperties().setCommitOnReturn(true); + String t = createTable(ds); + + Connection c1 = ds.getConnection(); + c1.setAutoCommit(false); + try (Statement st = c1.createStatement()) { + st.executeUpdate("INSERT INTO " + t + " VALUES (1)"); + } + c1.close(); // terminate first, then validate + + Assertions.assertEquals(1, adminRowCount(t), + "the return validation must not discard the work a " + + "configured commitOnReturn promised to commit"); + } } 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 ad73287220..1a06bbb407 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,50 @@ class TestValidation extends JdbcPoolBaseTest { "the pool must serve again once validation passes"); } + @Test + void validationTerminatesTheTransactionOpenOnTheSession() throws Exception { + // Corrected contract (legacy TestValidation pinned "no transaction + // must be running" after a validation - that property still holds): + // a validation that reaches the database ends whatever transaction + // is open on the session - the one its own read-only check may have + // started, or one its borrower left behind - but it ends it with a + // rollback. A health check must never commit application work the + // application itself never committed, so what used to be committed + // here is now discarded. The termination happens in place, on the + // reused session. + DataSource ds = validatingDataSource("SELECT 1"); + ds.getPoolProperties().setDefaultAutoCommit(Boolean.FALSE); + ds.getPoolProperties().setTestOnReturn(true); + String table = uniqueTable(); + try (Connection admin = adminConnection(); + Statement st = admin.createStatement()) { + st.execute("CREATE TABLE " + table + " (v INT)"); + } + + Connection first = ds.getConnection(); + setSessionMarker(first, 13); + try (Statement st = first.createStatement()) { + st.executeUpdate("INSERT INTO " + table + " VALUES (1)"); + } + first.close(); // returned without an explicit commit, autoCommit is off + + Assertions.assertEquals(0, adminRowCount(table), + "the validation on return must leave no open transaction " + + "behind, and it must not commit the pending work"); + try (Connection second = ds.getConnection()) { + Assertions.assertEquals(Integer.valueOf(13), sessionMarker(second), + "the session must have been reused - the transaction was " + + "terminated in place, not by a reconnect"); + try (java.sql.ResultSet rs = second.createStatement() + .executeQuery("SELECT COUNT(*) FROM " + table)) { + rs.next(); + Assertions.assertEquals(0, rs.getInt(1), + "the next borrower must not inherit the open " + + "transaction"); + } + } + } + @Test void theValidationIntervalSuppressesRepeatedChecks() throws Exception { CountingValidator validator = new CountingValidator(); @@ -233,26 +277,6 @@ class TestValidation extends JdbcPoolBaseTest { "with the interval switched off the borrow must validate"); } - @Test - void validationCommitsTheTransactionOpenOnTheSession() throws Exception { - // Documented behaviour: PooledConnection.validate() finishes with a - // silent commit, so that a validated connection never carries an - // open transaction into the next borrow. Legacy TestValidation - // pinned the same contract ("no transaction must be running"). - DataSource ds = validatingDataSource("SELECT 1"); - ds.getPoolProperties().setDefaultAutoCommit(Boolean.FALSE); - ds.getPoolProperties().setTestOnReturn(true); - String table = uniqueTable(); - - try (Connection c = ds.getConnection(); Statement st = c.createStatement()) { - st.execute("CREATE TABLE " + table + " (v INT)"); - st.executeUpdate("INSERT INTO " + table + " VALUES (1)"); - } // returned without an explicit commit, autoCommit is off - - Assertions.assertEquals(1, adminRowCount(table), - "the validation on return must leave no open transaction behind"); - } - /** Records every validation the pool asks it to perform. */ private static final class CountingValidator implements Validator { private final List<Integer> actions = new ArrayList<>(); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestPendingTransactionAcrossBorrowers.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestPendingTransactionAcrossBorrowers.java index c1ca7582c1..ec4dd2d5a1 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestPendingTransactionAcrossBorrowers.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestPendingTransactionAcrossBorrowers.java @@ -25,6 +25,8 @@ import org.junit.jupiter.api.Test; import org.apache.tomcat.jdbc.JdbcPoolBaseTest; import org.apache.tomcat.jdbc.pool.DataSource; +import org.apache.tomcat.jdbc.pool.ProxyConnection; +import org.apache.tomcat.jdbc.pool.PooledConnection; /** * A borrower that returns a connection while still in a transaction hands @@ -38,6 +40,11 @@ import org.apache.tomcat.jdbc.pool.DataSource; * defaults - but only when there are defaults to restore, since a pool that * uses the interceptor purely to cache the state must leave the pool's own * documented transaction handling untouched. + * Validation of the connection (testOnBorrow, testOnReturn, testWhileIdle) + * runs before any of this and is part of that handling: a validation that + * actually reaches the database must terminate a pending transaction the + * same way - by rollback, never by commit - and the tests below pin that + * for each validation mode as well. */ class TestPendingTransactionAcrossBorrowers extends JdbcPoolBaseTest { @@ -170,4 +177,130 @@ class TestPendingTransactionAcrossBorrowers extends JdbcPoolBaseTest { Assertions.assertEquals(0, adminRowCount(t), "the abandoned work must not have survived"); } + + /* + * Validation runs before the state restore on borrow and before the + * evictor ever lets reset() see the connection, so it must terminate a + * pending transaction the same way the restore does - by rollback. A + * health check is read-only and must never commit the work the + * application itself never committed. + */ + private DataSource enforcingPoolValidatingOn(String action) { + DataSource ds = enforcingPool(); + ds.getPoolProperties().setValidationQuery("SELECT 1"); + ds.getPoolProperties().setValidationInterval(0); + if ("borrow".equals(action)) { + ds.getPoolProperties().setTestOnBorrow(true); + } else if ("return".equals(action)) { + ds.getPoolProperties().setTestOnReturn(true); + } else if ("idle".equals(action)) { + ds.getPoolProperties().setTestWhileIdle(true); + } + return ds; + } + + private void assertReusableAndWorking(Connection second, String table, + int marker) throws Exception { + Assertions.assertEquals(Integer.valueOf(marker), sessionMarker(second), + "the same physical session must have been reused - the " + + "transaction is terminated in place, the " + + "connection is not discarded"); + Assertions.assertEquals(0, rowCount(second, table), + "the next borrower must not find itself inside the previous " + + "borrower's transaction"); + try (Statement st = second.createStatement(); + ResultSet rs = st.executeQuery("SELECT 1")) { + Assertions.assertTrue(rs.next(), + "the validated connection must still work"); + } + } + + @Test + void borrowValidationDoesNotCommitThePreviousBorrowersWork() throws Exception { + DataSource ds = enforcingPoolValidatingOn("borrow"); + String t = createTable(ds); + + Connection first = ds.getConnection(); + setSessionMarker(first, 41); + insertInTransaction(first, t); + first.close(); + + try (Connection second = ds.getConnection()) { + assertReusableAndWorking(second, t, 41); + } + Assertions.assertEquals(0, adminRowCount(t), + "the validation on borrow must not commit the work the " + + "previous borrower abandoned"); + } + + @Test + void returnValidationDoesNotCommitThePreviousBorrowersWork() throws Exception { + DataSource ds = enforcingPoolValidatingOn("return"); + String t = createTable(ds); + + Connection first = ds.getConnection(); + setSessionMarker(first, 42); + insertInTransaction(first, t); + first.close(); // the return validation runs here + + try (Connection second = ds.getConnection()) { + assertReusableAndWorking(second, t, 42); + } + Assertions.assertEquals(0, adminRowCount(t), + "the validation on return must not commit the work the " + + "previous borrower abandoned"); + } + + @Test + void idleValidationDoesNotCommitThePreviousBorrowersWork() throws Exception { + DataSource ds = enforcingPoolValidatingOn("idle"); + String t = createTable(ds); + + Connection first = ds.getConnection(); + PooledConnection pooled = + interceptorOf(first, ProxyConnection.class).getConnection(); + setSessionMarker(first, 43); + insertInTransaction(first, t); + first.close(); + + // The evictor's own check, invoked directly to keep the test + // deterministic; a live session must pass it. + Assertions.assertTrue(pooled.validate(PooledConnection.VALIDATE_IDLE), + "the idle validation must succeed on a live session"); + try (Connection second = ds.getConnection()) { + assertReusableAndWorking(second, t, 43); + } + Assertions.assertEquals(0, adminRowCount(t), + "the validation while idle must not commit the work the " + + "previous borrower abandoned"); + } + + @Test + void withoutConfiguredDefaultsValidationStillTerminatesTheTransaction() + throws Exception { + // Caching-only configuration: reset() leaves a pending transaction + // alone (pinned above), but a validation that actually reaches the + // database still ends it - by rollback. So with validation enabled + // the abandoned work does not survive to the next borrower either; + // the "transaction untouched" behavior only holds for pools whose + // validations do not run. + DataSource ds = newDataSource(); + ds.getPoolProperties().setJdbcInterceptors(ConnectionState.class.getSimpleName()); + ds.getPoolProperties().setValidationQuery("SELECT 1"); + ds.getPoolProperties().setValidationInterval(0); + ds.getPoolProperties().setTestOnBorrow(true); + String t = createTable(ds); + + Connection first = ds.getConnection(); + setSessionMarker(first, 44); + insertInTransaction(first, t); + first.close(); + + try (Connection second = ds.getConnection()) { + assertReusableAndWorking(second, t, 44); + } + Assertions.assertEquals(0, adminRowCount(t), + "the validation must discard - not commit - the pending " + + "work even when no defaults are configured"); + } } diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 23b78e344a..c8ba7b709a 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -1286,6 +1286,17 @@ terminally instead of leaving its future in a state nobody can complete. (remm) </fix> + <fix> + A JDBC pool validation that runs its built-in check against the + database now terminates a pending transaction with a rollback rather + than by committing it, so a health check no longer commits work the + application left uncommitted, and a validation whose rollback fails + is reported as failed so the connection is taken out of service; the + configured <code>commitOnReturn</code> or + <code>rollbackOnReturn</code> termination of a returned connection + now runs before the return validation that would otherwise override + it. (remm) + </fix> </changelog> </subsection> <subsection name="Other"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
