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 bec768a63bf7887aaefa762ce6c444bc055b6df5 Author: opencode <[email protected]> AuthorDate: Mon Oct 26 10:49:06 2026 +0100 Do not restore connection state on top of an abandoned transaction reset() restored the configured isolation, read-only and auto-commit defaults while the previous borrower's transaction could still be open: transaction-scoped restores are refused or deferred by many drivers, and restoring auto-commit commits whatever the previous borrower left pending. The pending transaction is now terminated before the restores, and a connection whose state cannot be verified fails the borrow and is retired. Transaction handling is untouched for pools that configure no defaults, and for borrows that need no restore. --- .../jdbc/pool/interceptor/ConnectionState.java | 91 +++++++++-- .../TestPendingTransactionAcrossBorrowers.java | 173 +++++++++++++++++++++ webapps/docs/changelog.xml | 14 ++ 3 files changed, 262 insertions(+), 16 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/ConnectionState.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/ConnectionState.java index 3f2939d49d..5809d1d7a6 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/ConnectionState.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/ConnectionState.java @@ -17,6 +17,7 @@ package org.apache.tomcat.jdbc.pool.interceptor; import java.lang.reflect.Method; +import java.sql.Connection; import java.sql.SQLException; import org.apache.juli.logging.Log; @@ -80,6 +81,27 @@ public class ConnectionState extends JdbcInterceptor { protected String catalog = null; + /** + * Restores the connection state the pool was configured to enforce. + * The pool invokes this on every borrow, before the connection is + * handed to its next borrower. If the previous borrower returned the + * connection while still in a transaction, that transaction is + * terminated first: the isolation level and the read only state are + * transaction scoped on many drivers, and an open transaction would + * otherwise absorb the restores below (or, when auto-commit is + * restored, silently commit the previous borrower's uncommitted work). + * If terminating that transaction fails, the borrow fails and the pool + * retires the connection. Nothing is terminated when there is nothing + * to restore - no default is configured, or the cache already shows + * every configured default in place - so a pool that uses this + * interceptor only to cache the state does not pay for the + * termination. + * + * @param parent the pool the connection belongs to, or {@code null} + * when the interceptor chain is being torn down + * @param con the pooled connection being set up for its next + * borrower + */ @Override public void reset(ConnectionPool parent, PooledConnection con) { if (parent==null || con==null) { @@ -91,44 +113,81 @@ public class ConnectionState extends JdbcInterceptor { return; } PoolConfiguration poolProperties = parent.getPoolProperties(); - if (poolProperties.getDefaultTransactionIsolation()!=DataSourceFactory.UNKNOWN_TRANSACTIONISOLATION) { + final int defaultTransactionIsolation = poolProperties.getDefaultTransactionIsolation(); + final Boolean defaultReadOnly = poolProperties.getDefaultReadOnly(); + final Boolean defaultAutoCommit = poolProperties.getDefaultAutoCommit(); + final String defaultCatalog = poolProperties.getDefaultCatalog(); + boolean resetTransactionIsolation = + defaultTransactionIsolation != DataSourceFactory.UNKNOWN_TRANSACTIONISOLATION + && (transactionIsolation == null + || transactionIsolation.intValue() != defaultTransactionIsolation); + boolean resetReadOnly = defaultReadOnly != null + && (readOnly == null || readOnly.booleanValue() != defaultReadOnly.booleanValue()); + boolean resetAutoCommit = defaultAutoCommit != null + && (autoCommit == null || autoCommit.booleanValue() != defaultAutoCommit.booleanValue()); + boolean resetCatalog = defaultCatalog != null + && (catalog == null || !catalog.equals(defaultCatalog)); + if (resetTransactionIsolation || resetReadOnly || resetAutoCommit || resetCatalog) { + // The previous borrower may have returned this connection while + // its transaction was still open, unless the pool was configured + // to terminate transactions on return. Transaction isolation and + // read only state are transaction scoped on many drivers (the + // change is refused or deferred until the next transaction) and + // restoring auto-commit to true would commit whatever the + // previous borrower left pending. Terminate that transaction + // first so the restores below are actually applied. + try { + Connection raw = con.getConnection(); + if (!raw.getAutoCommit()) { + raw.rollback(); + } + } catch (SQLException x) { + // Whether the previous borrower's work survived is unknown, + // so the connection must not go to the next borrower. + // ConnectionPool.setupConnection() catches this, discards the + // connection and fails the borrow. + throw new IllegalStateException( + "Unable to check for, or terminate, the pending transaction before restoring the configured connection state.", x); + } + } + if (resetTransactionIsolation) { try { - if (transactionIsolation==null || transactionIsolation.intValue()!=poolProperties.getDefaultTransactionIsolation()) { - con.getConnection().setTransactionIsolation(poolProperties.getDefaultTransactionIsolation()); - transactionIsolation = Integer.valueOf(poolProperties.getDefaultTransactionIsolation()); + if (transactionIsolation==null || transactionIsolation.intValue()!=defaultTransactionIsolation) { + con.getConnection().setTransactionIsolation(defaultTransactionIsolation); + transactionIsolation = Integer.valueOf(defaultTransactionIsolation); } }catch (SQLException x) { transactionIsolation = null; log.error("Unable to reset transaction isolation state to connection.",x); } } - if (poolProperties.getDefaultReadOnly()!=null) { + if (resetReadOnly) { try { - if (readOnly==null || readOnly.booleanValue()!=poolProperties.getDefaultReadOnly().booleanValue()) { - con.getConnection().setReadOnly(poolProperties.getDefaultReadOnly().booleanValue()); - readOnly = poolProperties.getDefaultReadOnly(); + if (readOnly==null || readOnly.booleanValue()!=defaultReadOnly.booleanValue()) { + con.getConnection().setReadOnly(defaultReadOnly.booleanValue()); + readOnly = defaultReadOnly; } }catch (SQLException x) { readOnly = null; log.error("Unable to reset readonly state to connection.",x); } } - if (poolProperties.getDefaultAutoCommit()!=null) { + if (resetAutoCommit) { try { - if (autoCommit==null || autoCommit.booleanValue()!=poolProperties.getDefaultAutoCommit().booleanValue()) { - con.getConnection().setAutoCommit(poolProperties.getDefaultAutoCommit().booleanValue()); - autoCommit = poolProperties.getDefaultAutoCommit(); + if (autoCommit==null || autoCommit.booleanValue()!=defaultAutoCommit.booleanValue()) { + con.getConnection().setAutoCommit(defaultAutoCommit.booleanValue()); + autoCommit = defaultAutoCommit; } }catch (SQLException x) { autoCommit = null; log.error("Unable to reset autocommit state to connection.",x); } } - if (poolProperties.getDefaultCatalog()!=null) { + if (resetCatalog) { try { - if (catalog==null || (!catalog.equals(poolProperties.getDefaultCatalog()))) { - con.getConnection().setCatalog(poolProperties.getDefaultCatalog()); - catalog = poolProperties.getDefaultCatalog(); + if (catalog==null || (!catalog.equals(defaultCatalog))) { + con.getConnection().setCatalog(defaultCatalog); + catalog = defaultCatalog; } }catch (SQLException x) { catalog = null; 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 new file mode 100644 index 0000000000..eb047437ce --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestPendingTransactionAcrossBorrowers.java @@ -0,0 +1,173 @@ +/* + * 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.interceptor; + +import java.sql.Connection; +import java.sql.ResultSet; +import java.sql.Statement; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; +import org.apache.tomcat.jdbc.pool.DataSource; + +/** + * A borrower that returns a connection while still in a transaction hands + * that transaction to the pool. Without {@code rollbackOnReturn}/ + * {@code commitOnReturn} the pool leaves it open, and the next borrow's + * state restore then runs against it: on many drivers the isolation and + * read-only restores are deferred until the next transaction (or refused), + * and restoring auto-commit to true commits whatever the previous borrower + * left pending. The {@link ConnectionState} interceptor must therefore + * terminate the pending transaction before restoring the configured + * 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. + */ +class TestPendingTransactionAcrossBorrowers extends JdbcPoolBaseTest { + + private DataSource enforcingPool() { + DataSource ds = newDataSource(); + ds.getPoolProperties().setJdbcInterceptors(ConnectionState.class.getSimpleName()); + ds.getPoolProperties().setDefaultAutoCommit(Boolean.TRUE); + return ds; + } + + private String createTable(DataSource ds) throws Exception { + String t = uniqueTable(); + try (Connection c = ds.getConnection(); + Statement st = c.createStatement()) { + st.execute("CREATE TABLE " + t + " (v INT)"); + } + return t; + } + + private void insertInTransaction(Connection c, String table) throws Exception { + c.setAutoCommit(false); + try (Statement st = c.createStatement()) { + st.executeUpdate("INSERT INTO " + table + " VALUES (1)"); + } + } + + private int rowCount(Connection c, String table) throws Exception { + try (Statement st = c.createStatement(); + ResultSet rs = st.executeQuery("SELECT COUNT(*) FROM " + table)) { + rs.next(); + return rs.getInt(1); + } + } + + @Test + void restoringAutoCommitDoesNotCommitThePreviousBorrowersWork() throws Exception { + DataSource ds = enforcingPool(); + String t = createTable(ds); + + Connection first = ds.getConnection(); + setSessionMarker(first, 33); + insertInTransaction(first, t); + first.close(); // returned mid-transaction, pool must not touch it + + try (Connection second = ds.getConnection()) { + assertEquals(Integer.valueOf(33), sessionMarker(second), + "the same physical session must have been reused"); + // the borrow alone ran the restore; nothing else was done here + } + // Same session, so the only way the row can be visible from outside + // is that the restore committed it. + assertEquals(0, adminRowCount(t), + "restoring the configured auto-commit must not commit the " + + "work the previous borrower abandoned"); + } + + @Test + void theAbandonedTransactionIsNotInheritedByTheNextBorrower() throws Exception { + DataSource ds = enforcingPool(); + String t = createTable(ds); + + Connection first = ds.getConnection(); + setSessionMarker(first, 31); + insertInTransaction(first, t); + first.close(); + + try (Connection second = ds.getConnection()) { + assertEquals(Integer.valueOf(31), sessionMarker(second), + "the same physical session must have been reused"); + assertEquals(0, rowCount(second, t), + "the next borrower must not find itself inside the " + + "previous borrower's transaction"); + } + } + + @Test + void withoutConfiguredDefaultsTheInterceptorLeavesTheTransactionAlone() + throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setJdbcInterceptors(ConnectionState.class.getSimpleName()); + // no default auto-commit/isolation/read-only/catalog: the + // interceptor only caches the state, nothing is restored on borrow + String t = createTable(ds); + + Connection first = ds.getConnection(); + setSessionMarker(first, 32); + insertInTransaction(first, t); + first.close(); + + try (Connection second = ds.getConnection()) { + assertEquals(Integer.valueOf(32), sessionMarker(second), + "the same physical session must have been reused"); + assertEquals(1, rowCount(second, t), + "a caching-only configuration must keep the pool's " + + "documented transaction handling untouched"); + } + } + + @Test + void aRestoreNeededOnlyForTheIsolationLevelStillTerminatesTheTransaction() + throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setJdbcInterceptors(ConnectionState.class.getSimpleName()); + ds.getPoolProperties().setDefaultAutoCommit(Boolean.FALSE); + ds.getPoolProperties() + .setDefaultTransactionIsolation(Connection.TRANSACTION_READ_COMMITTED); + String t = createTable(ds); + + Connection first = ds.getConnection(); + setSessionMarker(first, 34); + // auto-commit stays at the configured default, so only the + // isolation level must be restored for the next borrow - and that + // restore is only honest if the abandoned transaction ends first + first.setTransactionIsolation(Connection.TRANSACTION_SERIALIZABLE); + try (Statement st = first.createStatement()) { + st.executeUpdate("INSERT INTO " + t + " VALUES (1)"); + } + first.close(); // still in a transaction (auto-commit default false) + + try (Connection second = ds.getConnection()) { + assertEquals(Integer.valueOf(34), sessionMarker(second), + "the same physical session must have been reused"); + assertEquals(Connection.TRANSACTION_READ_COMMITTED, + second.getTransactionIsolation(), + "the isolation level must be back at the pool default"); + assertEquals(0, rowCount(second, t), + "the isolation-only restore must still terminate the " + + "transaction the previous borrower left open"); + } + assertEquals(0, adminRowCount(t), + "the abandoned work must not have survived"); + } +} diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 439f20dc36..f7bf87763f 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -768,6 +768,20 @@ plain connections falsely did, with XA method calls on them failing with a NullPointerException. (remm) </fix> + <fix> + Correct a defect in the <code>ConnectionState</code> interceptor where + the configured connection defaults were restored while the previous + borrower's transaction was still open: the isolation level and the + read only state are transaction scoped on many drivers, so the + restore was refused or deferred and the previous borrower's state + reached the next borrower, and restoring auto-commit committed + whatever the previous borrower had left pending. The interceptor now + terminates the pending transaction before restoring the configured + state, and fails the borrow rather than handing out a connection it + cannot verify. Pools that configure no defaults, and borrows that + need no restore, keep the previous behaviour of leaving the + transaction untouched. (remm) + </fix> </changelog> </subsection> <subsection name="Other"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
