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 b8c1ae289d58667fee9606f25a2fa3225cf79146 Author: opencode <[email protected]> AuthorDate: Fri Oct 30 11:27:39 2026 +0100 Stop rolling back borrower work for a transaction isolation level that can never be restored When the pool was configured with a transaction isolation level the driver refuses, the failed restore nulled the state cache, so every subsequent borrow saw a pending restore, terminated whatever the previous borrower had left open, attempted the same doomed setTransactionIsolation and logged the error again - forever, on every borrow. Record the refused level per physical session instead: the refusal is attempted and logged once, the level stops counting as a pending restore for that session, and a later configuration of a different level (the pool reads the configured level on every borrow) or a replaced session retries as usual. Cover both paths with driver-level counter tests in TestConnectionSetup. --- .../jdbc/pool/interceptor/ConnectionState.java | 52 +++++++- .../tomcat/jdbc/pool/TestConnectionSetup.java | 138 ++++++++++++++++++++- 2 files changed, 180 insertions(+), 10 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 5f1f0c2e3f..5494e861ad 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 @@ -68,6 +68,21 @@ public class ConnectionState extends JdbcInterceptor { */ protected Integer transactionIsolation = null; + /** + * The transaction isolation level this physical session refused, or + * {@link DataSourceFactory#UNKNOWN_TRANSACTIONISOLATION} when the session has + * not refused one. A refused level does not count as needing a restore on later + * borrows, which keeps the pool out of a loop of doomed restore attempts, error + * logs and transaction terminations on every borrow; see {@link #reset}. The + * record covers the refused level only: the pool reads the configured level on + * every borrow, so a level this session has not refused - for instance one + * configured after the refusal - is restored as usual. Cleared when the session + * is replaced, so every physical session gets its own attempt. The interceptor + * instance belongs to one pooled connection, so a plain field needs no + * synchronisation. + */ + private int refusedIsolationLevel = DataSourceFactory.UNKNOWN_TRANSACTIONISOLATION; + /** * Cached read-only state of the connection. */ @@ -87,10 +102,18 @@ public class ConnectionState extends JdbcInterceptor { * is restored, silently commit the previous borrower's uncommitted work). If terminating that transaction fails, * the borrow fails and the pool retires the connection. So does a failed restore of the read-only, auto-commit or * catalog state: a connection known to sit in a state other than the configured default must not reach the next - * borrower. A transaction isolation level the driver refuses is the exception - it is logged and the connection - * stays in service, so the pool never strands borrowers over that setting. 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. + * borrower. A transaction isolation level the driver refuses is the exception: the connection stays in service, + * so the pool never strands borrowers over that setting. The refusal is logged once, at refusal time, and that + * level then stops counting as a pending restore on that session - later borrows neither retry it nor log it + * again nor terminate a pending transaction on its behalf. The record covers the refused level only: the pool + * reads the configured level on every borrow, so a level the session has not refused yet - one configured after + * the refusal, for instance - is attempted and applied as usual, and a replaced session retries even the same + * level. If the refused level was the only pending restore, the next borrower then inherits an open transaction + * left by the previous one, exactly as it does for a pool that configures no defaults at all. Nothing is + * terminated when there is nothing to restore - no default is configured, the + * cache already shows every configured default in place, or the only default that would need restoring is one + * this session refused - 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 @@ -104,6 +127,7 @@ public class ConnectionState extends JdbcInterceptor { transactionIsolation = null; readOnly = null; catalog = null; + refusedIsolationLevel = DataSourceFactory.UNKNOWN_TRANSACTIONISOLATION; return; } PoolConfiguration poolProperties = parent.getPoolProperties(); @@ -112,6 +136,7 @@ public class ConnectionState extends JdbcInterceptor { final Boolean defaultAutoCommit = poolProperties.getDefaultAutoCommit(); final String defaultCatalog = poolProperties.getDefaultCatalog(); boolean resetTransactionIsolation = defaultTransactionIsolation != DataSourceFactory.UNKNOWN_TRANSACTIONISOLATION && + refusedIsolationLevel != defaultTransactionIsolation && (transactionIsolation == null || transactionIsolation.intValue() != defaultTransactionIsolation); boolean resetReadOnly = defaultReadOnly != null && (readOnly == null || readOnly.booleanValue() != defaultReadOnly.booleanValue()); @@ -150,7 +175,23 @@ public class ConnectionState extends JdbcInterceptor { } } catch (SQLException x) { transactionIsolation = null; - log.error("Unable to reset transaction isolation state to connection.", x); + // A driver that refuses a level refuses it for this session, + // and the configured level is pool configuration that changes + // rarely, so retrying the refused level per borrow would keep + // the pool paying a doomed round trip, a termination of + // whatever the previous borrower left pending, and this error + // log - on every borrow, forever. Record the refused level so + // it stops counting as a pending restore; the next borrower + // inherits an open transaction the way it does on a pool that + // configures no defaults at all. A later configuration of a + // different level, or a replaced session, clears the way for a + // fresh attempt: this record names only the refused level. + // Report it here, at refusal time, as no later borrow will log + // it again. + refusedIsolationLevel = defaultTransactionIsolation; + log.error("Unable to reset transaction isolation state to connection. The configured level " + + defaultTransactionIsolation + " will not be retried on this session; it keeps its " + + "current isolation level.", x); } } if (resetReadOnly) { @@ -212,6 +253,7 @@ public class ConnectionState extends JdbcInterceptor { transactionIsolation = null; readOnly = null; catalog = null; + refusedIsolationLevel = DataSourceFactory.UNKNOWN_TRANSACTIONISOLATION; super.disconnected(parent, con, finalizing); } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestConnectionSetup.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestConnectionSetup.java index b61215cd02..f16ba34c02 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestConnectionSetup.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestConnectionSetup.java @@ -28,6 +28,7 @@ import java.sql.SQLException; import java.sql.SQLFeatureNotSupportedException; import java.sql.Statement; import java.util.Properties; +import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; import java.util.logging.Logger; @@ -50,7 +51,9 @@ import org.apache.tomcat.jdbc.pool.interceptor.ConnectionState; * is applied to the session, unusable configuration is normalised or * tolerated instead of wedging the pool, a state the driver refuses to * restore fails the borrow instead of silently handing out the wrong - * state, and the session the application receives is in a defined state. + * state, an isolation default the driver refuses for good converges at its + * first refusal instead of looping on every borrow, and the session the + * application receives is in a defined state. * * <p>Scenarios harvested from the retired legacy classes * {@code Bug54225} (blank init SQL), {@code Bug54978} (init SQL used for @@ -70,6 +73,23 @@ class TestConnectionSetup extends JdbcPoolBaseTest { */ private static final AtomicReference<String> THROW_FROM = new AtomicReference<>(); + /* + * Driver-level call counters, fed by the same wrapping proxy: tests + * that must pin what a borrow did to the physical session - not just + * what the session ended up looking like - count through these. The + * tests below are sequential and reset what they observe. + */ + private static final AtomicInteger ROLLBACKS = new AtomicInteger(); + private static final AtomicInteger ISOLATION_ATTEMPTS = new AtomicInteger(); + + 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); + } + } + private static String initMarker(Connection c) throws Exception { try (Statement st = c.createStatement(); ResultSet rs = st.executeQuery("SELECT @POOL_INIT_STATE")) { @@ -161,6 +181,105 @@ class TestConnectionSetup extends JdbcPoolBaseTest { Assertions.assertEquals(1, ds.getIdle(), "the pool must still pool the connection"); } + /* + * A permanently refused isolation default used to leave the pool in a + * loop: the failed restore re-nulled the cache, so every later borrow + * again saw a restore to do, again terminated whatever the previous + * borrower left pending - work the doomed restore could never make right + * - and again attempted, and logged, the same refused + * setTransactionIsolation. The refusal is now recorded per session, for + * the refused level (and logged there and then), so later borrows stop + * treating the level as a restore need: nothing is terminated, attempted + * or logged for it again, while a healthy restore still terminates as + * ever, and a later configuration of a different level is applied as + * usual. The honest + * trade-off, pinned below: with the refused level as the only restore + * need, a transaction the previous borrower left open now reaches the + * next one - the pre-existing behavior of a pool that configures no + * defaults at all, still covered by rollbackOnReturn or abandon handling + * where those are configured. + */ + @Test + void aRefusedIsolationRestoreConvergesAfterTheFirstRefusal() throws Exception { + DataSource ds = newDataSource(); + ds.setDriverClassName(FailingRestoreDriver.class.getName()); + ds.getPoolProperties().setJdbcInterceptors(ConnectionState.class.getName()); + // not a JDBC isolation level: H2 itself refuses it, nothing armed + ds.getPoolProperties().setDefaultTransactionIsolation(-55); + ds.createPool(); + String t = uniqueTable(); + try (Connection admin = adminConnection(); Statement st = admin.createStatement()) { + st.execute("CREATE TABLE " + t + " (v INT)"); + } + ROLLBACKS.set(0); + ISOLATION_ATTEMPTS.set(0); + + // The refusal surfaces on this first borrow; the borrower then + // returns the connection mid-transaction. + Connection first = ds.getConnection(); + setSessionMarker(first, 51); + first.setAutoCommit(false); + try (Statement st = first.createStatement()) { + st.executeUpdate("INSERT INTO " + t + " VALUES (1)"); + } + first.close(); // still in a transaction + Assertions.assertEquals(1, ISOLATION_ATTEMPTS.get(), + "the configured level must be attempted once on the first borrow"); + + // The two later borrows: pre-fix each of them rolled the abandoned + // row away and re-attempted (and re-logged) the doomed restore; now + // neither does, and the abandoned work survives to the new borrower. + for (int borrow = 2; borrow <= 3; borrow++) { + try (Connection later = ds.getConnection()) { + Assertions.assertEquals(Integer.valueOf(51), sessionMarker(later), + "borrow " + borrow + " must reuse the same physical session"); + Assertions.assertEquals(Connection.TRANSACTION_READ_COMMITTED, + later.getTransactionIsolation(), + "borrow " + borrow + " must be told the session's real " + + "isolation level, never the refused default"); + Assertions.assertEquals(1, rowCount(later, t), + "borrow " + borrow + " must not terminate the transaction " + + "the previous borrower left open"); + } + } + Assertions.assertEquals(0, ROLLBACKS.get(), + "no borrow after the refusal may roll the session back"); + Assertions.assertEquals(1, ISOLATION_ATTEMPTS.get(), + "the refused level must not be re-attempted on later borrows"); + Assertions.assertEquals(1, poolSessions(), "the refused session stays in service"); + Assertions.assertEquals(1, ds.getIdle(), "and the pool keeps pooling it"); + + // The refusal covers the refused level only: re-configuring a level + // this session has not refused (the JMX setter just writes the pool + // property, which the reset reads on every borrow) must be attempted + // on the same session, and applied. + ds.getPoolProperties().setDefaultTransactionIsolation(Connection.TRANSACTION_REPEATABLE_READ); + try (Connection corrected = ds.getConnection()) { + Assertions.assertEquals(Integer.valueOf(51), sessionMarker(corrected), + "the re-configured level must be applied on the same physical session"); + Assertions.assertEquals(Connection.TRANSACTION_REPEATABLE_READ, + corrected.getTransactionIsolation(), + "a level the session never refused must not be suppressed by the earlier refusal"); + } + Assertions.assertEquals(2, ISOLATION_ATTEMPTS.get(), + "the re-configured level must be attempted exactly once"); + Assertions.assertEquals(1, ROLLBACKS.get(), + "restoring the re-configured level must terminate the transaction left open"); + + // and it sticks: the cache shows it in place, so later borrows neither + // re-attempt nor terminate for it + try (Connection later = ds.getConnection()) { + Assertions.assertEquals(Integer.valueOf(51), sessionMarker(later), + "later borrows still reuse the same physical session"); + Assertions.assertEquals(Connection.TRANSACTION_REPEATABLE_READ, + later.getTransactionIsolation(), "the re-configured level must stick"); + } + Assertions.assertEquals(2, ISOLATION_ATTEMPTS.get(), + "the re-configured level must not be re-attempted once it is in place"); + Assertions.assertEquals(1, ROLLBACKS.get(), + "and no borrow after the restore may terminate a transaction for it"); + } + /* * The read-only, auto-commit and catalog defaults the ConnectionState * interceptor restores on every borrow. Unlike the isolation level above, @@ -230,9 +349,10 @@ class TestConnectionSetup extends JdbcPoolBaseTest { /** * Driver handing out the real H2 connections wrapped in a proxy that * fails the currently armed method with a SQLException, as a real driver - * refuses a catalog it cannot select or a state it cannot apply. - * Configured through {@code driverClassName}, so it must stay a public - * static class with a no-arg constructor. + * refuses a catalog it cannot select or a state it cannot apply, and + * that counts the {@code rollback()} and {@code setTransactionIsolation()} + * calls reaching the driver. Configured through {@code driverClassName}, + * so it must stay public with a no-arg constructor. */ public static class FailingRestoreDriver implements Driver { @@ -281,7 +401,10 @@ class TestConnectionSetup extends JdbcPoolBaseTest { } } - /* Fails the armed method by name; delegates everything else to H2. */ + /* + * Fails the armed method by name and counts the rollback() and + * setTransactionIsolation() calls; delegates everything else to H2. + */ private static class FailingHandler implements InvocationHandler { private final Connection vendor; @@ -295,6 +418,11 @@ class TestConnectionSetup extends JdbcPoolBaseTest { if (method.getName().equals(THROW_FROM.get())) { throw new SQLException("driver refuses " + method.getName() + " with these arguments"); } + if ("rollback".equals(method.getName())) { + ROLLBACKS.incrementAndGet(); + } else if ("setTransactionIsolation".equals(method.getName())) { + ISOLATION_ATTEMPTS.incrementAndGet(); + } try { return method.invoke(vendor, args); } catch (InvocationTargetException x) { --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
