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 6f463a41a75e737b15f1c137f244e503f43c2e7c Author: opencode <[email protected]> AuthorDate: Sun Oct 25 21:44:36 2026 +0100 Act on rollbackOnReturn and commitOnReturn regardless of defaultAutoCommit terminateTransaction() gated the whole feature on the pool-level defaultAutoCommit property being explicitly false, so a runtime setAutoCommit(false) on a pool with the default configuration returned connections to the idle queue inside the previous borrower's transaction. The options now act on the returned connection's actual transaction state; rollback keeps precedence over commit. --- .../apache/tomcat/jdbc/pool/ConnectionPool.java | 17 +-- .../apache/tomcat/jdbc/pool/PoolConfiguration.java | 5 +- .../pool/TestTerminateTransactionOnReturn.java | 137 +++++++++++++++++++++ webapps/docs/changelog.xml | 9 ++ 4 files changed, 158 insertions(+), 10 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 bfe9608296..64c895ca3b 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 @@ -998,15 +998,16 @@ public class ConnectionPool { */ protected boolean terminateTransaction(PooledConnection con) { try { - if (Boolean.FALSE.equals(con.getPoolProperties().getDefaultAutoCommit())) { - if (this.getPoolProperties().getRollbackOnReturn()) { - boolean autocommit = con.getConnection().getAutoCommit(); - if (!autocommit) { + // terminate the transaction of any connection that is returned + // while still in one; the pool-level defaultAutoCommit property + // must not gate this, the connection's own state is what matters + boolean doRollback = getPoolProperties().getRollbackOnReturn(); + boolean doCommit = !doRollback && getPoolProperties().getCommitOnReturn(); + if (doRollback || doCommit) { + if (!con.getConnection().getAutoCommit()) { + if (doRollback) { con.getConnection().rollback(); - } - } else if (this.getPoolProperties().getCommitOnReturn()) { - boolean autocommit = con.getConnection().getAutoCommit(); - if (!autocommit) { + } else { con.getConnection().commit(); } } diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolConfiguration.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolConfiguration.java index a67f5d5508..35dabb6667 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolConfiguration.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolConfiguration.java @@ -818,7 +818,8 @@ public interface PoolConfiguration { /** * Set to true if you want the connection pool to commit any pending transaction when a connection is returned. * The default value is false, as this could result in committing data. - * This parameter is only looked at if the {@link #getDefaultAutoCommit()} returns false + * This parameter is ignored if {@link #setRollbackOnReturn(boolean)} is also set to true. + * The commit is only performed when the returned connection is actually in a transaction. * @param commitOnReturn set to true if the pool should call {@link java.sql.Connection#commit()} when a connection is returned to the pool. * Default is false */ @@ -835,7 +836,7 @@ public interface PoolConfiguration { /** * Set to true if you want the connection pool to rollback any pending transaction when a connection is returned. * The default value is false, as this could result in committing data. - * This parameter is only looked at if the {@link #getDefaultAutoCommit()} returns false + * The rollback is only performed when the returned connection is actually in a transaction. * @param rollbackOnReturn set to true if the pool should call {@link java.sql.Connection#rollback()} when a connection is returned to the pool. * Default is false */ 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 new file mode 100644 index 0000000000..bd2b296cca --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestTerminateTransactionOnReturn.java @@ -0,0 +1,137 @@ +/* + * 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.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; + +/** + * rollbackOnReturn/commitOnReturn must terminate the transaction of a + * connection that was left in a transaction by its borrower, regardless of + * the pool-level defaultAutoCommit property - the documented semantics key + * off the connection's actual state, and the property must not act as a + * silent kill switch for the feature. + */ +class TestTerminateTransactionOnReturn extends JdbcPoolBaseTest { + + 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 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 rollbackOnReturnIsNotGatedOnDefaultAutoCommitFalse() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setRollbackOnReturn(true); + // defaultAutoCommit is deliberately left at its default (null) + 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(); // returned mid-transaction -> must be rolled back + + assertEquals(0, adminRowCount(t), + "uncommitted work must not survive the return"); + // The pool reuses the same idle session; a still-open transaction + // would make the previous borrower's work visible right here. + try (Connection c2 = ds.getConnection()) { + assertEquals(0, rowCount(c2, t), + "the next borrower must not observe the previous " + + "borrower's uncommitted work"); + } + } + + @Test + void commitOnReturnIsNotGatedOnDefaultAutoCommitFalse() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setCommitOnReturn(true); + // defaultAutoCommit is deliberately left at its default (null) + 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(); // returned mid-transaction -> must be committed + + assertEquals(1, adminRowCount(t), + "commitOnReturn must persist the work on return, even " + + "without defaultAutoCommit=false"); + } + + @Test + void withoutEitherFlagTheTransactionSurvivesTheReturn() throws Exception { + DataSource ds = newDataSource(); + // neither rollbackOnReturn nor commitOnReturn is set + 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(); // returned mid-transaction -> pool must not touch it + + try (Connection c2 = ds.getConnection()) { + assertEquals(1, rowCount(c2, t), + "without either flag the pool must leave the " + + "transaction untouched"); + } + } + + @Test + void rollbackWinsWhenBothFlagsAreSet() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setRollbackOnReturn(true); + 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(); // rollback takes precedence over commit + + assertEquals(0, adminRowCount(t), + "when both flags are set, rollbackOnReturn takes " + + "precedence"); + } +} diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index e0fedd5673..36e5b52980 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -1030,6 +1030,15 @@ inflating the pool size and allowing more physical connections than <code>maxActive</code>. (remm) </fix> + <fix> + Correct a defect that meant <code>rollbackOnReturn</code> and + <code>commitOnReturn</code> were silently ignored unless + <code>defaultAutoCommit</code> was explicitly set to + <code>false</code>, so a connection returned while still in a + transaction could be handed to the next borrower inside that + transaction. These options now act on the actual transaction state + of the connection being returned. (remm) + </fix> </changelog> </subsection> <subsection name="Other"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
