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 d8a51a847b5cfd35518120fb4a70603f4770595d Author: opencode <[email protected]> AuthorDate: Thu Oct 29 21:39:53 2026 +0100 Undo the pool MBean publication when DataSource registration fails MBeanRegistration hands a bean whose registration the server failed postRegister(FALSE) and no postDeregister(), making that callback the only chance to undo a preRegister side effect; the DataSource ignored the flag, so a pool MBean published from preRegister() stayed registered for a DataSource no container ever adopted: reporting statistics and notifications for a pool nobody manages, and pinning the pool, its credentials and its JDBC driver class loader in the MBean server - the stale registration 4f91400310 was written to prevent, re-entering through the failed-registration path. A failed registration now releases the pool MBean handle the same way postDeregister() and close() do, which makes a later close() skip the unregister rather than perform it twice. The MBean server itself cannot fail a raw DataSource registration - it is not a compliant MBean - so the regression test drives the lifecycle through a modeler-style forwarder mirroring BaseModelMBean, which is how a container reaches these callbacks. --- .../org/apache/tomcat/jdbc/pool/DataSource.java | 18 +- .../tomcat/jdbc/pool/jmx/TestJmxRegistration.java | 200 ++++++++++++++++++++- 2 files changed, 213 insertions(+), 5 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 750d666573..f7bb5d8ae0 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 @@ -92,12 +92,26 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, } /** - * no-op<br> + * 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.<br> * {@inheritDoc} */ @Override public void postRegister(Boolean registrationDone) { - // NOOP + // The same check-and-clear of the registration handle as + // postDeregister() and close(): 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) && oname != null) { + unregisterJmx(); + oname = null; + } } 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 7f2595876f..58c2d48798 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 @@ -22,6 +22,8 @@ import java.sql.Connection; import java.util.HashSet; import java.util.Set; +import javax.management.InstanceAlreadyExistsException; +import javax.management.MBeanRegistration; import javax.management.MBeanServer; import javax.management.MalformedObjectNameException; import javax.management.ObjectName; @@ -52,13 +54,16 @@ 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 three drive that through a real pool: a + * publish, or nothing at all. The last five 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; 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> + * handle it releases at {@code close()}, at the container's + * {@code postDeregister()}, and at a registration the MBean server failed - + * which, running no {@code postDeregister()} at all, leaves + * {@code postRegister(false)} as the only hook that could ever release the + * pool's publication.</p> */ class TestJmxRegistration extends JdbcPoolBaseTest { @@ -86,6 +91,52 @@ class TestJmxRegistration extends JdbcPoolBaseTest { } } + /* + * A modeler-style wrapper of the kind a container registers in place of a + * DataSource. The DataSource is not itself a JMX compliant MBean, so the + * MBean server only ever sees a wrapper, and the wrapper forwards all four + * registration callbacks to the resource it manages - which is what the + * modeler's BaseModelMBean does. This is what lets the server, rather than + * a test, drive a DataSource through a registration it is about to fail. + */ + public interface ContainerMBean { + String getMarker(); + } + + public static class Container implements ContainerMBean, MBeanRegistration { + private final DataSource ds; + + Container(DataSource ds) { + this.ds = ds; + } + + @Override + public String getMarker() { + return "container"; + } + + @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(); + } + } + /* * A caching pool that has published itself and handed out one connection, * left with that connection back in the pool and its cache holding no @@ -425,4 +476,147 @@ class TestJmxRegistration extends JdbcPoolBaseTest { ds.postDeregister(); } } + + /* + * Everything the pool publishes for one DataSource registration: the pool's + * own MBean and anything published below it. The uid is carried down from + * the DataSource's name into every name derived from it, so it identifies + * the publications of this registration and nothing else. + */ + private static Set<ObjectName> publishedFor(MBeanServer server, ObjectName dsName) + throws Exception { + String uid = dsName.getKeyProperty("uid"); + Set<ObjectName> found = new HashSet<>(); + for (ObjectName name : poolMBeans(server)) { + if (uid.equals(name.getKeyProperty("uid"))) { + found.add(name); + } + } + return found; + } + + @Test + void aFailedDataSourceRegistrationUnpublishesThePoolMBeanPreRegisterPublished() + throws Exception { + MBeanServer server = server(); + DataSource ds = newDataSource(); + ds.getPoolProperties().setJmxEnabled(true); + ds.getPoolProperties().setInitialSize(0); + ds.createPool(); + + // the container is about to fail registering this DataSource: its name + // is held by another MBean, while the pool name derived from it is free + // and so preRegister() does publish the pool MBean before the server + // notices the conflict + ObjectName blocked = uniqueName("failed"); + ObjectName poolOname = ds.createObjectName(blocked); + server.registerMBean(new Probe("owner"), blocked); + try { + Assertions.assertFalse(server.isRegistered(poolOname)); + Assertions.assertThrows(InstanceAlreadyExistsException.class, + () -> server.registerMBean(new Container(ds), blocked)); + + // what the failed registration leaves behind. The server calls + // postRegister(FALSE) and no postDeregister() at all, and a + // container that could not register the DataSource will not close + // it either, so a pool MBean still published here is published for + // good: statistics and notifications of a pool nobody manages, + // pinning the pool, its credentials and its driver class loader + Assertions.assertFalse(server.isRegistered(poolOname), + "a DataSource whose registration failed must unpublish the pool MBean its" + + " preRegister() published: postRegister(FALSE) is the only hook" + + " that ever runs for a registration that failed, so anything left" + + " registered here outlives every chance to unregister it"); + Assertions.assertTrue(publishedFor(server, blocked).isEmpty(), + "nothing of a registration the server refused may stay published"); + Assertions.assertNull(handleOf(ds), + "a failed registration must leave no handle behind: it is what close()" + + " and postDeregister() unregister, and there is nothing left to" + + " unregister"); + Assertions.assertEquals("owner", server.getAttribute(blocked, "Marker"), + "the cleanup must have released only the pool's own publication"); + + // the container gives up on the DataSource and closes it: the + // close's own release must ride over the cleanup that already ran + ds.close(); + ds.postDeregister(); + Assertions.assertTrue(publishedFor(server, blocked).isEmpty(), + "closing a DataSource whose registration failed must publish nothing" + + " retroactively"); + Assertions.assertEquals("owner", server.getAttribute(blocked, "Marker"), + "a DataSource whose registration was cleaned up must not take the name" + + " the registration failed against when it is later closed"); + } finally { + if (server.isRegistered(blocked)) { + server.unregisterMBean(blocked); + } + ds.close(); + ds.postDeregister(); + // test hygiene: take away anything a broken build left published + for (ObjectName stale : publishedFor(server, blocked)) { + server.unregisterMBean(stale); + } + } + } + + @Test + void aSurvivingDataSourcePublishesItsPoolAtTheNextSuccessfulRegistration() + throws Exception { + MBeanServer server = server(); + DataSource ds = newDataSource(); + ds.getPoolProperties().setJmxEnabled(true); + ds.getPoolProperties().setInitialSize(0); + ds.createPool(); + + ObjectName blocked = uniqueName("rebound"); + ObjectName fresh = uniqueName("rebound-ok"); + server.registerMBean(new Probe("owner"), blocked); + try { + Assertions.assertThrows(InstanceAlreadyExistsException.class, + () -> server.registerMBean(new Container(ds), blocked)); + + // the failed attempt must not have made the DataSource unmanageable + // for good: the same DataSource registered under a free name + // publishes its pool, and a cleanup that also ran on a registration + // the server accepted would take that publication away again + server.registerMBean(new Container(ds), fresh); + ObjectName poolOname = ds.createObjectName(fresh); + Assertions.assertTrue(server.isRegistered(poolOname), + "a DataSource that survived a failed registration must publish its pool" + + " MBean at the next successful registration"); + Assertions.assertEquals(poolOname, handleOf(ds), + "the handle kept after a successful registration must be the name the pool" + + " MBean is published under"); + Assertions.assertEquals(0, + ((Number) server.getAttribute(poolOname, "NumIdle")).longValue(), + "the published pool must answer for the live pool"); + + // the container stops the DataSource: the same lifecycle releases + // the publication as always, and nothing survives it + server.unregisterMBean(fresh); + Assertions.assertFalse(server.isRegistered(poolOname), + "a DataSource unregistered by its container must take its pool MBean with" + + " it, after an earlier registration of the same DataSource" + + " failed"); + Assertions.assertNull(handleOf(ds), + "the deregistration must drop the handle as it always does"); + Assertions.assertTrue(publishedFor(server, fresh).isEmpty(), + "the pool must publish nothing once its registration has been released"); + } finally { + if (server.isRegistered(fresh)) { + server.unregisterMBean(fresh); + } + if (server.isRegistered(blocked)) { + server.unregisterMBean(blocked); + } + ds.close(); + ds.postDeregister(); + // test hygiene: the first, failed registration's publication loses + // its handle under the second one, so a broken build would leave it + // with no owner at all - take away anything that is still published + for (ObjectName stale : publishedFor(server, blocked)) { + server.unregisterMBean(stale); + } + } + } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
