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 61b03946dbfa63815c99cf2814478054eac766ba Author: opencode <[email protected]> AuthorDate: Thu Nov 5 14:37:08 2026 +0100 Close XA and CPDS handles when the logical connection fails with an Error PooledConnection.connectUsingDataSource caught only SQLException and RuntimeException around the logical connection obtained from an XA or ConnectionPoolDataSource handle, and the failure-path handle close caught the same. A non-fatal Error therefore left the handle, and the physical session it owns, open while the logical connection stayed null. disconnect() also closed handles only when a logical connection existed, so the field could not rescue it. Catch Throwable, handle fatal errors, close the handle on any failure, and close any non-null handle in disconnect. Add Error regression tests for both provider types. --- .../apache/tomcat/jdbc/pool/PooledConnection.java | 97 +++++++++++++--------- .../pool/TestConnectionPoolDataSourceLeak.java | 31 ++++++- .../tomcat/jdbc/pool/TestXaConnectionLeak.java | 33 +++++++- 3 files changed, 119 insertions(+), 42 deletions(-) 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 b599903c1a..d8a4813887 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 @@ -368,38 +368,33 @@ public class PooledConnection implements PooledConnectionMBean { // physical connection is only closed when the handle is closed, // so losing the handle here would leak the physical connection. xaConnection = pooled; - SQLException failure = null; - RuntimeException failureRuntime = null; + Throwable failure = null; try { connection = pooled.getConnection(); - } catch (SQLException e) { - failure = e; - } catch (RuntimeException e) { - failureRuntime = e; + } catch (Throwable t) { + ExceptionUtils.handleThrowable(t); + failure = t; } - if (connection == null && failure == null && failureRuntime == null) { + if (connection == null && failure == null) { // Contract-violating provider: a handle without a logical // connection is of no use either. failure = new SQLException( MessageFactory.get("pooledConnection.xaDataSourceNullConnection", xds)); } - if (failure != null || failureRuntime != null) { + if (failure != null) { // Do not strand the handle if the logical connection - // could not be obtained. - Exception origin = failure != null ? failure : failureRuntime; + // could not be obtained, whatever the provider failed with. try { pooled.close(); - } catch (SQLException | RuntimeException closeX) { - if (closeX != origin) { - origin.addSuppressed(closeX); + } catch (Throwable closeX) { + ExceptionUtils.handleThrowable(closeX); + if (closeX != failure) { + failure.addSuppressed(closeX); } } xaConnection = null; connection = null; - if (failureRuntime != null) { - throw failureRuntime; - } - throw failure; + throw wrapConnectFailure(failure); } // Once the session is fully established, hand out the pool's // wrapper rather than the driver's handle: it tracks the @@ -431,38 +426,33 @@ public class PooledConnection implements PooledConnectionMBean { // physical connection is only closed when the handle is closed, // so losing the handle here would leak the physical connection. cpdsConnection = pooled; - SQLException failure = null; - RuntimeException failureRuntime = null; + Throwable failure = null; try { connection = pooled.getConnection(); - } catch (SQLException e) { - failure = e; - } catch (RuntimeException e) { - failureRuntime = e; + } catch (Throwable t) { + ExceptionUtils.handleThrowable(t); + failure = t; } - if (connection == null && failure == null && failureRuntime == null) { + if (connection == null && failure == null) { // Contract-violating provider: a handle without a logical // connection is of no use either. failure = new SQLException( MessageFactory.get("pooledConnection.connectionPoolDataSourceNullConnection", ds)); } - if (failure != null || failureRuntime != null) { + if (failure != null) { // Do not strand the handle if the logical connection - // could not be obtained. - Exception origin = failure != null ? failure : failureRuntime; + // could not be obtained, whatever the provider failed with. try { pooled.close(); - } catch (SQLException | RuntimeException closeX) { - if (closeX != origin) { - origin.addSuppressed(closeX); + } catch (Throwable closeX) { + ExceptionUtils.handleThrowable(closeX); + if (closeX != failure) { + failure.addSuppressed(closeX); } } cpdsConnection = null; connection = null; - if (failureRuntime != null) { - throw failureRuntime; - } - throw failure; + throw wrapConnectFailure(failure); } } else { throw new SQLException(MessageFactory.get("pooledConnection.unknownDataSource", @@ -785,8 +775,12 @@ public class PooledConnection implements PooledConnectionMBean { return; } setDiscarded(true); - if (connection != null) { - notifyDisconnected(finalize); + if (connection != null || xaConnection != null || cpdsConnection != null) { + if (connection != null) { + notifyDisconnected(finalize); + } + // Close any handle too: an XA/CPDS handle owns the physical + // session even when obtaining its logical connection failed. closeSession(connection, xaConnection, cpdsConnection); } connection = null; @@ -816,6 +810,31 @@ public class PooledConnection implements PooledConnectionMBean { } } + /** + * Turns a failure while acquiring a logical connection into the exception a connect reports, preserving + * SQLException, RuntimeException and Error, and wrapping any other Throwable. + * + * @param failure the failure to rethrow + * + * @return a wrapper for a checked failure that is not an SQLException + * + * @throws SQLException the original SQLException + * @throws RuntimeException the original RuntimeException + * @throws Error the original Error + */ + private static SQLException wrapConnectFailure(Throwable failure) throws SQLException { + if (failure instanceof SQLException) { + throw (SQLException) failure; + } else if (failure instanceof RuntimeException) { + throw (RuntimeException) failure; + } else if (failure instanceof Error) { + throw (Error) failure; + } + SQLException ex = new SQLException(failure.getMessage()); + ex.initCause(failure); + return ex; + } + /** * Closes a physical session and the handle it came in on, if it came in on one. The session is passed in rather * than read from the fields, because a caller that has already taken on the session replacing this one - see @@ -848,7 +867,11 @@ public class PooledConnection implements PooledConnectionMBean { } else { session.close(); } - } catch (Exception ignore) { + } catch (Throwable ignore) { + // A non-fatal Error from the driver's close is a failed close, + // not a reason to let the pool lose the slot or leave the chain + // installed: the caller has already settled the release. + ExceptionUtils.handleThrowable(ignore); if (log.isDebugEnabled()) { log.debug("Unable to close underlying SQL connection", ignore); } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestConnectionPoolDataSourceLeak.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestConnectionPoolDataSourceLeak.java index 9f70e74876..8bdff5ef91 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestConnectionPoolDataSourceLeak.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestConnectionPoolDataSourceLeak.java @@ -57,6 +57,8 @@ class TestConnectionPoolDataSourceLeak extends JdbcPoolBaseTest { Collections.synchronizedList(new ArrayList<>()); /** Once set, the next handle refuses to hand out its connection. */ private volatile boolean failNextGetConnection; + /** Once set, the next handle fails to hand out its connection with an Error. */ + private volatile boolean errorFailNextGetConnection; /** Once set, the next handle hands out a null connection. */ private volatile boolean nullNextGetConnection; @@ -78,13 +80,15 @@ class TestConnectionPoolDataSourceLeak extends JdbcPoolBaseTest { // exist only for the pool, not for the database. boolean failGetConnection = failNextGetConnection; failNextGetConnection = false; + boolean errorFailGetConnection = errorFailNextGetConnection; + errorFailNextGetConnection = false; boolean nullGetConnection = nullNextGetConnection; nullNextGetConnection = false; // The physical connection is opened even on the failing paths: // a provider that opens the session and then fails to hand out // the logical connection is exactly the stranding scenario. Handle handle = new Handle(DriverManager.getConnection(url, "sa", ""), - failGetConnection, nullGetConnection); + failGetConnection, errorFailGetConnection, nullGetConnection); handles.add(handle); return handle; } @@ -118,14 +122,16 @@ class TestConnectionPoolDataSourceLeak extends JdbcPoolBaseTest { private final Connection physical; private final boolean failGetConnection; + private final boolean errorFailGetConnection; private final boolean nullGetConnection; private final AtomicBoolean physicalClosed = new AtomicBoolean(); private final AtomicBoolean logicalClosed = new AtomicBoolean(); Handle(Connection physical, boolean failGetConnection, - boolean nullGetConnection) { + boolean errorFailGetConnection, boolean nullGetConnection) { this.physical = physical; this.failGetConnection = failGetConnection; + this.errorFailGetConnection = errorFailGetConnection; this.nullGetConnection = nullGetConnection; } @@ -134,6 +140,10 @@ class TestConnectionPoolDataSourceLeak extends JdbcPoolBaseTest { if (failGetConnection) { throw new SQLException("simulated logical-connection failure"); } + if (errorFailGetConnection) { + throw new NoClassDefFoundError( + "simulated stopped context in the logical connection"); + } if (nullGetConnection) { return null; } @@ -284,6 +294,23 @@ class TestConnectionPoolDataSourceLeak extends JdbcPoolBaseTest { } } + @Test + void anErrorFromTheLogicalConnectionDoesNotStrandTheHandle() throws Exception { + TestCpds cpds = new TestCpds(getUrl()); + DataSource ds = newDataSource(); + ds.setDataSource(cpds); + ds.getPoolProperties().setInitialSize(0); + + cpds.errorFailNextGetConnection = true; + Assertions.assertThrows(SQLException.class, ds::getConnection, + "the non-fatal Error must reach the borrower as the checked failure a borrow reports"); + + Assertions.assertEquals(1, cpds.handles.size()); + Assertions.assertTrue(cpds.handles.get(0).physicalClosed.get(), + "the handle must close the physical session when its logical connection fails with an Error"); + awaitSessions(0); + } + @Test void aFailedLogicalConnectionDoesNotStrandTheHandle() throws Exception { TestCpds cpds = new TestCpds(getUrl()); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestXaConnectionLeak.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestXaConnectionLeak.java index 778e248eb6..de2f24bc62 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestXaConnectionLeak.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestXaConnectionLeak.java @@ -65,6 +65,8 @@ class TestXaConnectionLeak extends JdbcPoolBaseTest { private volatile boolean failNextGetConnection; /** Once set, the next handle throws a runtime failure instead. */ private volatile boolean runtimeFailNextGetConnection; + /** Once set, the next handle throws a non-fatal Error instead. */ + private volatile boolean errorFailNextGetConnection; /** Once set, the next handle hands out a null connection. */ private volatile boolean nullNextGetConnection; /** Once set, the next handle fails (after performing) its close. */ @@ -96,6 +98,8 @@ class TestXaConnectionLeak extends JdbcPoolBaseTest { failNextGetConnection = false; boolean runtimeFailGetConnection = runtimeFailNextGetConnection; runtimeFailNextGetConnection = false; + boolean errorFailGetConnection = errorFailNextGetConnection; + errorFailNextGetConnection = false; boolean nullGetConnection = nullNextGetConnection; nullNextGetConnection = false; boolean failClose = failNextClose; @@ -105,7 +109,7 @@ class TestXaConnectionLeak extends JdbcPoolBaseTest { // the logical connection is exactly the stranding scenario. Handle handle = new Handle(delegate.getXAConnection(), failGetConnection, runtimeFailGetConnection, - nullGetConnection, failClose); + errorFailGetConnection, nullGetConnection, failClose); handles.add(handle); return handle; } @@ -151,6 +155,7 @@ class TestXaConnectionLeak extends JdbcPoolBaseTest { private final javax.sql.XAConnection physical; private final boolean failGetConnection; private final boolean runtimeFailGetConnection; + private final boolean errorFailGetConnection; private final boolean nullGetConnection; private final boolean failClose; private final AtomicInteger closeCount = new AtomicInteger(); @@ -158,11 +163,12 @@ class TestXaConnectionLeak extends JdbcPoolBaseTest { private final AtomicBoolean logicalClosed = new AtomicBoolean(); Handle(javax.sql.XAConnection physical, boolean failGetConnection, - boolean runtimeFailGetConnection, boolean nullGetConnection, - boolean failClose) { + boolean runtimeFailGetConnection, boolean errorFailGetConnection, + boolean nullGetConnection, boolean failClose) { this.physical = physical; this.failGetConnection = failGetConnection; this.runtimeFailGetConnection = runtimeFailGetConnection; + this.errorFailGetConnection = errorFailGetConnection; this.nullGetConnection = nullGetConnection; this.failClose = failClose; } @@ -176,6 +182,10 @@ class TestXaConnectionLeak extends JdbcPoolBaseTest { throw new IllegalStateException( "simulated runtime logical-connection failure"); } + if (errorFailGetConnection) { + throw new NoClassDefFoundError( + "simulated stopped context in the logical connection"); + } if (nullGetConnection) { return null; } @@ -371,6 +381,23 @@ class TestXaConnectionLeak extends JdbcPoolBaseTest { awaitSessions(0); } + @Test + void anErrorFromTheLogicalConnectionDoesNotStrandTheHandle() throws Exception { + TestXaDs xads = new TestXaDs(getUrl()); + DataSource ds = newXaDataSource(xads); + ds.getPoolProperties().setInitialSize(0); + + xads.errorFailNextGetConnection = true; + Assertions.assertThrows(SQLException.class, ds::getConnection, + "the non-fatal Error must reach the borrower as the checked failure a borrow reports"); + + Assertions.assertEquals(1, xads.handles.size()); + Assertions.assertEquals(1, xads.handles.get(0).closeCount.get(), + "a handle whose logical connection failed with an Error must be closed"); + Assertions.assertTrue(xads.handles.get(0).physicalClosed.get()); + awaitSessions(0); + } + @Test void theHandleRequestedWithoutCredentialsIsGuardedToo() throws Exception { // The pool asks for a handle without credentials when it holds no --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
