This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 1418f53742b6cd1a9213fbcd665d8b9da4655d46 Author: opencode <[email protected]> AuthorDate: Fri Nov 6 08:38:50 2026 +0100 Refuse a second DataSource JMX registration A DataSource already holding a pool MBean handle accepted a second MBean registration, but its postDeregister() could not identify which registration was ending and could release the first registration's live pool MBean. Refuse the second registration with an MBeanRegistrationException, leaving the existing publication untouched. --- .../org/apache/tomcat/jdbc/pool/DataSource.java | 164 ++++++--------------- .../tomcat/jdbc/pool/util/LocalStrings.properties | 2 +- .../pool/jmx/TestDataSourceReregistration.java | 66 ++++----- 3 files changed, 75 insertions(+), 157 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 bbaa0c1b72..18ad86cb7c 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 @@ -19,6 +19,7 @@ package org.apache.tomcat.jdbc.pool; import java.util.Hashtable; import javax.management.MBeanRegistration; +import javax.management.MBeanRegistrationException; import javax.management.MBeanServer; import javax.management.MalformedObjectNameException; import javax.management.ObjectName; @@ -75,19 +76,9 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, */ @Override public void close(boolean all) { - // This monitor serializes the registration lifecycle against the - // closure: preRegister(), postRegister() and postDeregister() take it - // 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. + // The monitor serializes the registration lifecycle against the + // closure. It is not held across the pool teardown, which can block on + // a driver call. final ConnectionPool p; synchronized (this) { releasePoolMBean(); @@ -97,35 +88,24 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, } /** - * 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 - * 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. + * The JMX ObjectName for the connection pool MBean. Written only by a + * completed publish, cleared only when that MBean is released, and + * accessed under this data source's monitor. */ 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. + * Whether the publish named by {@link #oname} was made by the registration + * currently in flight. Set by {@link #registerJmx()} when a + * registration-driven publish is accepted and cleared when that + * registration completes. */ 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 #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. + * The name derived by the registration currently in flight, used by + * {@link #preRegister(MBeanServer, ObjectName)} so a publish cannot + * overwrite the handle of a registration that is already held. */ private volatile ObjectName pendingName = null; @@ -135,24 +115,13 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, */ @Override public synchronized void postDeregister() { - // Under the monitor that close() and the registration callbacks take, - // so the container's deregistration and a concurrent close release the - // pool MBean exactly once: whichever of them runs second finds the - // handle already gone and unregisters nothing - not even a name some - // third component has published in the meantime. releasePoolMBean(); } /** - * Releases the pool MBean that {@link #preRegister(MBeanServer, ObjectName)} published when the MBean server did - * not complete this DataSource's own registration. Per the {@link MBeanRegistration} contract a {@code false} - * {@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. 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> + * Releases the pool MBean published by this registration when the MBean server did not complete it. A failed + * registration does not run {@link #postDeregister()}, so this is the only chance to undo a publish this + * registration made. A registration that published nothing releases nothing.<br> * {@inheritDoc} */ @Override @@ -176,28 +145,26 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, } /** - * 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> + * If the connection pool MBean exists, it is registered during this operation. A second registration is refused + * while this data source already holds a publication.<br> * {@inheritDoc} */ @Override 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. + // The publish runs under the monitor close() takes. The connection + // sweep runs outside it, since it can block on a connection lock. ConnectionPool sweepPool = null; try { synchronized (this) { - if (isJmxEnabled() && oname == null) { + if (oname != null) { + // A second registration has no handle of its own, and + // postDeregister() cannot tell which registration it is + // ending. Refuse it rather than let its deregistration + // release the live registration's MBean. + String message = MessageFactory.get("dataSource.mbeanAlreadyRegistered", oname, name); + throw new MBeanRegistrationException(new IllegalStateException(message), message); + } + if (isJmxEnabled()) { final ObjectName derived = createObjectName(name); if (derived != null) { pendingName = derived; @@ -207,14 +174,16 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, pendingName = null; } } - } 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: // there is nothing here to take back. log.error(MessageFactory.get("dataSource.unableToCreateObjectName"), x); + } catch (MBeanRegistrationException x) { + // The second registration is refused; the first registration's + // publish is left exactly as it stands. + throw x; } catch (RuntimeException x) { // Never break the caller's MBean registration because of // pool-side state (for instance a pool that has not been @@ -261,13 +230,9 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, } /** - * Registers the ConnectionPoolMBean under a unique name based on the ObjectName for the DataSource. A pool that has - * 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. 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. + * Registers the ConnectionPoolMBean under a unique name derived from the DataSource's ObjectName. A pool that has + * not been created, or a publish the server refused, is skipped without disturbing a handle this data source + * already holds. */ protected void registerJmx() { final ConnectionPool p; @@ -289,43 +254,23 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, */ 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 - // be followed by another that observes the null - a registration - // attempt against that null would only fail inside the helper, with a - // spurious error log + // Read the pool's JMX object once: a pool closed in the meantime has + // given it up. 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. + // A registration in flight publishes the name it derived. A direct + // call publishes the handle it already holds, so a duplicate attempt + // is refused and the handle survives. final ObjectName name = pendingName == null ? oname : pendingName; if (jmx == 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 null; } final ObjectName published = JmxUtil.registerJmx(name, null, jmx); if (published != null) { - // 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 - // 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. + // Only a publish made from preRegister() can be undone by a + // failed registration in postRegister(false). 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 - // 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; @@ -365,19 +310,9 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, } /** - * Releases the registration handle and unregisters the pool MBean - * published under the name it points to, as one step under this data - * source's monitor. Every path that gives the handle up runs through - * 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. 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. + * Releases the registration handle and unregisters the pool MBean it names, under this data source's monitor. Every + * release path goes through here, so the handle is given up at most once. The unregistration itself is best effort + * and never breaks the caller. */ private void releasePoolMBean() { final ObjectName name = oname; @@ -388,11 +323,6 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, 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/util/LocalStrings.properties b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties index 6a6080c7f3..abc1d8e752 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 @@ -97,7 +97,7 @@ connectionState.unableToResetTransactionIsolation=Unable to reset the transactio 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.mbeanAlreadyRegistered=JDBC pool MBean is already published under [{0}]; the second registration under [{1}] is refused and the existing publication is left unchanged. 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. 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 index 83caa378ac..7386a6baf6 100644 --- 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 @@ -19,7 +19,7 @@ package org.apache.tomcat.jdbc.pool.jmx; import java.lang.management.ManagementFactory; import java.util.Set; -import javax.management.InstanceAlreadyExistsException; +import javax.management.MBeanRegistrationException; import javax.management.MBeanServer; import javax.management.ObjectName; @@ -155,33 +155,25 @@ class TestDataSourceReregistration extends JdbcPoolBaseTest { 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); + // A second registration of the same data source has no handle of + // its own, and postDeregister() cannot tell which registration it + // is ending. It is refused so that its deregistration cannot + // release the publish of the still-live first registration. + Assertions.assertThrows(MBeanRegistrationException.class, + () -> server.registerMBean(new TestJmxRegistration.Container(ds), second), + "a second registration must be refused while the first publish is held"); 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"); + "a refused second registration must publish nothing of the pool"); 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); + "the held handle must survive a refused second registration"); + + // The surviving handle still works: deregistering the first + // registration takes the MBean back exactly once. + server.unregisterMBean(first); 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"); + "the deregistration of the surviving registration must take " + + "the pool MBean back"); 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); @@ -204,24 +196,20 @@ class TestDataSourceReregistration extends JdbcPoolBaseTest { ProbeDataSource ds = newProbeDataSource(); ds.createPool(); ObjectName first = uniqueName("alive"); - ObjectName blocked = uniqueName("failed-second"); + ObjectName second = 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)); + // The second registration of the same data source is refused + // before the server gets a chance to fail it against another + // name. A failed registration runs no postDeregister(), but it + // must not take back the publish of the still-live first one. + Assertions.assertThrows(MBeanRegistrationException.class, + () -> server.registerMBean(new TestJmxRegistration.Container(ds), second), + "the second registration must be refused"); Assertions.assertTrue(server.isRegistered(firstPoolOname), "a failed second registration must leave the pool MBean of " + "the live first registration published"); @@ -240,12 +228,12 @@ class TestDataSourceReregistration extends JdbcPoolBaseTest { if (server.isRegistered(first)) { server.unregisterMBean(first); } - if (server.isRegistered(blocked)) { - server.unregisterMBean(blocked); + if (server.isRegistered(second)) { + server.unregisterMBean(second); } ds.close(); sweep(server, first); - sweep(server, blocked); + sweep(server, second); } } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
