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 7ee0f9b8a79e71b41b68811d41733a51a01230f5 Author: opencode <[email protected]> AuthorDate: Fri Nov 6 09:18:16 2026 +0100 Retry a failed XA/CPDS handle close When obtaining the logical connection failed and closing the XA or CPDS handle also failed, the handle reference was dropped and its physical session was leaked. Keep the handle when the close fails so the release path can retry it, and clear it only after a successful close. --- .../apache/tomcat/jdbc/pool/PooledConnection.java | 18 +- .../tomcat/jdbc/pool/util/LocalStrings.properties | 1 + .../jdbc/pool/TestPooledConnectionHandleLeak.java | 264 +++++++++++++++++++++ .../tomcat/jdbc/pool/TestXaConnectionLeak.java | 6 +- 4 files changed, 283 insertions(+), 6 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 3c854aee1f..cab2293e11 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 @@ -393,15 +393,19 @@ public class PooledConnection implements PooledConnectionMBean { if (failure != null) { // Do not strand the handle if the logical connection // could not be obtained, whatever the provider failed with. + boolean closed = false; try { pooled.close(); + closed = true; } catch (Throwable closeX) { ExceptionUtils.handleThrowable(closeX); if (closeX != failure) { failure.addSuppressed(closeX); } } - xaConnection = null; + if (closed) { + xaConnection = null; + } connection = null; throw wrapConnectFailure(failure); } @@ -451,15 +455,19 @@ public class PooledConnection implements PooledConnectionMBean { if (failure != null) { // Do not strand the handle if the logical connection // could not be obtained, whatever the provider failed with. + boolean closed = false; try { pooled.close(); + closed = true; } catch (Throwable closeX) { ExceptionUtils.handleThrowable(closeX); if (closeX != failure) { failure.addSuppressed(closeX); } } - cpdsConnection = null; + if (closed) { + cpdsConnection = null; + } connection = null; throw wrapConnectFailure(failure); } @@ -782,7 +790,7 @@ public class PooledConnection implements PooledConnectionMBean { * @param finalize if set to true, a call to {@link ConnectionPool#finalize(PooledConnection)} is called. */ private void disconnect(boolean finalize) { - if (isDiscarded() && connection == null) { + if (isDiscarded() && connection == null && xaConnection == null && cpdsConnection == null) { return; } setDiscarded(true); @@ -884,7 +892,9 @@ public class PooledConnection implements PooledConnectionMBean { // 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()) { + if (cpdsHandle != null || xaHandle != null) { + log.warn(MessageFactory.get("pooledConnection.unableToCloseUnderlyingConnection"), ignore); + } else if (log.isDebugEnabled()) { log.debug("Unable to close underlying SQL connection", ignore); } } diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties index 6e68da9feb..b6cad9e1b6 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties @@ -191,6 +191,7 @@ pooledConnection.isValidReturnedFalse=isValid() returned false. pooledConnection.rollbackFailed=Failed to rollback transaction pooledConnection.sqlValidationError=SQL Validation error pooledConnection.unableToClearWarnings=Unable to clear warnings on the connection. +pooledConnection.unableToCloseUnderlyingConnection=Unable to close the underlying XA/CPDS connection handle; the physical session may still be open. pooledConnection.unknownDataSource=DataSource is of unknown class:{0} pooledConnection.xaDataSourceNullConnection=XADataSource {0} returned a null connection for the XA connection handle diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPooledConnectionHandleLeak.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPooledConnectionHandleLeak.java new file mode 100644 index 0000000000..0aed81e01e --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPooledConnectionHandleLeak.java @@ -0,0 +1,264 @@ +/* + * 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.io.PrintWriter; +import java.sql.Connection; +import java.sql.SQLException; +import java.sql.SQLFeatureNotSupportedException; +import java.util.logging.Logger; + +import javax.sql.ConnectionEventListener; +import javax.sql.ConnectionPoolDataSource; +import javax.sql.StatementEventListener; +import javax.sql.XAConnection; +import javax.sql.XADataSource; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; + +/** + * A physical handle must stay reachable when obtaining its logical connection + * fails and closing the handle also fails. The caller's release path then gets + * a chance to retry the physical close instead of leaking the session. + */ +class TestPooledConnectionHandleLeak extends JdbcPoolBaseTest { + + private static abstract class FailingHandle { + + int closeAttempts; + boolean closed; + + void close() throws SQLException { + closeAttempts++; + if (closeAttempts == 1) { + throw new SQLException("close boom"); + } + closed = true; + } + + Connection getConnection() throws SQLException { + throw new SQLException("logical connection boom"); + } + } + + public static class FailingXaHandle extends FailingHandle implements XAConnection { + + @Override + public Connection getConnection() throws SQLException { + return super.getConnection(); + } + + @Override + public javax.transaction.xa.XAResource getXAResource() { + return null; + } + + @Override + public void close() throws SQLException { + super.close(); + } + + @Override + public void addConnectionEventListener(ConnectionEventListener listener) { + // NO-OP + } + + @Override + public void removeConnectionEventListener(ConnectionEventListener listener) { + // NO-OP + } + + @Override + public void addStatementEventListener(StatementEventListener listener) { + // NO-OP + } + + @Override + public void removeStatementEventListener(StatementEventListener listener) { + // NO-OP + } + } + + public static class FailingXaDataSource implements XADataSource { + + private final XAConnection connection; + + public FailingXaDataSource(XAConnection connection) { + this.connection = connection; + } + + @Override + public XAConnection getXAConnection() { + return connection; + } + + @Override + public XAConnection getXAConnection(String user, String password) { + return connection; + } + + @Override + public PrintWriter getLogWriter() { + return null; + } + + @Override + public void setLogWriter(PrintWriter out) { + // NO-OP + } + + @Override + public void setLoginTimeout(int seconds) { + // NO-OP + } + + @Override + public int getLoginTimeout() { + return 0; + } + + @Override + public Logger getParentLogger() throws SQLFeatureNotSupportedException { + throw new SQLFeatureNotSupportedException(); + } + } + + public static class FailingCpdsHandle extends FailingHandle implements javax.sql.PooledConnection { + + @Override + public Connection getConnection() throws SQLException { + return super.getConnection(); + } + + @Override + public void close() throws SQLException { + super.close(); + } + + @Override + public void addConnectionEventListener(ConnectionEventListener listener) { + // NO-OP + } + + @Override + public void removeConnectionEventListener(ConnectionEventListener listener) { + // NO-OP + } + + @Override + public void addStatementEventListener(StatementEventListener listener) { + // NO-OP + } + + @Override + public void removeStatementEventListener(StatementEventListener listener) { + // NO-OP + } + } + + public static class FailingConnectionPoolDataSource implements ConnectionPoolDataSource { + + private final javax.sql.PooledConnection connection; + + public FailingConnectionPoolDataSource(javax.sql.PooledConnection connection) { + this.connection = connection; + } + + @Override + public javax.sql.PooledConnection getPooledConnection() { + return connection; + } + + @Override + public javax.sql.PooledConnection getPooledConnection(String user, String password) { + return connection; + } + + @Override + public PrintWriter getLogWriter() { + return null; + } + + @Override + public void setLogWriter(PrintWriter out) { + // NO-OP + } + + @Override + public void setLoginTimeout(int seconds) { + // NO-OP + } + + @Override + public int getLoginTimeout() { + return 0; + } + + @Override + public Logger getParentLogger() throws SQLFeatureNotSupportedException { + throw new SQLFeatureNotSupportedException(); + } + } + + private PooledConnection newPooledConnection(PoolProperties properties) { + DataSource ds = newDataSource(); + properties.setJmxEnabled(false); + return new PooledConnection(properties, ds.getPool()); + } + + @Test + void aFailedXaHandleCloseIsRetriedByTheRelease() throws Exception { + FailingXaHandle handle = new FailingXaHandle(); + PoolProperties properties = new PoolProperties(); + properties.setDataSource(new FailingXaDataSource(handle)); + PooledConnection pc = newPooledConnection(properties); + + Assertions.assertThrows(SQLException.class, pc::connect, + "the logical connection failure must be reported"); + Assertions.assertEquals(1, handle.closeAttempts, + "the handle must have been closed once before the failure was reported"); + Assertions.assertSame(handle, pc.getXAConnection(), + "a handle whose close failed must remain reachable for a retry"); + + pc.release(); + Assertions.assertEquals(2, handle.closeAttempts, + "the release must retry the physical close"); + Assertions.assertTrue(handle.closed, "the retried close must have succeeded"); + Assertions.assertNull(pc.getXAConnection(), "the released handle must be cleared"); + } + + @Test + void aFailedCpdsHandleCloseIsRetriedByTheRelease() throws Exception { + FailingCpdsHandle handle = new FailingCpdsHandle(); + PoolProperties properties = new PoolProperties(); + properties.setDataSource(new FailingConnectionPoolDataSource(handle)); + PooledConnection pc = newPooledConnection(properties); + + Assertions.assertThrows(SQLException.class, pc::connect, + "the logical connection failure must be reported"); + Assertions.assertEquals(1, handle.closeAttempts, + "the handle must have been closed once before the failure was reported"); + + pc.release(); + Assertions.assertEquals(2, handle.closeAttempts, + "the release must retry the physical close"); + Assertions.assertTrue(handle.closed, "the retried close must have succeeded"); + } +} 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 de2f24bc62..635b2782d7 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 @@ -521,8 +521,10 @@ class TestXaConnectionLeak extends JdbcPoolBaseTest { e.getSuppressed()[0].getMessage()); Assertions.assertEquals(1, xads.handles.size()); - Assertions.assertEquals(1, xads.handles.get(0).closeCount.get(), - "the cleanup must attempt the close exactly once"); + Assertions.assertEquals(2, xads.handles.get(0).closeCount.get(), + "the failed close must be retried when the pooled connection is released"); + Assertions.assertTrue(xads.handles.get(0).physicalClosed.get(), + "the retried close must release the physical session"); awaitSessions(0); } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
