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 41eeb41e36ec8e9bc43829f525099b42d9baaed2 Author: remm <[email protected]> AuthorDate: Wed Oct 28 16:15:58 2026 +0100 Keep only a published JMX name as the DataSource unregistration handle The data source registered the pool MBean through the JmxUtil helper but threw the helper's result away, while the object name it later unregisters with had already been assigned when the container declared the registration. The helper hands back the name of a publish the server accepted, or null when the server refused it - most often because the name is already in use by another MBean. A refused publish therefore left the data source holding a name it never published, and closing the data source, or the container later deregistering it, unregistered that name from the foreign MBean that really owned it, silently taking away the management surface of a component that is running perfectly well. Honor the helper's contract the way every other call site already does: keep the returned name as the handle, which is null when the server refused the registration, so a name a foreign MBean owns is never released as if it were this pool's registration. The pooled connections and the interceptors have followed this rule since the helper itself was corrected; this was the last site ignoring it. Cover it with a test that a DataSource whose pool publish was refused leaves the MBean that owns the name registered when it closes, in the style of the existing tests that pin the same contract for the interceptor MBeans. --- .../org/apache/tomcat/jdbc/pool/DataSource.java | 10 +++- .../tomcat/jdbc/pool/jmx/TestJmxRegistration.java | 64 +++++++++++++++++++++- 2 files changed, 70 insertions(+), 4 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 f83011ed91..8c3e33c637 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 @@ -167,7 +167,9 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, * (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. + * 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. */ protected void registerJmx() { final ConnectionPool p = pool; @@ -179,7 +181,11 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, this.oname = null; return; } - JmxUtil.registerJmx(oname, null, p.getJmxPool()); + // 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, p.getJmxPool()); } /** diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxRegistration.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxRegistration.java index afb48cb014..7f2595876f 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxRegistration.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxRegistration.java @@ -52,10 +52,13 @@ import org.apache.tomcat.jdbc.pool.interceptor.StatementCache; * perfectly well.</p> * <p> * The contract the first tests pin is that the returned name is this call's own - * publish, or nothing at all. The last two drive that through a real pool: a + * publish, or nothing at all. The last three drive that through a real pool: a * {@code StatementCache} whose name is taken must leave the MBean that owns the * name registered when its connection is discarded, and it must publish itself - * at the next borrow once the obstacle is gone.</p> + * at the next borrow once the obstacle is gone; the {@code DataSource} one + * level up must keep the same books for the pool MBean it publishes, whose + * handle it releases at {@code close()} and at the container's + * {@code postDeregister()}.</p> */ class TestJmxRegistration extends JdbcPoolBaseTest { @@ -128,6 +131,16 @@ class TestJmxRegistration extends JdbcPoolBaseTest { return (ObjectName) handle.get(cache); } + /* + * The pool-MBean handle a DataSource keeps for itself: the same state, + * read the same way, one level up. + */ + private static ObjectName handleOf(DataSource ds) throws Exception { + Field handle = DataSource.class.getDeclaredField("oname"); + handle.setAccessible(true); + return (ObjectName) handle.get(ds); + } + /* * The MBean of the one pooled connection that has been handed out, below the * given pool MBean. An interceptor's MBean sits below that and repeats the @@ -365,4 +378,51 @@ class TestJmxRegistration extends JdbcPoolBaseTest { ds.postDeregister(); } } + + @Test + void aDataSourceThatWasRefusedItsPoolNameDoesNotUnregisterTheMBeanThatOwnsIt() + throws Exception { + MBeanServer server = server(); + DataSource ds = newDataSource(); + ds.getPoolProperties().setJmxEnabled(true); + ds.createPool(); + ObjectName dsName = uniqueName("datasource"); + // the exact name the DataSource will ask the server for: createObjectName + // is a deterministic mapping of the DataSource's own name, so two data + // sources - or a data source and the leftover of one that died without + // deregistering - derive the same name from the same keys + ObjectName foreign = ds.createObjectName(dsName); + server.registerMBean(new Probe("foreign"), foreign); + try { + // the container registers the DataSource, and the pool MBean the + // DataSource publishes from preRegister() is refused its name + ds.preRegister(server, dsName); + Assertions.assertEquals("foreign", server.getAttribute(foreign, "Marker"), + "a refused publish must not have touched the MBean holding the name"); + // read the handle before close(): close() clears whatever handle is + // held, so after it the read no longer says what the refusal left + ObjectName kept = handleOf(ds); + + // the harm, on the DataSource's own lifecycle: close() and the + // container's postDeregister() each unregister the handle they hold + ds.close(); + ds.postDeregister(); + Assertions.assertTrue(server.isRegistered(foreign), + "closing a DataSource whose pool publish was refused must leave the MBean" + + " that owns that name registered: unregistering it takes the" + + " management surface of a component that has done nothing wrong," + + " and does so silently"); + Assertions.assertNull(kept, + "a DataSource whose pool publish was refused must not keep the refused" + + " name as its registration handle: that handle is exactly what" + + " close() and postDeregister() unregister, and this data source" + + " never published under it"); + } finally { + if (server.isRegistered(foreign)) { + server.unregisterMBean(foreign); + } + ds.close(); + ds.postDeregister(); + } + } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
