This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 10.1.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit f689f5207b7211ea6022512caf4227ca190dec15 Author: opencode <[email protected]> AuthorDate: Tue Oct 27 09:03:12 2026 +0100 Close the PooledConnection handle obtained from a ConnectionPoolDataSource When the pool's dataSource pointed at a javax.sql.ConnectionPoolDataSource, connectUsingDataSource() obtained a javax.sql.PooledConnection handle, took the logical connection from it and discarded the handle. Per the JDBC specification, closing the logical connection only returns that handle to the provider; the physical connection stays open until the handle itself is closed. disconnect() closed only the logical connection, so every release, eviction or reconnection of such a connection leaked a physical database session while the pool's counters looked healthy, up to database-level session exhaustion. The pool now keeps the handle in a new cpdsConnection field and closes it in disconnect() instead of closing the logical connection alone. A single cleanup path also closes the handle, rather than stranding it, when the logical connection cannot be obtained, including a contract-violating provider that hands out a null connection; a close failure is attached to the original failure as suppressed. Closing the handle also makes a nested jdbc-pool used as CPDS behave correctly: the borrow is returned to the inner pool instead of the inner pool's raw physical connection being closed out from under it. Regression tests in TestConnectionPoolDataSourceLeak use a spec-faithful ConnectionPoolDataSource over real H2 connections and assert against server-side sessions: the release path, the alternate-username reconnect path and both failing-handle paths leak a session pre-fix and are clean post-fix. --- .../apache/tomcat/jdbc/pool/PooledConnection.java | 58 +++- .../pool/TestConnectionPoolDataSourceLeak.java | 324 +++++++++++++++++++++ 2 files changed, 377 insertions(+), 5 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 043e3cbd6b..0a8bf428ef 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 @@ -82,6 +82,14 @@ public class PooledConnection implements PooledConnectionMBean { * If using a XAConnection underneath. */ protected volatile javax.sql.XAConnection xaConnection; + /** + * If using a ConnectionPoolDataSource underneath, this is the + * javax.sql.PooledConnection handle obtained from it. Per the JDBC + * specification, closing the logical connection obtained from that + * handle does not close the physical connection, so the handle itself + * must be kept in order to close the physical connection. + */ + private volatile javax.sql.PooledConnection cpdsConnection; /** * When we track abandon traces, this string holds the thread dump */ @@ -292,10 +300,44 @@ public class PooledConnection implements PooledConnectionMBean { } } else if (poolProperties.getDataSource() instanceof javax.sql.ConnectionPoolDataSource){ javax.sql.ConnectionPoolDataSource ds = (javax.sql.ConnectionPoolDataSource)poolProperties.getDataSource(); - if (usr!=null && pwd!=null) { - connection = ds.getPooledConnection(usr, pwd).getConnection(); - } else { - connection = ds.getPooledConnection().getConnection(); + javax.sql.PooledConnection pooled = (usr!=null && pwd!=null) ? + ds.getPooledConnection(usr, pwd) : ds.getPooledConnection(); + // Keep the handle before obtaining the logical connection: the + // 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; + try { + connection = pooled.getConnection(); + } catch (SQLException e) { + failure = e; + } catch (RuntimeException e) { + failureRuntime = e; + } + if (connection == null && failure == null && failureRuntime == null) { + // Contract-violating provider: a handle without a logical + // connection is of no use either. + failure = new SQLException("ConnectionPoolDataSource "+ds+ + " returned a null connection for the pooled connection handle"); + } + if (failure != null || failureRuntime != null) { + // Do not strand the handle if the logical connection + // could not be obtained. + Exception origin = failure != null ? failure : failureRuntime; + try { + pooled.close(); + } catch (SQLException | RuntimeException closeX) { + if (closeX != origin) { + origin.addSuppressed(closeX); + } + } + cpdsConnection = null; + connection = null; + if (failureRuntime != null) { + throw failureRuntime; + } + throw failure; } } else { throw new SQLException("DataSource is of unknown class:"+(poolProperties.getDataSource()!=null?poolProperties.getDataSource().getClass():"null")); @@ -438,7 +480,12 @@ public class PooledConnection implements PooledConnectionMBean { + "physical connection can be closed.", x); } try { - if (xaConnection == null) { + if (cpdsConnection != null) { + // Closing the handle closes the physical connection + // and invalidates the logical connection; closing the + // logical connection alone would not. + cpdsConnection.close(); + } else if (xaConnection == null) { connection.close(); } else { xaConnection.close(); @@ -451,6 +498,7 @@ public class PooledConnection implements PooledConnectionMBean { } connection = null; xaConnection = null; + cpdsConnection = null; lastConnected = -1; if (finalize) { parent.finalize(this); 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 new file mode 100644 index 0000000000..9f70e74876 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestConnectionPoolDataSourceLeak.java @@ -0,0 +1,324 @@ +/* + * 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.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.DriverManager; +import java.sql.SQLException; +import java.sql.Statement; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.logging.Logger; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; + +/** + * When the pool obtains its connections from a + * {@link javax.sql.ConnectionPoolDataSource}, the JDBC specification keeps + * the physical connection open until the {@link javax.sql.PooledConnection} + * handle obtained from it is closed - closing the logical connection only + * returns that handle to the provider. The pool must therefore keep the + * handle and close it, or every disconnect leaks a database session. + */ +class TestConnectionPoolDataSourceLeak extends JdbcPoolBaseTest { + + /** + * A spec-faithful ConnectionPoolDataSource over real H2 connections: + * the logical connection obtained from a handle survives the logical + * close, and only closing the handle closes the physical connection. + */ + private static final class TestCpds implements javax.sql.ConnectionPoolDataSource { + + private final String url; + private final List<Handle> handles = + Collections.synchronizedList(new ArrayList<>()); + /** Once set, the next handle refuses to hand out its connection. */ + private volatile boolean failNextGetConnection; + /** Once set, the next handle hands out a null connection. */ + private volatile boolean nullNextGetConnection; + + TestCpds(String url) { + this.url = url; + } + + @Override + public javax.sql.PooledConnection getPooledConnection() throws SQLException { + return getPooledConnection("sa", ""); + } + + @Override + public javax.sql.PooledConnection getPooledConnection(String user, + String password) throws SQLException { + // Credentials are deliberately ignored: this fake exercises the + // CPDS handle lifecycle, not provider authentication, and the + // alternate-username reconnect test passes credentials that + // exist only for the pool, not for the database. + boolean failGetConnection = failNextGetConnection; + failNextGetConnection = 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); + handles.add(handle); + return handle; + } + + @Override + public java.io.PrintWriter getLogWriter() { + return null; + } + + @Override + public void setLogWriter(java.io.PrintWriter out) { + // no-op + } + + @Override + public void setLoginTimeout(int seconds) { + // no-op + } + + @Override + public int getLoginTimeout() { + return 0; + } + + @Override + public Logger getParentLogger() { + return Logger.getLogger(TestCpds.class.getName()); + } + + private static final class Handle implements javax.sql.PooledConnection { + + private final Connection physical; + private final boolean failGetConnection; + private final boolean nullGetConnection; + private final AtomicBoolean physicalClosed = new AtomicBoolean(); + private final AtomicBoolean logicalClosed = new AtomicBoolean(); + + Handle(Connection physical, boolean failGetConnection, + boolean nullGetConnection) { + this.physical = physical; + this.failGetConnection = failGetConnection; + this.nullGetConnection = nullGetConnection; + } + + @Override + public Connection getConnection() throws SQLException { + if (failGetConnection) { + throw new SQLException("simulated logical-connection failure"); + } + if (nullGetConnection) { + return null; + } + return (Connection) Proxy.newProxyInstance( + TestCpds.class.getClassLoader(), + new Class<?>[] { Connection.class }, + new LogicalConnection(physical, logicalClosed)); + } + + @Override + public void close() throws SQLException { + // Per the JDBC specification this closes the physical + // connection; the logical connection alone does not. + if (physicalClosed.compareAndSet(false, true)) { + physical.close(); + } + } + + @Override + public void addConnectionEventListener(javax.sql.ConnectionEventListener listener) { + // no-op + } + + @Override + public void removeConnectionEventListener(javax.sql.ConnectionEventListener listener) { + // no-op + } + + @Override + public void addStatementEventListener(javax.sql.StatementEventListener listener) { + // no-op + } + + @Override + public void removeStatementEventListener(javax.sql.StatementEventListener listener) { + // no-op + } + } + + private static final class LogicalConnection implements InvocationHandler { + + private final Connection physical; + private final AtomicBoolean logicalClosed; + + LogicalConnection(Connection physical, AtomicBoolean logicalClosed) { + this.physical = physical; + this.logicalClosed = logicalClosed; + } + + @Override + public Object invoke(Object proxy, Method method, Object[] args) + throws Throwable { + if ("close".equals(method.getName())) { + // Returning the logical connection to the provider must + // not close the physical connection. + logicalClosed.set(true); + return null; + } + if ("isClosed".equals(method.getName())) { + return Boolean.valueOf(logicalClosed.get()); + } + if (logicalClosed.get()) { + throw new SQLException("Logical connection already closed"); + } + try { + return method.invoke(physical, args); + } catch (InvocationTargetException e) { + throw e.getCause(); + } + } + } + } + + private DataSource newCpdsDataSource(TestCpds cpds) { + DataSource ds = newDataSource(); + ds.setDataSource(cpds); + return ds; + } + + @Test + void closingTheLogicalConnectionAloneDoesNotCloseThePhysicalOne() throws Exception { + // Pins the premise of the defect: a spec-faithful CPDS keeps the + // physical session open until the handle itself is closed. + TestCpds cpds = new TestCpds(getUrl()); + javax.sql.PooledConnection handle = cpds.getPooledConnection("sa", ""); + Connection logical = handle.getConnection(); + try (Statement st = logical.createStatement()) { + st.execute("SELECT 1"); + } + logical.close(); + Assertions.assertTrue(cpds.handles.get(0).logicalClosed.get()); + Assertions.assertFalse(cpds.handles.get(0).physicalClosed.get(), + "closing only the logical connection must not close the physical one"); + awaitSessions(1); + + handle.close(); + Assertions.assertTrue(cpds.handles.get(0).physicalClosed.get()); + awaitSessions(0); + } + + @Test + void releasingAConnectionClosesThePhysicalConnection() throws Exception { + TestCpds cpds = new TestCpds(getUrl()); + DataSource ds = newCpdsDataSource(cpds); + + try (Connection c = ds.getConnection()) { + try (Statement st = c.createStatement()) { + st.execute("SELECT 1"); + } + } + awaitSessions(1); + + // Returning the connection to the pool and then closing the pool + // releases the idle connection, which must close the physical + // session through the handle, not just the logical connection. + ds.close(); + + Assertions.assertEquals(1, cpds.handles.size()); + Assertions.assertTrue(cpds.handles.get(0).physicalClosed.get(), + "releasing the connection must close the physical connection " + + "through the PooledConnection handle"); + awaitSessions(0); + } + + @Test + void reconnectClosesThePreviousPhysicalConnection() throws Exception { + TestCpds cpds = new TestCpds(getUrl()); + DataSource ds = newCpdsDataSource(cpds); + // A different credential on the next borrow forces a reconnect of + // the idle connection through disconnect(false). + ds.getPoolProperties().setAlternateUsernameAllowed(true); + + try (Connection c = ds.getConnection()) { + setSessionMarker(c, 1); + } + awaitSessions(1); + + try (Connection c = ds.getConnection("otheruser", "otherpass")) { + // The reconnect took a new physical session... + Assertions.assertNull(sessionMarker(c), + "the reconnect must not reuse the old session"); + Assertions.assertEquals(2, cpds.handles.size()); + Assertions.assertTrue(cpds.handles.get(0).physicalClosed.get(), + "the reconnect must close the previous physical " + + "connection through its handle"); + Assertions.assertFalse(cpds.handles.get(1).physicalClosed.get()); + awaitSessions(1); + } + } + + @Test + void aFailedLogicalConnectionDoesNotStrandTheHandle() throws Exception { + TestCpds cpds = new TestCpds(getUrl()); + DataSource ds = newCpdsDataSource(cpds); + ds.getPoolProperties().setInitialSize(0); + + // The handle opens a real session and then refuses to hand out the + // logical connection; the session must not be stranded. + cpds.failNextGetConnection = true; + Assertions.assertThrows(SQLException.class, ds::getConnection, + "the simulated logical-connection failure must surface"); + + Assertions.assertEquals(1, cpds.handles.size()); + Assertions.assertTrue(cpds.handles.get(0).physicalClosed.get(), + "a handle whose logical connection failed must be closed, " + + "not stranded"); + awaitSessions(0); + } + + @Test + void aNullLogicalConnectionDoesNotStrandTheHandle() throws Exception { + TestCpds cpds = new TestCpds(getUrl()); + DataSource ds = newCpdsDataSource(cpds); + ds.getPoolProperties().setInitialSize(0); + + // A contract-violating handle that hands out a null connection for + // a real session must be closed rather than kept. + cpds.nullNextGetConnection = true; + Assertions.assertThrows(SQLException.class, ds::getConnection, + "a null logical connection must be reported as a failure"); + + Assertions.assertEquals(1, cpds.handles.size()); + Assertions.assertTrue(cpds.handles.get(0).physicalClosed.get(), + "a handle that handed out a null connection must be closed, " + + "not stranded"); + awaitSessions(0); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
