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 fc28c2748acfd35138b8bf7d35ae29292d0a816d Author: opencode <[email protected]> AuthorDate: Mon Oct 26 08:45:24 2026 +0100 Do not leak or resurrect a connection pool when closing a data source DataSourceProxy.close(boolean) was unsynchronized against pCreatePool() and carried no closed state, so a close racing the first getConnection could observe the not-yet-published pool and return without closing anything, orphaning a fully live pool (database sessions plus scheduled cleaner), and any post-close borrow or pool access silently built a fresh pool on a data source its owner had stopped tracking. Close and creation are now serialized on the data source monitor and closure is terminal: pCreatePool() fails with an SQLException once closed, getPool() reports the normal closed state as null without an error log, and close() waits for an in-flight creation and then tears the resulting pool down instead of racing it. --- .../apache/tomcat/jdbc/pool/DataSourceProxy.java | 56 ++++-- .../tomcat/jdbc/pool/TestCloseVsPoolCreation.java | 201 +++++++++++++++++++++ 2 files changed, 244 insertions(+), 13 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceProxy.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceProxy.java index e37aae2022..3e3eb44214 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceProxy.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceProxy.java @@ -56,6 +56,15 @@ public class DataSourceProxy implements PoolConfiguration { */ protected volatile PoolConfiguration poolProperties = null; + /** + * Terminal lifecycle flag. Set by {@link #close(boolean)} while holding + * this proxy's monitor, the same monitor that serializes pool creation in + * {@link #pCreatePool()}. Once set, no new {@link ConnectionPool} is + * created: {@link #createPool()} fails with a {@link SQLException} instead + * of resurrecting a pool that its owner has already stopped tracking. + */ + private volatile boolean closed = false; + /** * Construct a DataSourceProxy with default pool properties. */ @@ -144,7 +153,8 @@ public class DataSourceProxy implements PoolConfiguration { /** * Sets up the connection pool, by creating a pooling driver. * @return the connection pool - * @throws SQLException Error creating pool + * @throws SQLException Error creating pool, or if this data source has + * been closed */ public ConnectionPool createPool() throws SQLException { if (pool != null) { @@ -158,6 +168,9 @@ public class DataSourceProxy implements PoolConfiguration { * Sets up the connection pool, by creating a pooling driver. */ private synchronized ConnectionPool pCreatePool() throws SQLException { + if (closed) { + throw new SQLException("DataSource has been closed."); + } if (pool != null) { return pool; } else { @@ -260,12 +273,18 @@ public class DataSourceProxy implements PoolConfiguration { /** * Return the underlying connection pool, creating it if necessary. * - * @return the connection pool or null if creation failed + * @return the connection pool, or null if creation failed or this data + * source has been closed */ public ConnectionPool getPool() { try { return createPool(); }catch (SQLException x) { + if (closed) { + // A closed data source legitimately has no pool; this is + // a normal lifecycle state, not a creation failure. + return null; + } log.error("Error during connection pool creation.", x); return null; } @@ -274,26 +293,37 @@ public class DataSourceProxy implements PoolConfiguration { /** * Close the connection pool, returning connections to the database. + * The closure is terminal: later borrow or pool creation attempts on + * this data source fail with an <code>SQLException</code> instead of + * recreating the pool. */ public void close() { close(false); } /** - * Close the connection pool. + * Close the connection pool. If the pool is being created concurrently, + * this method waits for the creation to complete and then closes the + * resulting pool rather than racing it. * * @param all if true close all connections, otherwise only idle ones */ - public void close(boolean all) { - try { - if (pool != null) { - final ConnectionPool p = pool; - pool = null; - if (p!=null) { - p.close(all); - } + public synchronized void close(boolean all) { + // Synchronized on the same monitor as pCreatePool() so a pool being + // created concurrently with this close cannot be published after the + // field has been nulled and thus leak, untracked, with its database + // sessions and scheduled cleaner still alive. The closed flag makes + // the closure terminal for the pool creation paths as well: a closed + // data source no longer resurrects its pool on the next borrow or + // statistics access. + closed = true; + final ConnectionPool p = pool; + pool = null; + if (p != null) { + try { + p.close(all); + } catch (Exception x) { + log.warn("Error during connection pool closure.", x); } - }catch (Exception x) { - log.warn("Error during connection pool closure.", x); } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestCloseVsPoolCreation.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestCloseVsPoolCreation.java new file mode 100644 index 0000000000..4a542b240f --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestCloseVsPoolCreation.java @@ -0,0 +1,201 @@ +/* + * 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.sql.Connection; +import java.sql.Driver; +import java.sql.DriverPropertyInfo; +import java.sql.SQLException; +import java.sql.SQLFeatureNotSupportedException; +import java.util.Properties; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; +import java.util.logging.Logger; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; + +/** + * Closing a data source must be serialized against pool creation and must + * be terminal. Unsynchronized, a {@code close()} racing the first + * {@code getConnection()} observed {@code pool == null}, returned without + * closing anything, and the concurrently created pool was published + * afterwards - live, unclosed, with its database sessions and scheduled + * cleaner no longer reachable through the closed data source. Likewise, + * any later borrow or statistics access silently resurrected a fresh pool + * on a data source its owner had already stopped tracking. + */ +class TestCloseVsPoolCreation extends JdbcPoolBaseTest { + + /* A driver that parks inside connect() until the test releases it, + so pool creation can be held mid-flight while close() is invoked. + It signals when it enters connect(). */ + public static class ParkingConnectDriver implements Driver { + + static final CountDownLatch CONNECT_STARTED = new CountDownLatch(1); + static final CountDownLatch CONNECT_RELEASE = new CountDownLatch(1); + + @Override + public Connection connect(String url, Properties info) throws SQLException { + CONNECT_STARTED.countDown(); + try { + CONNECT_RELEASE.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new SQLException(e); + } + return new org.h2.Driver().connect(url, info); + } + + @Override + public boolean acceptsURL(String url) { + return url != null && url.startsWith("jdbc:h2:"); + } + + @Override + public DriverPropertyInfo[] getPropertyInfo(String url, Properties info) { + return new DriverPropertyInfo[0]; + } + + @Override + public int getMajorVersion() { + return 0; + } + + @Override + public int getMinorVersion() { + return 0; + } + + @Override + public boolean jdbcCompliant() { + return false; + } + + @Override + public Logger getParentLogger() throws SQLFeatureNotSupportedException { + throw new SQLFeatureNotSupportedException(); + } + } + + @Test + void closeRacingPoolCreationClosesThePoolInsteadOfLeakingIt() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setDriverClassName(ParkingConnectDriver.class.getName()); + // initialSize=1 makes the physical connect part of pool creation, + // which is the window the close must be serialized against + ds.getPoolProperties().setInitialSize(1); + + AtomicReference<Connection> borrowed = new AtomicReference<>(); + Thread creator = new Thread(() -> { + try { + borrowed.set(ds.getConnection()); + } catch (SQLException e) { + // Rejected by the pool-level closed guard - expected when + // the close wins the race; see TestCloseDuringBorrow + } + }, "creator racing close"); + creator.setDaemon(true); + creator.start(); + try { + assertTrue(ParkingConnectDriver.CONNECT_STARTED.await(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS), + "pool creation must be parked inside the driver's connect()"); + + // close() while the pool is still being created: it must not + // observe the not-yet-published pool as "nothing to close" and + // return - it has to wait for the creation and then tear it + // down. (Whether the close also wins against a merely + // unscheduled creator is not what this asserts; the session + // and resurrection assertions below carry the regression.) + AtomicReference<Throwable> closeFailure = new AtomicReference<>(); + Thread closer = new Thread(() -> { + try { + ds.close(true); + } catch (Throwable e) { + closeFailure.set(e); + } + }, "close racing creation"); + closer.setDaemon(true); + closer.start(); + + closer.join(1000); + assertTrue(closer.isAlive(), + "close() must wait for the in-flight pool creation instead of" + + " returning while the pool is still unpublished"); + + ParkingConnectDriver.CONNECT_RELEASE.countDown(); + closer.join(AWAIT_TIMEOUT_MS); + creator.join(AWAIT_TIMEOUT_MS); + assertFalse(creator.isAlive(), "the creating thread must not outlive the test"); + assertNull(closeFailure.get(), "close() must not fail"); + + if (borrowed.get() != null) { + // The borrow won the race against the closure by a hair; the + // connection must be releasable so the closed pool drains it. + borrowed.get().close(); + } + // With the leak, this pool would still own its session: the close + // ran against a null field and the published pool was never closed. + // (Whether the racing borrow itself failed or merely got its + // connection torn down is not deterministic here; TestCloseDuring- + // Borrow pins that behavior down.) + awaitSessions(0); + + // The closure is terminal: no later access may resurrect a pool + SQLException x = assertThrows(SQLException.class, ds::getConnection, + "a closed data source must reject borrows instead of recreating the pool"); + assertTrue(x.getMessage() != null && x.getMessage().contains("closed"), + "the rejection must say the data source is closed, was: " + x.getMessage()); + assertNull(ds.getPool(), "getPool() must not create a pool on a closed data source"); + assertEquals(0, poolSessions(), "no database session may exist after the close"); + } finally { + // Never leave the creator parked inside connect(): it holds + // the data source monitor there, so the base class teardown + // (ds.close()) would block forever and hang the suite. + ParkingConnectDriver.CONNECT_RELEASE.countDown(); + creator.join(AWAIT_TIMEOUT_MS); + } + } + + @Test + void closedDataSourceDoesNotResurrectItsPool() throws Exception { + DataSource ds = newDataSource(); + ds.getConnection().close(); + assertEquals(1, poolSessions(), "the pool must own its connection before the close"); + ds.close(); + awaitSessions(0); + + SQLException x = assertThrows(SQLException.class, ds::getConnection, + "a closed data source must reject borrows instead of recreating the pool"); + assertTrue(x.getMessage() != null && x.getMessage().contains("closed"), + "the rejection must say the data source is closed, was: " + x.getMessage()); + assertEquals(0, poolSessions(), "no new pool may be created after close()"); + + // A failing pool creation surfaces through getPool() as null... + assertNull(ds.getPool()); + // ...while the explicit creation attempt reports the reason + assertNotNull(assertThrows(SQLException.class, ds::createPool)); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
