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 a5ed174f52051ffcceb59eb0dd771f263c2cd778 Author: opencode <[email protected]> AuthorDate: Thu Nov 5 13:13:37 2026 +0100 Run the connection MBean sweep outside the DataSource monitor DataSource.registerJmx() was synchronized and published the pool MBean and then swept every pooled connection while still holding the DataSource monitor. The sweep takes each connection's lock, which pool maintenance holds across validation and reconnect driver calls, so a JMX registration could park close and first-borrow publication behind an unresponsive database. Split the publication into publishJmx(), which runs under the monitor, and run registerConnectionMBeans() after releasing it. Update the registration-vs-close test seam to hook publishJmx() and add a test that a lifecycle callback completes while the sweep is parked on a held connection lock. --- .../org/apache/tomcat/jdbc/pool/DataSource.java | 98 ++++++++++++++-------- .../apache/tomcat/jdbc/pool/mbeans-descriptors.xml | 14 ++-- .../jmx/TestDataSourceRegistrationVsClose.java | 77 ++++++++++++++++- 3 files changed, 147 insertions(+), 42 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 6950f51695..27dd5c4079 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 @@ -33,6 +33,10 @@ import org.apache.tomcat.jdbc.pool.util.MessageFactory; * A DataSource that can be instantiated through IoC and implements the DataSource interface since the DataSourceProxy * is used as a generic proxy. The DataSource simply wraps a {@link ConnectionPool} in order to provide a standard * interface to the user. + * <p> + * A JMX registration made before the pool exists publishes nothing, because there is no pool MBean to publish. Create + * the pool before registering the data source, or register it again after the pool has been created, to have the pool + * monitored. */ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, MBeanRegistration, org.apache.tomcat.jdbc.pool.jmx.ConnectionPoolMBean, javax.sql.ConnectionPoolDataSource { @@ -85,8 +89,6 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, 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. It is written @@ -113,7 +115,7 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, /** * 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 + * into {@link #publishJmx()}, 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. @@ -164,7 +166,6 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, */ @Override public void preDeregister() throws Exception { - // NOOP } /** @@ -174,27 +175,34 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, * {@inheritDoc} */ @Override - public synchronized ObjectName preRegister(MBeanServer server, ObjectName name) throws Exception { - // Under the monitor close() takes, so the publication below cannot - // overlap a closure: a close that ran completely between registerJmx() - // reading the registration name and the server actually accepting the - // publish would unregister a name the pool never took and close the - // pool, and the publish would then register the dead pool's MBean - // under a live name that no later close() or postDeregister() - which - // only release a held handle - could ever take back. + public ObjectName preRegister(MBeanServer server, ObjectName name) throws Exception { + // The publish runs under the monitor close() takes, so it cannot + // overlap a closure: a close that ran completely between the name + // being derived and the server accepting the publish would unregister + // a name the pool never took and close the pool, and the publish would + // then register the dead pool's MBean under a live name that no later + // close() or postDeregister() - which only release a held handle - + // could ever take back. The connection sweep that follows the publish + // runs outside the monitor: the sweep takes each connection's lock, + // which maintenance holds across validation and reconnect driver + // calls, so holding the data source monitor across it would park a + // close or a later publication behind a hung database. + ConnectionPool sweepPool = null; try { - if (isJmxEnabled() && oname == null) { - final ObjectName derived = createObjectName(name); - if (derived != null) { - try { + synchronized (this) { + if (isJmxEnabled() && oname == null) { + final ObjectName derived = createObjectName(name); + if (derived != null) { pendingName = derived; - registerJmx(); - } finally { - pendingName = null; + try { + sweepPool = publishJmx(); + } finally { + pendingName = null; + } } + } else if (isJmxEnabled() && oname != null) { + log.warn(MessageFactory.get("dataSource.mbeanAlreadyRegistered", oname, name)); } - } else if (isJmxEnabled() && oname != null) { - log.warn(MessageFactory.get("dataSource.mbeanAlreadyRegistered", oname, name)); } } catch (MalformedObjectNameException x) { // Nothing has been published and the handle slot was not touched: @@ -205,12 +213,19 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, // pool-side state (for instance a pool that has not been // created yet); preRegister throwing aborts the whole // registration in the MBean server. The handle slot is left as - // registerJmx() last wrote it: a RuntimeException after a + // publishJmx() 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); } + if (sweepPool != null) { + try { + sweepPool.registerConnectionMBeans(); + } catch (RuntimeException x) { + log.error(MessageFactory.get("dataSource.unableToRegisterMBean"), x); + } + } return name; } @@ -247,10 +262,25 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, * 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 - // direct call - the monitor close(boolean) also takes, so no closure - // can interleave with the publish below. + protected void registerJmx() { + final ConnectionPool p; + synchronized (this) { + p = publishJmx(); + } + if (p != null) { + p.registerConnectionMBeans(); + } + } + + /** + * Publishes the pool MBean under this data source's monitor and returns + * the pool whose connections must then be published, or {@code null} when + * nothing was published. The caller must hold this monitor; the + * connection sweep is the caller's to run outside it. + * + * @return the pool whose connections need publishing, or {@code null} + */ + protected ConnectionPool publishJmx() { final ConnectionPool p = pool; // Read the pool's JMX object only once: a pool that was closed before // this registration has given it up, and one read that saw it must not @@ -268,13 +298,13 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, // 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; + return 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. + // Recorded before the caller runs the connection sweep: 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 @@ -286,10 +316,12 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, // 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 - // they are published now. A publish the server refused registers - // nothing of its own and has no connections to publish either. - p.registerConnectionMBeans(); + // the caller publishes them now. A publish the server refused + // registers nothing of its own and has no connections to publish + // either. + return p; } + return null; } /** diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/mbeans-descriptors.xml b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/mbeans-descriptors.xml index be6ba5a585..1e78ab914d 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/mbeans-descriptors.xml +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/mbeans-descriptors.xml @@ -32,12 +32,12 @@ writeable="false"/> <attribute name="size" - description="The number of established connections in the pool, idle and in use" + description="The current pool size: idle and in-use connection slots, including wake-up placeholders for waiting borrowers" type="java.lang.Integer" writeable="false"/> <attribute name="idle" - description="The number of established connections in the pool that are idle" + description="The number of idle entries, including wake-up placeholders for waiting borrowers" type="java.lang.Integer" writeable="false"/> @@ -47,7 +47,7 @@ writeable="false"/> <attribute name="active" - description="The number of established connections in the pool that are in use" + description="The number of connections currently in use by borrowers" type="java.lang.Integer" writeable="false"/> @@ -57,7 +57,7 @@ writeable="false"/> <attribute name="poolSweeperEnabled" - description="Returns true if the pool has a background thread running" + description="True if the pool configuration enables the background sweeper" type="java.lang.Boolean" is="true" writeable="false"/> @@ -95,7 +95,7 @@ writeable="false"/> <attribute name="defaultCatalog" - description="The JDBC transaction isolation setting for new connections" + description="The default JDBC catalog for new connections" type="java.lang.String" writeable="false"/> @@ -191,7 +191,7 @@ writeable="false"/> <attribute name="loginTimeout" - description="Not in use" + description="The login timeout in seconds, mapped to the pool's maxWait" type="java.lang.Integer" writeable="false"/> @@ -385,7 +385,7 @@ returnType="void" /> <operation name="testIdle" - description="forces a validation of abandoned connections" + description="Runs a validation and eviction pass over the idle connections" impact="ACTION" returnType="void" /> diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestDataSourceRegistrationVsClose.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestDataSourceRegistrationVsClose.java index 90758004e0..0398278011 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestDataSourceRegistrationVsClose.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestDataSourceRegistrationVsClose.java @@ -30,7 +30,9 @@ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.apache.tomcat.jdbc.JdbcPoolBaseTest; +import org.apache.tomcat.jdbc.pool.ConnectionPool; import org.apache.tomcat.jdbc.pool.DataSource; +import org.apache.tomcat.jdbc.pool.PooledConnection; /** * The {@code DataSource}'s JMX lifecycle - {@code preRegister()}, @@ -88,6 +90,7 @@ class TestDataSourceRegistrationVsClose extends JdbcPoolBaseTest { private final AtomicInteger releases = new AtomicInteger(); private volatile CountDownLatch publicationEntered; private volatile CountDownLatch publicationGate = new CountDownLatch(0); + private volatile CountDownLatch publicationDone; private volatile boolean publicationUnderMonitor; private volatile CountDownLatch releaseEntered; private volatile CountDownLatch releaseGate = new CountDownLatch(0); @@ -99,7 +102,7 @@ class TestDataSourceRegistrationVsClose extends JdbcPoolBaseTest { } @Override - protected void registerJmx() { + protected ConnectionPool publishJmx() { publicationUnderMonitor = Thread.holdsLock(this); CountDownLatch entered = publicationEntered; if (entered != null) { @@ -107,7 +110,13 @@ class TestDataSourceRegistrationVsClose extends JdbcPoolBaseTest { entered.countDown(); awaitGate(publicationGate); } - super.registerJmx(); + ConnectionPool published = super.publishJmx(); + CountDownLatch done = publicationDone; + if (done != null) { + publicationDone = null; + done.countDown(); + } + return published; } @Override @@ -286,6 +295,70 @@ class TestDataSourceRegistrationVsClose extends JdbcPoolBaseTest { } } + @Test + void theConnectionSweepDoesNotHoldTheDataSourceMonitor() throws Exception { + MBeanServer server = server(); + SeamDataSource ds = newSeamDataSource(); + ds.createPool(); + ObjectName dsName = uniqueName("sweep-vs-monitor"); + + PooledConnection held; + try (java.sql.Connection connection = ds.getConnection()) { + held = connection.unwrap(PooledConnection.class); + } + // The connection is idle now; hold its lock so the registration's + // connection sweep parks on it. + held.lock(); + ds.publicationDone = new CountDownLatch(1); + AtomicReference<Throwable> registrationFailure = new AtomicReference<>(); + Thread registrar = new Thread(() -> { + try { + server.registerMBean(new TestJmxRegistration.Container(ds), dsName); + } catch (Throwable x) { + registrationFailure.set(x); + } + }, "registration parked in sweep"); + registrar.setDaemon(true); + try { + registrar.start(); + Assertions.assertTrue(ds.publicationDone.await(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS), + "the registration must have published the pool MBean"); + + // The publication is done and the sweep is now stuck on the + // connection lock. A lifecycle callback that touches no connection + // must not be behind that sweep. + AtomicBoolean callbackDone = new AtomicBoolean(); + AtomicReference<Throwable> callbackFailure = new AtomicReference<>(); + Thread callback = new Thread(() -> { + try { + ds.postRegister(Boolean.TRUE); + } catch (Throwable x) { + callbackFailure.set(x); + } finally { + callbackDone.set(true); + } + }, "lifecycle during sweep"); + callback.setDaemon(true); + callback.start(); + callback.join(2000); + Assertions.assertTrue(callbackDone.get(), + "the connection sweep must run outside the data source monitor: a " + + "lifecycle callback was parked behind a sweep waiting on a " + + "connection lock"); + Assertions.assertNull(callbackFailure.get(), "the lifecycle callback must not fail"); + } finally { + held.unlock(); + registrar.join(AWAIT_TIMEOUT_MS); + if (server.isRegistered(dsName)) { + server.unregisterMBean(dsName); + } + ds.close(); + sweep(server, dsName); + } + Assertions.assertFalse(registrar.isAlive(), "the registration must complete once the connection is released"); + Assertions.assertNull(registrationFailure.get(), "the registration must not fail"); + } + @Test void aDeregistrationAndACloseReleaseThePoolMBeanExactlyOnce() throws Exception { MBeanServer server = server(); --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
