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 9ae5816b00bf69eb0ded2fa6aae01a99e6392ebb Author: opencode <[email protected]> AuthorDate: Thu Oct 29 23:01:40 2026 +0100 Do not wait on pool creation when closing a JDBC pool data source The ConnectionPool constructor pre-establishes the initialSize connections and previously ran under the data-source monitor that close() also takes, so closing a data source whose initial connections were still being attempted against an unresponsive database parked the shutdown on the driver connect for an unbounded, uninterruptible time. Pool construction now runs under a dedicated gate that serializes it outside the monitor, and publication is a single check-and-publish step under the monitor: a creation that races closure either wins and has its pool closed by the close, or observes the closure at publication and tears the pool it built down as rejected. As before, no database session and no pool cleaner outlives the close and the closure stays terminal, but close() no longer waits on the driver. TestCloseVsPoolCreation now asserts that close completes while the creator is still parked in connect(). --- .../org/apache/tomcat/jdbc/pool/DataSource.java | 14 +-- .../apache/tomcat/jdbc/pool/DataSourceProxy.java | 109 ++++++++++++++++----- .../tomcat/jdbc/pool/TestCloseVsPoolCreation.java | 68 ++++++++----- webapps/docs/changelog.xml | 14 +++ 4 files changed, 148 insertions(+), 57 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSource.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSource.java index 10e44db112..76edea778d 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSource.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSource.java @@ -63,13 +63,13 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, public synchronized void close(boolean all) { // This monitor serializes the whole registration lifecycle against the // closure: preRegister(), postRegister() and postDeregister() take it - // too, and DataSourceProxy.close(boolean) and pCreatePool() below run - // under the same lock. A close therefore cannot slip in beside an - // in-flight publication - between its read of the registration name - // and the publish itself - where its own unregister would find nothing - // to take back and the pool MBean would then be published onto the - // dead pool under a live name that no later close() or - // postDeregister() could ever take back. + // too, and DataSourceProxy.close(boolean) and the publication step of + // pCreatePool() below run under the same lock. A close therefore + // cannot slip in beside an in-flight publication - between its read of + // the registration name and the publish itself - where its own + // unregister would find nothing to take back and the pool MBean would + // then be published onto the dead pool under a live name that no later + // close() or postDeregister() could ever take back. releasePoolMBean(); super.close(all); } 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 630824d953..82cddbb5ec 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 @@ -55,13 +55,28 @@ 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. + * Terminal lifecycle flag. Set by {@link #close(boolean)} while holding this proxy's monitor, the same monitor + * under which {@link #pCreatePool()} checks the flag and publishes its result. Once set, no {@link ConnectionPool} + * is published any more: {@link #createPool()} fails with a {@link SQLException} instead of resurrecting a pool + * that its owner has already stopped tracking, and a creation that was already in flight - whose constructor the + * close did not wait for - closes the pool it built instead of publishing it. */ private volatile boolean closed = false; + /** + * Serializes pool construction away from this proxy's monitor. The pool constructor pre-connects + * {@link PoolConfiguration#getInitialSize() initialSize} connections, so against a hung database it blocks in the + * driver for as long as the JDBC login timeout; running that under the monitor {@link #close(boolean)} takes made + * the close wait on the driver for an unbounded, uninterruptible time. The gate keeps the construction serialized + * anyway - it is the only point at which two pools are ever built from the same {@code poolProperties}, whose + * contract is that no two pools are ever constructed from one instance at a time - while leaving the close free, + * as it only has to serialize against the short publication step, not the driver. This lock is taken before the + * proxy monitor and never while holding it, so it cannot invert against the monitor's other users: + * {@link #close(boolean)} and, in the {@link DataSource} subclass, the JMX registration callbacks; none of them + * reaches pool creation. + */ + private final Object poolCreationLock = new Object(); + /** * Construct a DataSourceProxy with default pool properties. */ @@ -205,20 +220,60 @@ public class DataSourceProxy implements PoolConfiguration { /** * Sets up the connection pool, by creating a pooling driver. */ - private synchronized ConnectionPool pCreatePool() throws SQLException { - // Runs under the monitor that close(boolean) also takes, and that is - // the only writer that nulls the field, so the pool cannot disappear - // between the reads below the way it can on the unsynchronized read - // paths. Returning null is therefore not possible here: either the - // closed flag rejects the caller or a pool is published and returned. + private ConnectionPool pCreatePool() throws SQLException { + // Runs under the construction gate, not under the monitor that + // close(boolean) takes: the constructor pre-connects the initial + // size of the pool, so against a hung database it blocks inside the + // driver's connect for as long as the JDBC login timeout, and a + // close parked behind that would be unbounded and uninterruptible. + // Returning null is not possible here: either the closed flag + // rejects the caller or a pool is published and returned. if (closed) { throw new SQLException("DataSource has been closed."); } - if (pool != null) { - return pool; - } else { - pool = new ConnectionPool(poolProperties); - return pool; + synchronized (poolCreationLock) { + // Re-check under the gate: a close completed while this thread + // queued must not be resurrected, and a creator that won the + // gate has already published its pool. The gate also keeps this + // the only construction the shared poolProperties ever faces. + if (closed) { + throw new SQLException("DataSource has been closed."); + } + final ConnectionPool existing = pool; + if (existing != null) { + return existing; + } + // The constructor - and with it the driver connects of the + // initial pre-load - runs with the close monitor free. A failure + // here propagates directly: an init that fails after starting + // anything tears its pool down itself before throwing. + final ConnectionPool fresh = new ConnectionPool(poolProperties); + final boolean rejected; + synchronized (this) { + // Check-and-publish against the closure, which sets the flag + // and takes the field in one step under this same monitor: + // whichever of the two runs first wins, so the close either + // finds this pool published and closes it, or it found the + // field empty and this thread takes the pool down below. It + // is never neither - that was the leak - and never both. + rejected = closed; + if (!rejected) { + pool = fresh; + } + } + if (rejected) { + // close(boolean) completed while this pool was being built + // and had nothing published to close, so the sessions the + // pre-load established and the cleaner the constructor + // started are this thread's to release. + try { + fresh.close(true); + } catch (Exception x) { + log.warn("Error during connection pool closure.", x); + } + throw new SQLException("DataSource has been closed."); + } + return fresh; } } @@ -349,19 +404,25 @@ public class DataSourceProxy implements PoolConfiguration { } /** - * 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. + * Close the connection pool. If a pool creation is in flight, this method does not wait for it - the driver + * connects of the creation's initial pre-load can block for an unbounded, uninterruptible time against a hung + * database - it marks this data source closed and returns: the creation observes the closure at its publication + * step and closes the pool it built instead of publishing it. * * @param all if true close all connections, otherwise only idle ones */ 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. + // Sets the terminal closed flag and takes the published pool in one + // step under this monitor, which the publication step of + // pCreatePool() also takes as one step: a creation whose pool is not + // yet published cannot be missed by this close and then published + // after it - it either wins the monitor and this close closes its + // pool, or this close wins and that creation tears its pool down as + // rejected. 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. The monitor is + // never held across a pool construction, so this method never waits + // on a driver connect. closed = true; final ConnectionPool p = pool; pool = null; 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 index d48d9ff9db..34da3afe59 100644 --- 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 @@ -33,14 +33,17 @@ 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 + * Closing a data source must be serialized against the publication of its + * pool 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. + * 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. The construction + * itself runs outside the monitor the close takes, so the close is never + * parked behind a driver connect: a creation already in flight observes the + * closure at its publication step and tears its result down itself. */ class TestCloseVsPoolCreation extends JdbcPoolBaseTest { @@ -104,12 +107,12 @@ class TestCloseVsPoolCreation extends JdbcPoolBaseTest { ds.getPoolProperties().setInitialSize(1); AtomicReference<Connection> borrowed = new AtomicReference<>(); + AtomicReference<SQLException> borrowFailure = 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 + borrowFailure.set(e); } }, "creator racing close"); creator.setDaemon(true); @@ -119,12 +122,13 @@ class TestCloseVsPoolCreation extends JdbcPoolBaseTest { .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.) + // close() while the pool is still being created: the close must + // not be parked behind the driver's connect() the creation is + // blocked in - the construction runs outside the monitor the + // close takes - and the still-unpublished pool must not be lost + // either: whichever of the two reaches the monitor first owns + // the teardown, so the pool the parked creation later finishes + // is closed by that creation itself when it finds the closure. AtomicReference<Throwable> closeFailure = new AtomicReference<>(); Thread closer = new Thread(() -> { try { @@ -136,22 +140,32 @@ class TestCloseVsPoolCreation extends JdbcPoolBaseTest { closer.setDaemon(true); closer.start(); - closer.join(500); - Assertions.assertTrue(closer.isAlive(), - "close() must wait for the in-flight pool creation instead of" - + " returning while the pool is still unpublished"); + closer.join(AWAIT_TIMEOUT_MS); + Assertions.assertFalse(closer.isAlive(), + "close() must not block behind the in-flight pool creation:" + + " parking it on the driver's connect() against a hung" + + " database would park the shutdown forever"); + Assertions.assertNull(closeFailure.get(), "close() must not fail"); ParkingConnectDriver.CONNECT_RELEASE.countDown(); - closer.join(AWAIT_TIMEOUT_MS); creator.join(AWAIT_TIMEOUT_MS); Assertions.assertFalse(creator.isAlive(), "the creating thread must not outlive the test"); - Assertions.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. + // The borrow published before the closure and won the race + // by a hair; the connection must be releasable so the closed + // pool drains it. borrowed.get().close(); + } else { + // The closure won: the creation must have been rejected, and + // told so as a closed data source, not left to hang. + Assertions.assertNotNull(borrowFailure.get(), + "the losing creation must have been rejected"); + Assertions.assertTrue(borrowFailure.get().getMessage() != null + && borrowFailure.get().getMessage().contains("closed"), + "the rejection must say the data source is closed, was: " + + borrowFailure.get().getMessage()); } // With the leak, this pool would still own its session: the close // ran against a null field and the published pool was never closed. @@ -170,9 +184,11 @@ class TestCloseVsPoolCreation extends JdbcPoolBaseTest { Assertions.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. + // Release and reap the parked creation even on a red run. It no + // longer holds the data source monitor inside connect(), so the + // base class teardown (ds.close()) cannot be blocked by it, but + // it must not linger past the test still owning a half-built + // pool whose connect() then completes and publishes nothing. ParkingConnectDriver.CONNECT_RELEASE.countDown(); creator.join(AWAIT_TIMEOUT_MS); } diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index d5afcccae8..f65edca899 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -1101,6 +1101,20 @@ value for that attribute is still rejected, with a message that now points at the supported ways of providing a data source. (remm) </fix> + <fix> + Do not wait on pool creation when closing a JDBC pool data source. + The pool constructor pre-establishes the <code>initialSize</code> + connections, and it previously ran under the data source monitor that + <code>close()</code> takes, so closing a data source whose initial + connections were still being attempted against an unresponsive + database parked the shutdown on the driver for an unbounded, + uninterruptible time. The construction now runs under a gate that + serializes it on its own, outside that monitor, so a close does not + wait for an in-flight creation; that creation observes the closure at its + publication step and tears down the pool it built rather than + publishing it. As before, no database session and no pool cleaner + outlives the close, and the closure stays terminal. (remm) + </fix> </changelog> </subsection> <subsection name="Other"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
