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 174f6caa963764d53fb834275505f2584ab79b1b Author: opencode <[email protected]> AuthorDate: Fri Oct 30 11:06:42 2026 +0100 Fail the borrow when a configured connection state cannot be restored Reported as Bugzilla #50571: when restoring the configured defaultAutoCommit, defaultReadOnly or defaultCatalog failed, ConnectionState.reset() logged the driver's SQLException and carried on, so the borrower received a session that was provably in a state the pool had been configured never to hand out - a defaultCatalog naming a database that does not exist, for example, left the application working on the session's original catalog with nothing surfaced to the application - and the interceptor's own transaction termination could even discard the previous borrower's work for a restore that never succeeded. Those three restores now throw an IllegalStateException carrying the driver exception and naming the configured value, which makes the borrow fail with an SQLException naming the interceptor and retires the connection, the same fate as a failed inspection in the rollback path and, for pools without the interceptor, the already documented connect-time behavior. Restoring the transaction isolation level keeps the previously logged-and-ignored behavior, since a refused isolation level degrades a preference rather than breaking the session semantics and a permanently unusable setting would otherwise block the pool; that exception is now stated in the javadoc. A parameterized test injects a driver that refuses each of the three setters and verifies the failed borrow, the retired session and the working next borrow. --- .../jdbc/pool/interceptor/ConnectionState.java | 30 +++- .../tomcat/jdbc/pool/TestConnectionSetup.java | 170 ++++++++++++++++++++- webapps/docs/changelog.xml | 11 ++ 3 files changed, 203 insertions(+), 8 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 4edb2b6856..5f1f0c2e3f 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 @@ -85,9 +85,12 @@ public class ConnectionState extends JdbcInterceptor { * 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. + * 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. * * @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 @@ -158,7 +161,13 @@ public class ConnectionState extends JdbcInterceptor { } } catch (SQLException x) { readOnly = null; - log.error("Unable to reset readonly state to connection.", x); + // The connection is known to sit in a state the pool was + // configured not to hand out, and the restore that would fix + // that failed, so it must not go to the next borrower. + // ConnectionPool.setupConnection() catches this, discards the + // connection and fails the borrow. + throw new IllegalStateException("Unable to restore the configured read-only state " + + defaultReadOnly + " on the connection.", x); } } if (resetAutoCommit) { @@ -169,7 +178,11 @@ public class ConnectionState extends JdbcInterceptor { } } catch (SQLException x) { autoCommit = null; - log.error("Unable to reset autocommit state to connection.", x); + // As for the read-only state above: the driver refused the + // configured auto-commit state, so the borrow fails and the + // connection is discarded. + throw new IllegalStateException("Unable to restore the configured auto-commit state " + + defaultAutoCommit + " on the connection.", x); } } if (resetCatalog) { @@ -180,7 +193,12 @@ public class ConnectionState extends JdbcInterceptor { } } catch (SQLException x) { catalog = null; - log.error("Unable to reset default catalog state to connection.", x); + // As for the read-only state above: the driver refused the + // configured catalog, so the borrower learns of the + // misconfiguration instead of getting a connection that was + // never pointed at the database the pool asks it to work in. + throw new IllegalStateException("Unable to restore the configured default catalog " + + defaultCatalog + " on the connection.", x); } } 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 47fd5b0083..b61215cd02 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 @@ -16,13 +16,28 @@ */ 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.ResultSet; +import java.sql.SQLException; +import java.sql.SQLFeatureNotSupportedException; import java.sql.Statement; +import java.util.Properties; +import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Consumer; +import java.util.logging.Logger; +import java.util.stream.Stream; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import org.junit.jupiter.params.provider.NullAndEmptySource; import org.junit.jupiter.params.provider.ValueSource; @@ -33,8 +48,9 @@ import org.apache.tomcat.jdbc.pool.interceptor.ConnectionState; * Expected functionality of the per-connection setup the pool performs * while a physical session is being established: the configured init SQL * is applied to the session, unusable configuration is normalised or - * tolerated instead of wedging the pool, and the session the application - * receives is in a defined state. + * 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. * * <p>Scenarios harvested from the retired legacy classes * {@code Bug54225} (blank init SQL), {@code Bug54978} (init SQL used for @@ -46,6 +62,14 @@ class TestConnectionSetup extends JdbcPoolBaseTest { private static final String INIT_MARKER = "SET @POOL_INIT_STATE = 'applied'"; + /* + * The Connection method the wrapping driver below should fail from with + * a SQLException, 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> THROW_FROM = new AtomicReference<>(); + private static String initMarker(Connection c) throws Exception { try (Statement st = c.createStatement(); ResultSet rs = st.executeQuery("SELECT @POOL_INIT_STATE")) { @@ -136,4 +160,146 @@ class TestConnectionSetup extends JdbcPoolBaseTest { } Assertions.assertEquals(1, ds.getIdle(), "the pool must still pool the connection"); } + + /* + * The read-only, auto-commit and catalog defaults the ConnectionState + * interceptor restores on every borrow. Unlike the isolation level above, + * a driver that refuses one of these leaves the session KNOWN to sit in a + * state the pool was configured never to hand out - swallowing that + * failure - the log-and-ignore of an unusable configured default that + * Bugzilla #50571 reports - delivered the wrong state to the application + * in silence. H2 accepts any catalog argument without a murmur, so the + * refusal is injected through the wrapping driver below. + */ + static Stream<Arguments> refusedRestores() { + return Stream.of( + Arguments.of("setCatalog", "no_such_catalog", + (Consumer<PoolConfiguration>) p -> p.setDefaultCatalog("no_such_catalog")), + Arguments.of("setReadOnly", "true", + (Consumer<PoolConfiguration>) p -> p.setDefaultReadOnly(Boolean.TRUE)), + Arguments.of("setAutoCommit", "false", + (Consumer<PoolConfiguration>) p -> p.setDefaultAutoCommit(Boolean.FALSE))); + } + + @ParameterizedTest(name = "{0}({1})") + @MethodSource("refusedRestores") + void aRefusedStateRestoreFailsTheBorrowAndRetiresTheConnection( + String method, String value, Consumer<PoolConfiguration> configureDefault) throws Exception { + DataSource ds = newDataSource(); + ds.setDriverClassName(FailingRestoreDriver.class.getName()); + ds.getPoolProperties().setJdbcInterceptors(ConnectionState.class.getName()); + configureDefault.accept(ds.getPoolProperties()); + ds.createPool(); + awaitSessions(1); + + THROW_FROM.set(method); + SQLException borrowFailure; + try { + borrowFailure = Assertions.assertThrows(SQLException.class, ds::getConnection, + "a borrow whose configured default could not be restored must fail"); + } finally { + THROW_FROM.set(null); + } + Assertions.assertTrue(borrowFailure.getMessage().contains(ConnectionState.class.getName()), + "the borrow failure must name the failing interceptor, but was: " + borrowFailure); + Throwable restoreFailure = borrowFailure.getCause(); + Assertions.assertInstanceOf(IllegalStateException.class, restoreFailure, + "the reset must report the failed restore through the pool's discard path"); + Assertions.assertTrue(restoreFailure.getMessage().contains(value), + "the restore failure must name the configured value '" + value + + "', but was: " + restoreFailure); + Assertions.assertInstanceOf(SQLException.class, restoreFailure.getCause(), + "the driver's refusal must survive as the cause"); + + // the wrong-state session was retired, and with it the pool kept + // neither a misconfigured session nor the slot it occupied + awaitSessions(0); + Assertions.assertEquals(0, ds.getIdle()); + Assertions.assertEquals(0, ds.getActive()); + Assertions.assertEquals(0, ds.getSize()); + + // one failed borrow does not wedge the pool: the next one is served + // by a fresh session + try (Connection again = ds.getConnection()) { + Assertions.assertTrue(again.createStatement().execute("SELECT 1"), + "the replacement connection must work"); + } + awaitSessions(1); + } + + /** + * 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. + */ + public static class FailingRestoreDriver 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( + FailingRestoreDriver.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 { + if (method.getName().equals(THROW_FROM.get())) { + throw new SQLException("driver refuses " + method.getName() + " with these arguments"); + } + try { + return method.invoke(vendor, args); + } catch (InvocationTargetException x) { + throw x.getCause(); + } + } + } } diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index c133424ae6..4391e89bde 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -1122,6 +1122,17 @@ publishing it. As before, no database session and no pool cleaner outlives the close, and the closure stays terminal. (remm) </fix> + <fix> + <bug>50571</bug>: A configured connection state (auto-commit, + read-only or catalog) that could not be restored on a pooled + connection was logged and ignored, and the connection was handed out + in the wrong state: a <code>defaultCatalog</code> naming a database + that does not exist, for example, left the application working on the + session's original catalog with nothing surfaced to the application. + The borrow now fails with an <code>SQLException</code> and the + connection is discarded. Restoring the transaction isolation level + retains the previously logged-and-ignored behavior. (remm) + </fix> </changelog> </subsection> <subsection name="Other"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
