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 babbe9deb1bdc7fb9f0efb73a7000bc9c34a73d5 Author: opencode <[email protected]> AuthorDate: Thu Nov 5 11:43:43 2026 +0100 Release a connection when its creation fails with an Error ConnectionPool.createConnection caught only Exception. A non-fatal Error from the driver, typically a LinkageError raised by a stopped web application, skipped the error flag and the finally block never released the connection, so the maxActive slot taken before the attempt and the physical session were lost for the life of the pool. Catch Throwable, apply ExceptionUtils.handleThrowable so fatal errors still propagate, and report the failure as an SQLException. Apply the same treatment to the initialSize pre-load in init so a failed start follows the pool's start-up teardown. Add regression tests for a failed first borrow and for a failed pre-load. --- .../apache/tomcat/jdbc/pool/ConnectionPool.java | 33 ++++-- .../apache/tomcat/jdbc/pool/PooledConnection.java | 6 +- .../pool/TestThrowingDriverOnClaimedPaths.java | 116 +++++++++++++++++++++ .../pool/interceptor/TestInterceptorLifecycle.java | 65 ++++++++++++ webapps/docs/changelog.xml | 14 +++ 5 files changed, 225 insertions(+), 9 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 f82e013215..e26beab053 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 @@ -475,7 +475,8 @@ public class ConnectionPool { } finally { con.unlock(); } - } catch (Exception x) { + } catch (Throwable x) { + ExceptionUtils.handleThrowable(x); SQLException sx = new SQLException(MessageFactory.get("connectionPool.unableToRegisterMBean")); sx.initCause(x); // Nothing has been done to the connection, it can go back to the pool @@ -500,7 +501,8 @@ public class ConnectionPool { interceptor.setNext(handler); // Configure the last one to be held by the connection handler = interceptor; - } catch (Exception x) { + } catch (Throwable x) { + ExceptionUtils.handleThrowable(x); SQLException sx = new SQLException(MessageFactory.get("connectionPool.unableToInstantiateInterceptorChain")); sx.initCause(x); // Nothing has been done to the connection, it can go back to the pool @@ -526,7 +528,8 @@ public class ConnectionPool { next.reset(this, con); next = next.getNext(); } - } catch (Exception x) { + } catch (Throwable x) { + ExceptionUtils.handleThrowable(x); SQLException sx = new SQLException(MessageFactory.get("connectionPool.unableToResetInterceptor", (next != null ? next.getClass().getName() : "unknown"))); sx.initCause(x); @@ -583,7 +586,8 @@ public class ConnectionPool { invokeRequestHint(BEGIN_REQUEST, con.getConnection()); // Return the connection return connection; - } catch (Exception x) { + } catch (Throwable x) { + ExceptionUtils.handleThrowable(x); SQLException s = new SQLException(); s.initCause(x); // The reset walk above has already armed every interceptor of the chain for this borrow, and from here @@ -833,7 +837,8 @@ public class ConnectionPool { JdbcInterceptor interceptor = definition.getInterceptorClass().getConstructor().newInstance(); interceptor.setProperties(definition.getProperties()); interceptor.poolClosed(this); - } catch (Exception x) { + } catch (Throwable x) { + ExceptionUtils.handleThrowable(x); if (log.isDebugEnabled()) { log.debug("Unable to inform interceptor of pool closure.", x); } @@ -977,7 +982,8 @@ public class ConnectionPool { initialPool[i] = this.borrowConnection(0, null, null); // Don't wait, should be no contention } - } catch (SQLException x) { + } catch (Throwable x) { + ExceptionUtils.handleThrowable(x); log.error(MessageFactory.get("connectionPool.unableToCreateInitialConnections"), x); if (!poolProperties.isIgnoreExceptionOnPreLoad()) { org.apache.tomcat.jdbc.pool.jmx.ConnectionPool jmx = jmxPool; @@ -985,7 +991,12 @@ public class ConnectionPool { jmx.notify(org.apache.tomcat.jdbc.pool.jmx.ConnectionPool.NOTIFY_INIT, getStackTrace(x)); } close(true); - throw x; + if (x instanceof SQLException) { + throw (SQLException) x; + } + SQLException ex = new SQLException(x.getMessage()); + ex.initCause(x); + throw ex; } } finally { // Return the members as idle to the pool. These hand-backs are @@ -1679,7 +1690,13 @@ public class ConnectionPool { // and clean up throw new SQLException(MessageFactory.get("connectionPool.validationQueryFailed")); } - } catch (Exception e) { + } catch (Throwable e) { + // A non-fatal Error from a driver or wrapper (typically a + // LinkageError from a stopped web application) is an ordinary + // failed creation here. Guarding only Exception would skip the + // error flag and the finally block's release, stranding the + // connection and the maxActive slot it already owns. + ExceptionUtils.handleThrowable(e); error = true; if (log.isDebugEnabled()) { log.debug("Unable to create a new JDBC connection.", e); 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 19e993495d..52430a9173 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 @@ -1266,7 +1266,11 @@ public class PooledConnection implements PooledConnectionMBean { if (first) { try { disconnect(true); - } catch (Exception x) { + } catch (Throwable x) { + // A non-fatal Error from the driver's close is a failed + // disconnect, not a reason to skip the first-CAS result the + // pool uses to retire the slot and count the release. + ExceptionUtils.handleThrowable(x); if (log.isDebugEnabled()) { log.debug("Unable to close SQL connection", x); } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestThrowingDriverOnClaimedPaths.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestThrowingDriverOnClaimedPaths.java index 5fd468abf1..7e02f28523 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestThrowingDriverOnClaimedPaths.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestThrowingDriverOnClaimedPaths.java @@ -70,11 +70,13 @@ class TestThrowingDriverOnClaimedPaths extends JdbcPoolBaseTest { */ private static final AtomicReference<String> THROW_FROM = new AtomicReference<>(); private static final AtomicBoolean FAIL_NEXT_CONNECT = new AtomicBoolean(); + private static final AtomicBoolean FAIL_NEXT_RESET = new AtomicBoolean(); private static final AtomicInteger CONNECTS = new AtomicInteger(); private static void resetDriver() { THROW_FROM.set(null); FAIL_NEXT_CONNECT.set(false); + FAIL_NEXT_RESET.set(false); CONNECTS.set(0); } @@ -196,6 +198,120 @@ class TestThrowingDriverOnClaimedPaths extends JdbcPoolBaseTest { assertNothingStranded(ds); } + @Test + void anErrorFromTheDriverOnThePhysicalCloseDoesNotLoseTheSlot() throws Exception { + resetDriver(); + DataSource ds = oneSlotPool(); + // No idle retention: the return destroys the connection, so the + // driver's close runs on the release path. + ds.getPoolProperties().setMaxIdle(0); + + Connection first = ds.getConnection(); + awaitSessions(1); + THROW_FROM.set("close"); + try { + // The driver's Error from the physical close must not escape the + // return and leave the destroyed connection counted in the pool. + Assertions.assertDoesNotThrow(first::close); + } finally { + THROW_FROM.set(null); + } + + assertNothingStranded(ds); + } + + @Test + void anErrorFromTheDriverOnInitialCreationFailsTheBorrowAndKeepsTheSlot() throws Exception { + resetDriver(); + DataSource ds = oneSlotPool(); + + FAIL_NEXT_CONNECT.set(true); + try { + // The first borrow takes a maxActive slot before the driver is + // called. The Error from connect() must fail the borrow as a + // checked exception and give the slot back. + Assertions.assertThrows(SQLException.class, ds::getConnection); + } finally { + FAIL_NEXT_CONNECT.set(false); + } + + Assertions.assertEquals(1, CONNECTS.get(), + "the borrow must have reached the driver's connect()"); + assertNothingStranded(ds); + } + + @Test + void anErrorFromTheDriverDuringTheInitialPreloadFailsPoolCreation() throws Exception { + resetDriver(); + DataSource ds = newDataSource(); + ds.setDriverClassName(ThrowingDriver.class.getName()); + ds.getPoolProperties().setInitialSize(1); + ds.getPoolProperties().setMaxActive(1); + ds.getPoolProperties().setTimeBetweenEvictionRunsMillis(0); + ds.getPoolProperties().setMaxWait(15000); + + FAIL_NEXT_CONNECT.set(true); + try { + Assertions.assertThrows(SQLException.class, ds::createPool); + } finally { + FAIL_NEXT_CONNECT.set(false); + } + + awaitSessions(0); + } + + @Test + void anErrorFromAnInterceptorDuringSetupDoesNotStrandTheConnection() throws Exception { + resetDriver(); + DataSource ds = newDataSource(); + ds.setDriverClassName(ThrowingDriver.class.getName()); + ds.getPoolProperties().setInitialSize(0); + ds.getPoolProperties().setMaxActive(1); + ds.getPoolProperties().setTimeBetweenEvictionRunsMillis(0); + ds.getPoolProperties().setMaxWait(15000); + ds.getPoolProperties().setJdbcInterceptors(ErrorOnResetInterceptor.class.getName()); + ds.createPool(); + + FAIL_NEXT_RESET.set(true); + try { + // setupConnection() has already claimed the connection into + // busy. The Error from the interceptor reset must be reported + // as a failed borrow and the connection taken out of service. + Assertions.assertThrows(SQLException.class, ds::getConnection); + } finally { + FAIL_NEXT_RESET.set(false); + } + + Assertions.assertEquals(0, ds.getPool().getIdle()); + Assertions.assertEquals(0, ds.getPool().getActive()); + Assertions.assertEquals(0, ds.getPool().getSize()); + awaitSessions(0); + try (Connection again = ds.getConnection()) { + Assertions.assertNotNull(again); + } + ds.close(); + awaitSessions(0); + } + + /** + * Interceptor failing the borrow-time reset once with an Error. Must be + * public with a no-argument constructor because it is instantiated by + * class name from {@code jdbcInterceptors}. + */ + public static class ErrorOnResetInterceptor extends JdbcInterceptor { + + public ErrorOnResetInterceptor() { + } + + @Override + public void reset(ConnectionPool parent, PooledConnection con) { + if (parent != null && FAIL_NEXT_RESET.compareAndSet(true, false)) { + throw new NoClassDefFoundError( + "the context that loaded this interceptor has stopped"); + } + } + } + /** * Driver handing out the real H2 connections wrapped in a proxy that * fails the currently armed method, or its own connect() when that is diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestInterceptorLifecycle.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestInterceptorLifecycle.java index 627429ca55..a7fa322073 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestInterceptorLifecycle.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestInterceptorLifecycle.java @@ -20,12 +20,16 @@ import java.sql.Connection; import java.sql.ResultSet; import java.sql.Statement; import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.apache.tomcat.jdbc.JdbcPoolBaseTest; +import org.apache.tomcat.jdbc.pool.ConnectionPool; import org.apache.tomcat.jdbc.pool.DataSource; +import org.apache.tomcat.jdbc.pool.JdbcInterceptor; +import org.apache.tomcat.jdbc.pool.PooledConnection; /** * Expected functionality of the interceptor configuration a resource @@ -90,6 +94,67 @@ class TestInterceptorLifecycle extends JdbcPoolBaseTest { "the report must see the work of every borrower"); } + @Test + void anErrorFromOneInterceptorDoesNotAbortTheRemainingCloseNotifications() throws Exception { + ErrorOnCloseInterceptor.CLOSED.set(0); + RecordingCloseInterceptor.CLOSED.set(0); + DataSource ds = newDataSource(); + ds.getPoolProperties().setJdbcInterceptors( + ErrorOnCloseInterceptor.class.getName() + ";" + + RecordingCloseInterceptor.class.getName()); + ds.createPool(); + + ds.close(); + + Assertions.assertEquals(1, ErrorOnCloseInterceptor.CLOSED.get(), + "the failing interceptor must have been told about the close"); + Assertions.assertEquals(1, RecordingCloseInterceptor.CLOSED.get(), + "an Error from one interceptor must not skip the remaining definitions"); + } + + /** + * Interceptor whose poolClosed() fails with an Error. Public with a + * no-argument constructor because the pool instantiates it by class name. + */ + public static class ErrorOnCloseInterceptor extends JdbcInterceptor { + + static final AtomicInteger CLOSED = new AtomicInteger(); + + public ErrorOnCloseInterceptor() { + } + + @Override + public void reset(ConnectionPool parent, PooledConnection con) { + // empty + } + + @Override + public void poolClosed(ConnectionPool pool) { + CLOSED.incrementAndGet(); + throw new NoClassDefFoundError( + "the context that loaded this interceptor has stopped"); + } + } + + /** Interceptor recording that it was told about the pool close. */ + public static class RecordingCloseInterceptor extends JdbcInterceptor { + + static final AtomicInteger CLOSED = new AtomicInteger(); + + public RecordingCloseInterceptor() { + } + + @Override + public void reset(ConnectionPool parent, PooledConnection con) { + // empty + } + + @Override + public void poolClosed(ConnectionPool pool) { + CLOSED.incrementAndGet(); + } + } + @Test void thePoolLifecycleIsReportedToTheInterceptor() throws Exception { DataSource ds = reportedPool(); diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 493a477909..6b345d5a24 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -1623,6 +1623,20 @@ the default pool name, which identified only the class loader and added no uniqueness, has been removed. (remm) </fix> + <fix> + Correct a defect in the JDBC connection pool where a non-fatal + <code>Error</code> raised by a driver or a wrapper while a connection + was being created, for example a <code>LinkageError</code> from a web + application that has stopped, escaped the creation guard. The + connection had already claimed one of the pool's + <code>maxActive</code> slots, so the slot and the physical session + were lost for the life of the pool; a pre-load of + <code>initialSize</code> connections that failed the same way also + bypassed the pool's start-up failure handling. The creation now treats + a non-fatal <code>Error</code> like any other failed creation, releases + the connection and reports the failure as an + <code>SQLException</code>. (remm) + </fix> </changelog> </subsection> <subsection name="Other"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
