This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 11.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 2e7deeb435cf234a31a459e33296f627b1bdc295 Author: opencode <[email protected]> AuthorDate: Mon Nov 2 15:50:36 2026 +0100 Harden the data source's JMX registration lifecycle and its closure Review round on org.apache.tomcat.jdbc.pool.DataSource, five findings, one consolidated state: Closure. DataSource.close(boolean) holds the data source monitor only for releasing the pool MBean and the flag-and-take of the pool; the physical disconnect of every pooled session - an unbounded, uninterruptible wait against a hung database - runs outside it, where it can no longer park later borrows' publication step or the container's registration callbacks. Pinned by TestCloseVsMonitorOccupancy. Registration handle. The handle slot now names only a publish this data source completed and still holds: it is written after the server accepts a publish, and no refusal, skip or second registration clears or replaces it. The MBean server runs preRegister() before its own duplicate check and permits a second registration of one object under another name, so previously a registration tool run twice nulled the handle against the live first publish, or a second registration succeeded a second publish and overwrote it - either way orphaning a pool MBean that no close or deregistration could ever take back, pinning the pool, its credentials and its driver class loader. A registration that finds a handle held now publishes nothing and warns (dataSource.mbeanAlreadyRegistered); the in-flight publish target travels in pendingName so it cannot disturb the held handle. Pinned by TestDataSourceReregistration. Failed-registration cleanup. postRegister(false) releases only a publish that same registration made; a failed duplicate registration no longer tears down the pool MBean of the live registration that holds the handle. Failing release. The unregistration inside releasePoolMBean() is contained (logged, dataSource.unableToUnregisterMBean, never propagated): the handle is taken first, so a throwing unregisterJmx subclass or a surfaced Error can cost the registered MBean but can no longer abort the close between taking the handle and setting the terminal flag - a close that could not unregister did not close at all, while still giving up the handle. Pinned by TestCloseVsFailingUnregistration. Documentation. jdbc-pool.xml: the registration propagates to the pool MBean once, at registration time (a pool created later stays unmonitored; create the pool first or re-register), a directly registered pool MBean is not taken back by close(), and a new 'Closing the data source' subsection states the terminal-close contract and the close(false) semantics. close(boolean) javadoc reworded to match; comments that restated adjacent javadoc trimmed. No changelog entries per review decision. Suite: 139 classes, 704 tests, green. --- modules/jdbc-pool/doc/jdbc-pool.xml | 30 ++ .../org/apache/tomcat/jdbc/pool/DataSource.java | 173 ++++++++--- .../apache/tomcat/jdbc/pool/DataSourceProxy.java | 54 +++- .../tomcat/jdbc/pool/util/LocalStrings.properties | 2 + .../pool/TestCloseVsFailingUnregistration.java | 170 +++++++++++ .../jdbc/pool/TestCloseVsMonitorOccupancy.java | 319 +++++++++++++++++++++ .../pool/jmx/TestDataSourceReregistration.java | 251 ++++++++++++++++ 7 files changed, 937 insertions(+), 62 deletions(-) diff --git a/modules/jdbc-pool/doc/jdbc-pool.xml b/modules/jdbc-pool/doc/jdbc-pool.xml index 6670aadd46..307c478d5e 100644 --- a/modules/jdbc-pool/doc/jdbc-pool.xml +++ b/modules/jdbc-pool/doc/jdbc-pool.xml @@ -154,6 +154,36 @@ and it propagates the registration to the underlying pool. To do this you would call <code>mBeanServer.registerMBean(dataSource.getPool().getJmxPool(),objectname)</code>. Prior to this call, ensure that the pool has been created by calling <code>dataSource.createPool()</code>. </p> + <p> + The propagation of the registration to the connection pool MBean happens once, + at the moment the data source is registered. If the pool has not been created + at that point, no connection pool MBean is published, nothing is published + later when the pool is created, and the registration is not retried; a warning + is logged at registration time. Ensure the pool exists before the data source + is registered - in a container the resource factory creates it up front - or + deregister and register the data source again afterwards. + </p> + <p> + Only a registration propagated through the data source is taken back when the + data source is closed or deregistered. A connection pool MBean registered + directly, as in the call above, is not unregistered by <code>close()</code>; + unregister it yourself when you close the data source. (Inside Tomcat the + container registers the data source through its own wrapper, and the pool + MBean's lifecycle is managed for you.) + </p> + </subsection> + <subsection name="Closing the data source"> + <p> + Calling <code>close()</code> or <code>close(false)</code> on the data source + closes the connection pool and is terminal: later calls to + <code>getConnection()</code> or <code>createPool()</code> fail with a + <code>SQLException</code> instead of silently recreating the pool. Connections + that are currently borrowed are not disconnected by <code>close(false)</code>; + they keep their database sessions and stay usable until their holders close + them. <code>close(true)</code> additionally disconnects the database sessions + of the connections that are still in use. JMX monitoring of the pool ends when + <code>close()</code> returns, including for connections that are still in use. + </p> </subsection> </section> 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 a7f2a420bc..6950f51695 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 @@ -55,35 +55,71 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, } /** - * Unregisters the connection pool MBean before closing the pool, so a dead pool does not keep reporting stale - * statistics under a live object name (and does not pin the closed pool, its credentials and its JDBC driver class - * loader in the MBean server).<br> + * Unregisters the connection pool MBean before closing the pool, so a pool under teardown does not keep reporting + * statistics under a live object name (and does not pin the closing pool, its credentials and its JDBC driver class + * loader in the MBean server). Monitoring stops with this call, whichever way the close goes: with + * <code>all=false</code> a connection that is currently borrowed keeps its database session and stays usable + * until its holder returns it, but it is no longer reported by the pool MBean.<br> * {@inheritDoc} */ @Override - public synchronized void close(boolean all) { - // This monitor serializes the whole registration lifecycle against the + public void close(boolean all) { + // This monitor serializes the registration lifecycle against the // closure: preRegister(), postRegister() and postDeregister() take it - // 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); + // too, and the flag-and-take step of DataSourceProxy.close(boolean) + // and the publication step of pCreatePool() run under the same lock - + // why a close therefore cannot slip in beside an in-flight publication + // is told at preRegister(). + // The monitor is held for these steps only: the pool teardown below + // disconnects every pooled session, an unbounded and uninterruptible + // wait against a hung database, and a parked teardown must not park + // the later borrows' publication step or the container's callbacks - + // the same reason pool construction already runs outside it. A close + // concurrent with this one finds the pool already taken and returns + // instead of waiting for the in-flight teardown. + final ConnectionPool p; + synchronized (this) { + releasePoolMBean(); + p = takePoolForClose(); + } + closePool(p, all); } - // The JMX operations below register the pool itself under the // tomcat.jdbc domain. /** * The JMX ObjectName for the connection pool MBean. The handle is checked, - * cleared and released only under this data source's monitor. + * cleared and released only under this data source's monitor. It is written + * only by a publish that completed - it always names an MBean this data + * source registered and still holds - and cleared only when that MBean is + * unregistered, so no failure or skip of a (re-)registration can drop or + * replace the handle the close and the deregistration callbacks release. */ protected volatile ObjectName oname = null; + /** + * Whether the publish that the handle names was made by the registration + * currently in flight - the {@link #preRegister(MBeanServer, ObjectName)} + * whose {@link #postRegister(Boolean)} completion is still pending. Set by + * {@link #registerJmx()} when a registration-driven publish is accepted, + * and cleared with the handle and when a registration completes. The + * server reports a failed registration to {@link #postRegister(Boolean)} + * without saying what that registration published; a registration that + * published nothing - because an earlier registration's handle was held - + * has nothing of its own to undo, and this is how that is known. + */ + private volatile boolean publishedByRegistration = false; + + /** + * The name derived from the registration currently in flight, set and + * cleared by {@link #preRegister(MBeanServer, ObjectName)} around its call + * into {@link #registerJmx()}, under this data source's monitor. A second + * registration must read its publish target from here rather than from the + * handle slot, so it can never overwrite the handle of a publish that is + * already held. + */ + private volatile ObjectName pendingName = null; + /** * Unregisters the underlying connection pool mbean.<br> * {@inheritDoc} @@ -104,24 +140,24 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, * {@code registrationDone} means that the registration phase failed, and after a failed registration the server * does not call {@link #postDeregister()}: this callback is the only chance to undo that side effect. Leaving the * pool MBean registered would keep a pool nobody manages reporting statistics and notifications, and would pin the - * pool - with its properties, its credentials and its JDBC driver class loader - in the MBean server.<br> + * pool - with its properties, its credentials and its JDBC driver class loader - in the MBean server. Only a + * publish this registration made is undone: a registration that published nothing - because an earlier + * registration still held the handle, or because it derived no name at all - releases nothing, since the handle it + * finds in place belongs to that earlier registration and is that registration's to give back.<br> * {@inheritDoc} */ @Override public synchronized void postRegister(Boolean registrationDone) { - // The same check-and-clear-unregister of the registration handle as - // postDeregister() and close(), under the same monitor: a handle this - // bean still holds at a failed registration is exactly the publish - // preRegister() made, since registerJmx() keeps a name only for an - // accepted publish. Clearing it here also makes a later close() or - // postDeregister() skip the unregister, so the cleanup is never - // performed twice. - if (!Boolean.TRUE.equals(registrationDone)) { + if (Boolean.TRUE.equals(registrationDone)) { + publishedByRegistration = false; + return; + } + if (publishedByRegistration) { releasePoolMBean(); } + publishedByRegistration = false; } - /** * no-op<br> * {@inheritDoc} @@ -132,7 +168,9 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, } /** - * If the connection pool MBean exists, it will be registered during this operation.<br> + * If the connection pool MBean exists, it will be registered during this operation. A pool MBean this data source + * already published under an earlier registration is kept, and this registration then publishes nothing (see + * {@link #registerJmx()}).<br> * {@inheritDoc} */ @Override @@ -145,21 +183,32 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, // under a live name that no later close() or postDeregister() - which // only release a held handle - could ever take back. try { - if (isJmxEnabled()) { - this.oname = createObjectName(name); - if (oname != null) { - registerJmx(); + if (isJmxEnabled() && oname == null) { + final ObjectName derived = createObjectName(name); + if (derived != null) { + try { + pendingName = derived; + registerJmx(); + } finally { + pendingName = null; + } } + } else if (isJmxEnabled() && oname != null) { + log.warn(MessageFactory.get("dataSource.mbeanAlreadyRegistered", oname, name)); } } catch (MalformedObjectNameException x) { - this.oname = null; + // Nothing has been published and the handle slot was not touched: + // there is nothing here to take back. log.error(MessageFactory.get("dataSource.unableToCreateObjectName"), x); } catch (RuntimeException x) { // Never break the caller's MBean registration because of // pool-side state (for instance a pool that has not been // created yet); preRegister throwing aborts the whole - // registration in the MBean server. - this.oname = null; + // registration in the MBean server. The handle slot is left as + // registerJmx() last wrote it: a RuntimeException after a + // completed publish - from a later step than the publish itself - + // must not lose the handle of a live MBean, whose release then + // belongs to the close and the deregistration callbacks as ever. log.error(MessageFactory.get("dataSource.unableToRegisterMBean"), x); } return name; @@ -194,7 +243,9 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, * not been created yet (or was already closed) has no MBean to publish; the registration is then skipped rather * than failing, since the caller of {@link #preRegister(MBeanServer, ObjectName)} must not see an exception over * pool-side lifecycle state. A publish the MBean server refused - because the name is already in use by another - * MBean, for instance - is likewise not this pool's registration. + * MBean, for instance - is likewise not this pool's registration. In neither case is a handle this data source + * already holding disturbed: it names a publish that is still in effect, and dropping it here would leave that + * MBean held by nothing - neither a later close() nor a deregistration could ever take it back. */ protected synchronized void registerJmx() { // Runs under this data source's monitor - from preRegister() or a @@ -207,19 +258,31 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, // attempt against that null would only fail inside the helper, with a // spurious error log final org.apache.tomcat.jdbc.pool.jmx.ConnectionPool jmx = p == null ? null : p.getJmxPool(); + // The name to publish: the one the registration in flight derived, or, + // for a direct call outside a registration, the handle this data + // source holds. A direct call that still holds a live publish then + // aims at the name that publish took, which the server refuses as a + // duplicate - and the handle survives the refusal, as it must. + final ObjectName name = pendingName == null ? oname : pendingName; if (jmx == null) { - log.warn(MessageFactory.get("dataSource.mbeanNotRegistered", oname)); - // oname is the handle postDeregister() unregisters; do not keep - // one that was never registered - this.oname = null; + // Nothing to publish; the handle slot, which this call did not + // write, is left as it stands. + log.warn(MessageFactory.get("dataSource.mbeanNotRegistered", name)); return; } - // oname is only kept for an actually published MBean: the helper - // hands back the name of its own publish, or null when the server - // refused it, so a name a foreign MBean owns is never released as - // if it were this pool's registration - this.oname = JmxUtil.registerJmx(oname, null, jmx); - if (this.oname != null) { + final ObjectName published = JmxUtil.registerJmx(name, null, jmx); + if (published != null) { + // Recorded before the connection sweep below: a publish the server + // accepted is held from the moment it happened, whatever a later + // step makes of it. + this.oname = published; + // Only a registration-driven publish - one made from + // preRegister(), which sets pendingName around its call - has a + // registration that can fail and undo it in postRegister(false); + // a publish from a direct call belongs to no registration, and + // releasing it on a later registration failure would tear down a + // publication that registration never made. + publishedByRegistration = pendingName != null; // A connection publishes itself below this name when its physical // connection is established; the connections the pool was already // holding could not do that while this name did not exist yet, so @@ -250,13 +313,29 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, * here - {@link #close(boolean)}, the {@link #postRegister(Boolean)} of a * failed registration and the container's {@link #postDeregister()} - so * of all of them the release is performed at most once: whichever runs - * second finds the handle gone and unregisters nothing. + * second finds the handle gone and unregisters nothing. The unregistration + * is then best-effort and cannot break the caller: the handle is already + * taken, so a failure of the unregistration - a subclass that throws from + * {@link #unregisterJmx(ObjectName)}, or an {@code Error} surfacing + * through it - may cost a registered MBean, but the close that asked for + * this release still closes, and no later path re-attempts a name this + * data source has given up. */ private void releasePoolMBean() { final ObjectName name = oname; oname = null; + publishedByRegistration = false; if (name != null) { - unregisterJmx(name); + try { + unregisterJmx(name); + } catch (Throwable t) { + // Logged, never propagated: an escaping exception here would + // abort the caller between taking the handle and setting the + // terminal flag, leaving a pool still running under a caller + // that believes it closed - with the handle already gone, so + // no later path could ever take the MBean back either. + log.error(MessageFactory.get("dataSource.unableToUnregisterMBean", name), t); + } } } } 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 23b8aa713b..96df1c2bf4 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 @@ -419,27 +419,51 @@ public class DataSourceProxy implements PoolConfiguration { * 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. + * step and closes the pool it built instead of publishing it. The same reason keeps the teardown outside the + * monitor: the physical disconnect of every pooled session is as unbounded against a hung database as the + * connect is, and it must not park the paths that need the monitor meanwhile - a later borrow's publication step, + * or the container's JMX lifecycle callbacks. * * @param all if true close all connections, otherwise only idle ones */ - public synchronized void close(boolean all) { - // Sets the terminal closed flag and takes the published pool in one - // step under this monitor - the check-and-publish race this closes - // with is told at pCreatePool()'s publication step. 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. + public void close(boolean all) { + closePool(takePoolForClose(), all); + } + + /** + * Sets the terminal closed flag and takes the published pool, as one step under this data source's monitor. + * The check-and-publish race this closes with is told at + * {@code pCreatePool()}'s publication step, which takes the same monitor: whichever of the + * two runs first wins. 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. + * + * @return the pool that must now be torn down, or {@code null} when nothing was published (yet) + */ + protected synchronized ConnectionPool takePoolForClose() { closed = true; final ConnectionPool p = pool; pool = null; - if (p != null) { - try { - p.close(all); - } catch (Exception x) { - log.warn(MessageFactory.get("dataSourceProxy.errorClosingPool"), x); - } + return p; + } + + /** + * Tears down the pool that {@link #takePoolForClose()} handed over, outside this data source's monitor: a + * physical disconnect against a hung database is unbounded and uninterruptible, and a parked teardown must not + * park the later borrows or lifecycle callbacks that need that monitor. A failure of the teardown is logged, + * not propagated: the closure already happened when the flag was set, and one failing session must not stop + * the rest from being disconnected or hide the closure from the caller. + * + * @param p the pool to close, or {@code null} to close nothing + * @param all if true close all connections, otherwise only idle ones + */ + protected void closePool(ConnectionPool p, boolean all) { + if (p == null) { + return; + } + try { + p.close(all); + } catch (Exception x) { + log.warn(MessageFactory.get("dataSourceProxy.errorClosingPool"), x); } } 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 a86770bb56..8e937701fc 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 @@ -80,9 +80,11 @@ connectionState.unableToResetTransactionIsolation=Unable to reset transaction is connectionState.resetStateToNull=Reset state to null as an exception occurred while calling method[{0}]. connectionState.unableToTerminatePendingTransaction=Unable to check for, or terminate, the pending transaction before restoring the configured connection state. +dataSource.mbeanAlreadyRegistered=JDBC pool MBean is already published under [{0}]; the registration under [{1}] publishes nothing of the pool and takes over nothing. dataSource.mbeanNotRegistered=JDBC pool MBean [{0}] not registered: the pool has not been created yet or has already been closed. dataSource.unableToCreateObjectName=Unable to create object name for JDBC pool. dataSource.unableToRegisterMBean=Unable to register JDBC pool MBean. +dataSource.unableToUnregisterMBean=Unable to unregister JDBC pool MBean [{0}]. The registration handle was already given up, so the close or callback that asked for this release continues. dataSourceFactory.cannotSetDataSourceString=Can''t set dataSource property as a string "{0}". Either carry the javax.sql.DataSource object itself under the dataSource attribute of the Reference, or use the dataSourceJNDI attribute to name a data source bound in JNDI. dataSourceFactory.invalidBoolean=Invalid boolean value ''{0}'' for property {1} diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestCloseVsFailingUnregistration.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestCloseVsFailingUnregistration.java new file mode 100644 index 0000000000..3988c0ba23 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestCloseVsFailingUnregistration.java @@ -0,0 +1,170 @@ +/* + * 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.management.ManagementFactory; +import java.sql.SQLException; + +import javax.management.MBeanServer; +import javax.management.ObjectName; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; + +/** + * The release of the pool MBean runs inside the same critical section as the + * close's flag-and-take of the pool, and inside the deregistration callback. + * The unregistration is best-effort: the handle is taken from its slot before + * the unregistration is attempted, so a failure of the unregistration - a + * subclass of {@code unregisterJmx} that throws, or an {@code Error} surfacing + * through it - may cost a registered MBean, but must never break the path + * that called it. Before this fix it did: the exception escaped the close + * before the terminal flag was set and the pool was taken, so the caller's + * close silently had not happened - the pool kept handing out connections + * after being closed - while the handle was already gone, orphaning the very + * MBean the failed release had failed to unregister. + */ +class TestCloseVsFailingUnregistration extends JdbcPoolBaseTest { + + /* A data source whose next unregistration attempt throws once. */ + private static final class UnreliableDataSource extends DataSource { + private volatile boolean failNext = false; + private volatile int releaseAttempts = 0; + + ObjectName heldHandle() { + return oname; + } + + void failNextRelease() { + failNext = true; + } + + int releaseAttempts() { + return releaseAttempts; + } + + @Override + protected void unregisterJmx(ObjectName name) { + releaseAttempts++; + if (failNext) { + failNext = false; + throw new IllegalStateException("simulated unregistration failure"); + } + super.unregisterJmx(name); + } + } + + private static MBeanServer server() { + return ManagementFactory.getPlatformMBeanServer(); + } + + private static ObjectName uniqueName(String role) throws Exception { + return new ObjectName("tomcat:type=FailingUnregistrationTest,role=" + role + + ",uid=" + System.nanoTime()); + } + + /* Test hygiene: take away anything a broken build left published for this + registration - the uid is carried down into every name derived from it. */ + private static void sweep(MBeanServer server, ObjectName uidBearer) throws Exception { + String uid = uidBearer.getKeyProperty("uid"); + for (ObjectName name : server.queryNames(new ObjectName("tomcat.jdbc:*"), null)) { + if (uid.equals(name.getKeyProperty("uid"))) { + server.unregisterMBean(name); + } + } + } + + private UnreliableDataSource newUnreliableDataSource() { + UnreliableDataSource ds = new UnreliableDataSource(); + ds.setUrl(getUrl()); + ds.setDriverClassName("org.h2.Driver"); + ds.setUsername("sa"); + ds.setPassword(""); + ds.getPoolProperties().setJmxEnabled(true); + ds.getPoolProperties().setInitialSize(0); + ds.getPoolProperties().setMinIdle(0); + return manage(ds); + } + + @Test + void theCloseSurvivesAFailingMBeanReleaseAndStillClosesThePool() throws Exception { + MBeanServer server = server(); + UnreliableDataSource ds = newUnreliableDataSource(); + ds.createPool(); + ObjectName dsName = uniqueName("close"); + ObjectName poolOname = ds.createObjectName(dsName); + try { + ds.preRegister(server, dsName); + Assertions.assertTrue(server.isRegistered(poolOname), + "the registration must have published the pool MBean"); + + ds.failNextRelease(); + // The close must ride over the failing release: before the fix the + // exception escaped, the terminal flag was never set and the pool + // was never taken, so this call closed nothing. + ds.close(true); + Assertions.assertNull(ds.heldHandle(), + "the failed release must still have taken the handle - it is " + + "what the release paths check, and a kept handle " + + "would invite a second release of a name this data " + + "source may no longer own"); + Assertions.assertThrows(SQLException.class, ds::getConnection, + "a close whose MBean release failed must still have closed " + + "the pool: a caller that asked for a close and " + + "still gets connections holds a pool nobody can " + + "close any more"); + } finally { + ds.close(); + sweep(server, dsName); + } + } + + @Test + void aFailingReleaseRunsOnceAndLaterPathsDoNotTryAgain() throws Exception { + MBeanServer server = server(); + UnreliableDataSource ds = newUnreliableDataSource(); + ds.createPool(); + ObjectName dsName = uniqueName("once"); + ObjectName poolOname = ds.createObjectName(dsName); + try { + ds.preRegister(server, dsName); + Assertions.assertTrue(server.isRegistered(poolOname)); + + ds.failNextRelease(); + // The deregistration callback must ride over the failing release + // too, and the handle it clears must stay cleared. + ds.postDeregister(); + Assertions.assertNull(ds.heldHandle(), + "the callback must leave no handle behind even when the " + + "unregistration it attempted failed"); + + // Every later release path finds the handle gone and attempts + // nothing: exactly one attempt was made, by the callback. + ds.close(true); + Assertions.assertEquals(1, ds.releaseAttempts(), + "a failed release must be attempted exactly once: the " + + "handle is taken before the unregistration, so no " + + "later close or callback may re-attempt a name " + + "this data source has given up"); + } finally { + ds.close(); + sweep(server, dsName); + } + } +} diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestCloseVsMonitorOccupancy.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestCloseVsMonitorOccupancy.java new file mode 100644 index 0000000000..1c4887668f --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestCloseVsMonitorOccupancy.java @@ -0,0 +1,319 @@ +/* + * 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.management.ManagementFactory; +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.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 javax.management.MBeanRegistration; +import javax.management.MBeanServer; +import javax.management.ObjectName; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; + +/** + * The data source monitor serializes pool publication, closure and the JMX + * registration lifecycle - and it must be held for those steps only. A + * driver call against a hung database is unbounded and uninterruptible, so + * anything the monitor may not straddle is exactly such a call. + * <p> + * Before this fix, {@code close(boolean)} held the monitor across the whole + * teardown - the physical disconnect of every pooled session - so a close + * racing a hung database parked every later first borrow (which needs the + * monitor at its publication step) and every container JMX callback behind + * an uninterruptible driver wait. Note that the other waits taken under this + * monitor cannot reach a driver call: the connection-MBean sweep's + * {@code con.lock()} only ever meets queue-resident lock regions, and every + * lock region that spans a driver call has claimed the connection out of + * the queues first, so the sweep waits at most on queue bookkeeping. + * <p> + * The test parks the close drain inside the driver's disconnect and then + * checks that the paths needing the monitor run to their outcome meanwhile: + * a borrow fails fast with the closed rejection instead of parking, and the + * container's registration completes, publishing nothing of the pool the + * close already took. + */ +class TestCloseVsMonitorOccupancy extends JdbcPoolBaseTest { + + private static MBeanServer server() { + return ManagementFactory.getPlatformMBeanServer(); + } + + /* A name no other test of the suite uses, so nothing collides by accident. */ + private static ObjectName uniqueName(String role) throws Exception { + return new ObjectName("tomcat:type=MonitorOccupancyTest,role=" + role + + ",uid=" + System.nanoTime()); + } + + /* Test hygiene: take away anything a broken build left published for this + registration - the uid is carried down into every name derived from it. */ + private static void sweepNames(MBeanServer server, ObjectName dsName) throws Exception { + String uid = dsName.getKeyProperty("uid"); + for (ObjectName name : server.queryNames(new ObjectName("tomcat.jdbc:*"), null)) { + if (uid.equals(name.getKeyProperty("uid"))) { + server.unregisterMBean(name); + } + } + } + + /* + * A modeler-style wrapper of the kind a container registers in place of a + * DataSource, forwarding the registration callbacks to it. + */ + public interface PoolHostMBean { + String getMarker(); + } + + public static class PoolHost implements PoolHostMBean, MBeanRegistration { + private final DataSource ds; + + PoolHost(DataSource ds) { + this.ds = ds; + } + + @Override + public String getMarker() { + return "pool-host"; + } + + @Override + public ObjectName preRegister(MBeanServer server, ObjectName name) throws Exception { + ds.preRegister(server, name); + return name; + } + + @Override + public void postRegister(Boolean registrationDone) { + ds.postRegister(registrationDone); + } + + @Override + public void preDeregister() throws Exception { + ds.preDeregister(); + } + + @Override + public void postDeregister() { + ds.postDeregister(); + } + } + + /* + * An H2 driver whose physical connections park inside close() once, so + * the close drain can be held on the driver while the test observes the + * paths that need the data source monitor. + */ + public static class ParkingCloseDriver implements Driver { + + static final CountDownLatch CLOSE_ENTERED = new CountDownLatch(1); + static final CountDownLatch CLOSE_RELEASED = new CountDownLatch(1); + + private final Driver delegate = new org.h2.Driver(); + + @Override + public Connection connect(String url, Properties info) throws SQLException { + Connection connection = delegate.connect(url, info); + if (connection == null) { + return null; + } + return (Connection) Proxy.newProxyInstance( + ParkingCloseDriver.class.getClassLoader(), + new Class<?>[] { Connection.class }, + new ParkingCloseHandler(connection)); + } + + @Override + public boolean acceptsURL(String url) throws SQLException { + return delegate.acceptsURL(url); + } + + @Override + public DriverPropertyInfo[] getPropertyInfo(String url, Properties info) throws SQLException { + return delegate.getPropertyInfo(url, info); + } + + @Override + public int getMajorVersion() { + return delegate.getMajorVersion(); + } + + @Override + public int getMinorVersion() { + return delegate.getMinorVersion(); + } + + @Override + public boolean jdbcCompliant() { + return delegate.jdbcCompliant(); + } + + @Override + public Logger getParentLogger() throws SQLFeatureNotSupportedException { + return delegate.getParentLogger(); + } + } + + /* Parks the first close() on the data source monitor's account. */ + private static class ParkingCloseHandler implements InvocationHandler { + + private final Connection vendor; + private boolean parked; + + ParkingCloseHandler(Connection vendor) { + this.vendor = vendor; + } + + @Override + public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { + if (method.getName().equals("close") && !parked) { + parked = true; + ParkingCloseDriver.CLOSE_ENTERED.countDown(); + try { + ParkingCloseDriver.CLOSE_RELEASED.await(AWAIT_TIMEOUT_MS * 4L, TimeUnit.MILLISECONDS); + } catch (InterruptedException x) { + Thread.currentThread().interrupt(); + throw new SQLException(x); + } + } + try { + return method.invoke(vendor, args); + } catch (InvocationTargetException x) { + throw x.getCause(); + } + } + } + + @Test + void aCloseParkedInADriverDisconnectDoesNotParkBorrowsOrJmxCallbacks() throws Exception { + MBeanServer server = server(); + DataSource ds = newDataSource(); + ds.setDriverClassName(ParkingCloseDriver.class.getName()); + ds.getPoolProperties().setJmxEnabled(true); + ds.getPoolProperties().setInitialSize(1); + ds.getPoolProperties().setMinIdle(0); + ds.createPool(); + + ObjectName dsName = uniqueName("close-vs-monitor"); + ObjectName poolOname = ds.createObjectName(dsName); + + // The drain: close takes the pool and reaches the driver's disconnect + // of the one idle session, where it parks. + AtomicReference<Throwable> closeFailure = new AtomicReference<>(); + Thread closer = new Thread(() -> { + try { + ds.close(true); + } catch (Throwable x) { + closeFailure.set(x); + } + }, "close parked in disconnect"); + closer.setDaemon(true); + Thread borrower = null; + Thread registrar = null; + try { + closer.start(); + Assertions.assertTrue(ParkingCloseDriver.CLOSE_ENTERED.await(AWAIT_TIMEOUT_MS, + TimeUnit.MILLISECONDS), + "the close must have reached the driver's disconnect"); + Assertions.assertTrue(closer.isAlive(), "the premise: the drain is parked on the driver"); + + // The paths that need the data source monitor while the teardown + // waits on the driver: + // a first borrow must fail fast as closed, not park behind it... + AtomicReference<SQLException> borrowFailure = new AtomicReference<>(); + AtomicReference<Connection> borrowed = new AtomicReference<>(); + borrower = new Thread(() -> { + try { + borrowed.set(ds.getConnection()); + } catch (SQLException x) { + borrowFailure.set(x); + } + }, "borrow racing the parked teardown"); + borrower.setDaemon(true); + borrower.start(); + borrower.join(AWAIT_TIMEOUT_MS); + Assertions.assertFalse(borrower.isAlive(), + "a borrow must not park behind the driver disconnect of an " + + "in-flight close: against a hung database the close's " + + "wait is unbounded and uninterruptible, so it would " + + "park the borrow forever"); + Assertions.assertNotNull(borrowFailure.get(), "the borrow 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()); + + // ...and the container's registration callback must complete (the + // pool is already taken, so it publishes nothing of the dead pool). + AtomicReference<Throwable> registrationFailure = new AtomicReference<>(); + registrar = new Thread(() -> { + try { + server.registerMBean(new PoolHost(ds), dsName); + } catch (Throwable x) { + registrationFailure.set(x); + } + }, "registration racing the parked teardown"); + registrar.setDaemon(true); + registrar.start(); + registrar.join(AWAIT_TIMEOUT_MS); + Assertions.assertFalse(registrar.isAlive(), + "the container's registration callback must not park behind " + + "the driver disconnect of an in-flight close"); + Assertions.assertNull(registrationFailure.get(), "the registration must not fail"); + Assertions.assertFalse(server.isRegistered(poolOname), + "a data source whose pool a running close already took has no " + + "MBean to publish and must publish nothing"); + + // Let the teardown finish: everything the parked close owned is + // released by it. + ParkingCloseDriver.CLOSE_RELEASED.countDown(); + closer.join(AWAIT_TIMEOUT_MS); + Assertions.assertFalse(closer.isAlive(), "the close must complete once released"); + Assertions.assertNull(closeFailure.get(), "the close must not fail"); + awaitSessions(0); + } finally { + ParkingCloseDriver.CLOSE_RELEASED.countDown(); + if (borrower != null) { + borrower.join(AWAIT_TIMEOUT_MS); + } + if (registrar != null) { + registrar.join(AWAIT_TIMEOUT_MS); + } + if (server.isRegistered(dsName)) { + server.unregisterMBean(dsName); + } + ds.close(); + sweepNames(server, dsName); + } + } + +} diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestDataSourceReregistration.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestDataSourceReregistration.java new file mode 100644 index 0000000000..83caa378ac --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestDataSourceReregistration.java @@ -0,0 +1,251 @@ +/* + * 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.jmx; + +import java.lang.management.ManagementFactory; +import java.util.Set; + +import javax.management.InstanceAlreadyExistsException; +import javax.management.MBeanServer; +import javax.management.ObjectName; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; +import org.apache.tomcat.jdbc.pool.DataSource; + +/** + * The registration handle of {@code DataSource} ({@code oname}) is the one + * slot through which {@code close()}, {@code postRegister(false)} and + * {@code postDeregister()} take the pool MBean back; the lifecycle promises + * pin that it is released exactly once and that a name this data source never + * published is never released. Both promises need the slot to name a publish + * that is in effect - so the slot may be written only by a completed publish, + * and no failure or skip of a (re-)registration may clear or replace a handle + * it does not own. + * <p> + * Before this fix the slot was pre-written with the derived name before the + * publish was even attempted and reassigned unconditionally from the helper's + * answer, so a rejected re-publish of a name already held (a second call into + * {@code registerJmx()}) cleared the live handle - orphaning that MBean + * permanently, since neither a later close nor a deregistration could find it + * any more - and a second registration under another name replaced the handle + * and leaked the first publish the same way. (The third interleaving the + * defect report listed - the connection sweep throwing after a completed + * publish, whose exception path used to null the handle - is unreachable + * today: the sweep's helpers swallow every checked failure and neither the + * locks nor the queue iterators it uses throw. Its closure by construction, + * the slot being written before the sweep and the exception path no longer + * clearing it, is carried by review rather than by a test of its own.) + */ +class TestDataSourceReregistration extends JdbcPoolBaseTest { + + private static MBeanServer server() { + return ManagementFactory.getPlatformMBeanServer(); + } + + /* A name no other test of the suite uses, so nothing collides by accident. */ + private static ObjectName uniqueName(String role) throws Exception { + return new ObjectName("tomcat:type=ReregistrationTest,role=" + role + + ",uid=" + System.nanoTime()); + } + + /* Test hygiene: take away anything a broken build left published for this + registration - the uid is carried down into every name derived from it. */ + private static void sweep(MBeanServer server, ObjectName uidBearer) throws Exception { + String uid = uidBearer.getKeyProperty("uid"); + for (ObjectName name : server.queryNames(new ObjectName("tomcat.jdbc:*"), null)) { + if (uid.equals(name.getKeyProperty("uid"))) { + server.unregisterMBean(name); + } + } + } + + /* A data source exposing the two protected members the tests observe. */ + private static final class ProbeDataSource extends DataSource { + ObjectName heldHandle() { + return oname; + } + + void registerAgain() { + registerJmx(); + } + } + + private ProbeDataSource newProbeDataSource() { + ProbeDataSource ds = new ProbeDataSource(); + ds.setUrl(getUrl()); + ds.setDriverClassName("org.h2.Driver"); + ds.setUsername("sa"); + ds.setPassword(""); + ds.getPoolProperties().setJmxEnabled(true); + ds.getPoolProperties().setInitialSize(0); + ds.getPoolProperties().setMinIdle(0); + return manage(ds); + } + + @Test + void aRefusedRePublishKeepsTheHeldHandle() throws Exception { + MBeanServer server = server(); + ProbeDataSource ds = newProbeDataSource(); + ds.createPool(); + ObjectName dsName = uniqueName("duplicate-call"); + ObjectName poolOname = ds.createObjectName(dsName); + try { + server.registerMBean(new TestJmxRegistration.Container(ds), dsName); + Assertions.assertTrue(server.isRegistered(poolOname), + "the registration must have published the pool MBean"); + Assertions.assertEquals(poolOname, ds.heldHandle(), + "the handle must name the publish in effect"); + + // A direct second call aims at the name the held publish took; + // the server refuses the duplicate. The refusal is not a + // registration of this call - the handle of the publish that IS + // in effect must survive it, or nothing could ever take that + // MBean back. + ds.registerAgain(); + Assertions.assertEquals(poolOname, ds.heldHandle(), + "a publish the server refused must leave the held handle " + + "in place: clearing it here orphaned the live " + + "MBean permanently"); + Assertions.assertTrue(server.isRegistered(poolOname), + "the held publish must still be registered"); + + // The handle still works: the close takes the MBean back. + ds.close(true); + Assertions.assertFalse(server.isRegistered(poolOname), + "the close of a data source that kept its handle must " + + "take the pool MBean back"); + Assertions.assertNull(ds.heldHandle(), "the close must leave no handle behind"); + } finally { + if (server.isRegistered(dsName)) { + server.unregisterMBean(dsName); + } + ds.close(); + sweep(server, dsName); + } + } + + @Test + void aSecondRegistrationNeitherOrphansTheFirstPublishNorTakesItsName() throws Exception { + MBeanServer server = server(); + ProbeDataSource ds = newProbeDataSource(); + ds.createPool(); + ObjectName first = uniqueName("first"); + ObjectName second = uniqueName("second"); + ObjectName firstPoolOname = ds.createObjectName(first); + ObjectName secondPoolOname = ds.createObjectName(second); + try { + server.registerMBean(new TestJmxRegistration.Container(ds), first); + Assertions.assertTrue(server.isRegistered(firstPoolOname)); + Assertions.assertEquals(firstPoolOname, ds.heldHandle()); + + // The state before the fix's guard was a second registration of + // the same data source under another name: it overwrote the slot + // with its own derived name and republished the pool under it, so + // the first pool MBean was held by nothing and stayed registered + // for the life of the JVM, and the later deregistrations released + // only the second name. + server.registerMBean(new TestJmxRegistration.Container(ds), second); + Assertions.assertFalse(server.isRegistered(secondPoolOname), + "while the first registration's publish is held, a second " + + "registration must publish nothing of the pool " + + "instead of taking over the handle"); + Assertions.assertEquals(firstPoolOname, ds.heldHandle(), + "the held handle must survive a second registration"); + + // The single handle is released once, by the first lifecycle path + // that runs: the second registration's deregistration finds the + // handle in place and takes it (nothing of its own was ever + // published under the second name), and the first registration's + // then finds nothing left to release. + server.unregisterMBean(second); + Assertions.assertFalse(server.isRegistered(firstPoolOname), + "the pool MBean of the first registration must not stay " + + "registered once the held handle has been " + + "released - it was the leak before the fix"); + Assertions.assertNull(ds.heldHandle(), "the release must clear the handle"); + server.unregisterMBean(first); + Assertions.assertFalse(server.isRegistered(firstPoolOname)); + } finally { + if (server.isRegistered(second)) { + server.unregisterMBean(second); + } + if (server.isRegistered(first)) { + server.unregisterMBean(first); + } + ds.close(); + sweep(server, first); + sweep(server, second); + Set<ObjectName> leaked = server.queryNames( + new ObjectName("tomcat.jdbc:*,uid=" + first.getKeyProperty("uid")), null); + Assertions.assertTrue(leaked.isEmpty(), "no MBean of this test may remain: " + leaked); + } + } + + @Test + void aFailedSecondRegistrationDoesNotReleaseTheFirstRegistrationsPublish() throws Exception { + MBeanServer server = server(); + ProbeDataSource ds = newProbeDataSource(); + ds.createPool(); + ObjectName first = uniqueName("alive"); + ObjectName blocked = uniqueName("failed-second"); + ObjectName firstPoolOname = ds.createObjectName(first); + server.registerMBean(new TestJmxRegistration.Probe("owner"), blocked); + try { + server.registerMBean(new TestJmxRegistration.Container(ds), first); + Assertions.assertTrue(server.isRegistered(firstPoolOname), + "the first registration must have published the pool MBean"); + + // The second registration of the same data source fails against + // the name the Probe above occupies. Its preRegister() published + // nothing - the first registration's handle was held - and the + // server then fails the registration itself, running + // postRegister(false) without any postDeregister(). That callback + // has nothing of this registration to undo: what it must not do + // is take back the publish a different, still-live registration + // made. + Assertions.assertThrows(InstanceAlreadyExistsException.class, + () -> server.registerMBean(new TestJmxRegistration.Container(ds), blocked)); + Assertions.assertTrue(server.isRegistered(firstPoolOname), + "a failed second registration must leave the pool MBean of " + + "the live first registration published"); + Assertions.assertEquals(firstPoolOname, ds.heldHandle(), + "a failed second registration must leave the handle of the " + + "live first registration in place"); + + // The surviving handle still works: deregistration takes the MBean + // back exactly once. + server.unregisterMBean(first); + Assertions.assertFalse(server.isRegistered(firstPoolOname), + "the deregistration of the surviving registration must still " + + "take the pool MBean back"); + Assertions.assertNull(ds.heldHandle(), "the release must clear the handle"); + } finally { + if (server.isRegistered(first)) { + server.unregisterMBean(first); + } + if (server.isRegistered(blocked)) { + server.unregisterMBean(blocked); + } + ds.close(); + sweep(server, first); + sweep(server, blocked); + } + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
