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 0d3dc3ba5f8ebf1c72c06021d57ca33369c37e9a Author: opencode <[email protected]> AuthorDate: Sat Oct 31 09:35:45 2026 +0100 Release the pool MBean's name when the server takes it back The pool's JMX resource recorded its ObjectName in preRegister and kept it for the rest of the resource's life, but the MBean server also calls back when it takes the registration away - and both deregistration hooks here were no-ops. Since the notification source was made the registered name, that left a live pool notifying under an ObjectName that resolves to no MBean: a bare unregistration of the pool MBean while the DataSource keeps the pool running, and the window inside close() itself, where the pool MBean is released before the pool drains and nulls its JMX handle. A remote listener cannot correlate such notifications with anything registered, and getObjectName() kept answering a name the server no longer held. Use the callback the MBeanRegistration contract provides for exactly this cleanup: postDeregister() now releases the name, so the resource answers the registered name while it is registered and nothing once it is not - the same unregistered posture the class already defines for a pool that never got registered, whose pool-identifying string the notifications fall back to. Nothing publishes below the pool after that: both consumers of the name skip their registration on a null base, which is the desired outcome under a base that names nothing, and a later registration rewrites the field as before. --- .../tomcat/jdbc/pool/jmx/ConnectionPool.java | 36 +++++++---- .../jdbc/pool/jmx/TestJmxNotificationSource.java | 75 +++++++++++++++++++++- 2 files changed, 98 insertions(+), 13 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/ConnectionPool.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/ConnectionPool.java index a1bb0a8de2..41517fedfb 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/ConnectionPool.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/ConnectionPool.java @@ -87,9 +87,10 @@ public class ConnectionPool extends NotificationBroadcasterSupport implements Co }); /** - * the ObjectName of this pool. Written by the thread that registers the MBean and read by the pool's own threads, - * so it is volatile - as it already is on the {@link org.apache.tomcat.jdbc.pool.DataSource} that publishes this - * MBean - to make the registration visible to the threads that raise notifications. + * the ObjectName of this pool, or {@code null} while this MBean is not registered - before its registration and + * after its deregistration. Written by the threads that register and deregister the MBean and read by the pool's + * own threads, so it is volatile - as it already is on the {@link org.apache.tomcat.jdbc.pool.DataSource} that + * publishes this MBean - to make those writes visible to the threads that raise notifications. */ private volatile ObjectName oname = null; @@ -126,7 +127,8 @@ public class ConnectionPool extends NotificationBroadcasterSupport implements Co /** * Returns the JMX ObjectName under which this MBean is registered. * - * @return the JMX ObjectName, or {@code null} if not yet registered + * @return the JMX ObjectName, or {@code null} while this MBean is not registered - before its registration and + * after its deregistration */ public ObjectName getObjectName() { return oname; @@ -146,8 +148,19 @@ public class ConnectionPool extends NotificationBroadcasterSupport implements Co public void preDeregister() throws Exception { } + /** + * Drops the registration name. + * <p> + * The MBean server invokes this when the MBean is deregistered, and the pool it wraps may well keep running - a + * container that takes its DataSource back releases the pool MBean without closing the pool. From then on the name + * resolves to no MBean: notifications the pool still raises must not attribute themselves to it (see + * {@link #getNotificationSource()}, which falls back to naming the pool as a string), and no further MBean may be + * published below it. {@link #preRegister(MBeanServer, ObjectName)} writes the name again if this MBean is ever + * registered anew. + */ @Override public void postDeregister() { + this.oname = null; } // The notification types this MBean sends are declared as constants on @@ -253,17 +266,18 @@ public class ConnectionPool extends NotificationBroadcasterSupport implements Co * {@link Notification} marshals its source, so a listener outside this JVM - one reached through a JMX connector, * or one that hands the notification on to another process - only receives the notification at all if the source is * serializable. This MBean is not, and neither is the pool it wraps, so the source is the {@link ObjectName} the - * MBean is registered under, which names the pool and is serializable. A pool MBean that has never been registered - * has no such name: the source is then a string of the shape the name would have had. + * MBean is registered under, which names the pool and is serializable. A pool MBean that is not registered - one + * that has never been registered, or one the server has taken back while its pool keeps running - has no such + * name: the source is then a string of the shape the name would have had. * * @return the registered ObjectName, or a string naming the pool if the MBean is not registered */ protected Object getNotificationSource() { - // The registration handle is written by whichever thread registered the - // MBean; the field is volatile so that write becomes visible to the pool - // threads that read it here, and it is read once so that a single - // notification is sent with one source whatever happens to the field - // while the notification is on its way + // The registration handle is written by whichever thread registers or + // deregisters the MBean; the field is volatile so those writes become + // visible to the pool threads that read it here, and it is read once so + // that a single notification is sent with one source whatever happens + // to the field while the notification is on its way ObjectName name = oname; if (name != null) { return name; diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxNotificationSource.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxNotificationSource.java index 7e076489a7..b98829cb4b 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxNotificationSource.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxNotificationSource.java @@ -55,8 +55,14 @@ import org.apache.tomcat.jdbc.pool.DataSource; * MBean as its own source, the MBean server replaces the source with the name * the MBean is registered under before it reaches its own listeners. That * rewriting is also what a client behind a connector effectively sees, which is - * why the last test pins that the name the remote side reads does not change - * with this fix.</p> + * why the test that subscribes through the server pins that the name the remote + * side reads does not change with this fix.</p> + * <p> + * The last test covers the pool MBean that was registered and then taken away + * from the MBean server while the pool it wraps keeps running. A deregistration + * releases the name: the server no longer resolves it, so a notification raised + * afterwards must name the pool the way an unregistered MBean does rather than + * attribute itself to a name that names nothing.</p> * <p> * Delivery is asynchronous (on the pool's notification thread), so the tests * wait for the expected notifications to arrive with the bounded @@ -267,4 +273,69 @@ class TestJmxNotificationSource extends JdbcPoolBaseTest { ds.postDeregister(); } } + + @Test + void aNotificationOfADeregisteredPoolNamesThePoolNotTheReleasedName() throws Exception { + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + DataSource ds = newDataSource(); + ds.getPoolProperties().setJmxEnabled(true); + String poolName = "JmxSourceDeregistered-" + System.nanoTime(); + ds.getPoolProperties().setName(poolName); + ds.createPool(); + + ObjectName poolOname = registerPoolMBean(server, ds, "notification-deregistered"); + ConnectionPool jmxPool = ds.getPool().getJmxPool(); + List<Notification> received = new CopyOnWriteArrayList<>(); + NotificationListener listener = (notification, handback) -> received.add(notification); + try { + Assertions.assertNotNull(poolOname, "an enabled pool must publish its MBean"); + jmxPool.addListener(listener); + Assertions.assertTrue( + jmxPool.notify(ConnectionPool.POOL_EMPTY, "raised while registered"), + "the notification must be reported as sent"); + awaitUntil(() -> !received.isEmpty()); + Assertions.assertEquals(poolOname, received.get(0).getSource(), + "a notification raised while the MBean is registered must name the " + + "registration - the deregistration below must change the source " + + "because the name is gone, not because it was never there"); + + // The container takes its DataSource back: postDeregister() + // unregisters the pool's MBean from the server, which runs the + // MBean's own deregistration callbacks - but it does not close the + // pool, and the pool goes on notifying whatever is still listening. + ds.postDeregister(); + Assertions.assertFalse(server.isRegistered(poolOname), + "the container's deregistration must have taken the pool MBean away"); + Assertions.assertNull(jmxPool.getObjectName(), + "an MBean the server has deregistered must not keep answering with the " + + "name it no longer holds: that name resolves to no MBean, and " + + "per-connection MBeans would still be published below a base that " + + "names nothing"); + + Assertions.assertTrue( + jmxPool.notify(ConnectionPool.POOL_EMPTY, "raised after the deregistration"), + "a pool whose MBean was deregistered while the pool kept running still " + + "reports its conditions"); + awaitUntil(() -> received.size() >= 2); + Notification raised = received.get(1); + Assertions.assertEquals("[POOL EMPTY] raised after the deregistration", + raised.getMessage()); + Assertions.assertTrue(raised.getSource() instanceof String, + "the source of a notification raised after the deregistration must be the " + + "string naming the pool, as it is for an MBean that was never " + + "registered, was " + (raised.getSource() == null ? "null" + : raised.getSource().getClass().getName())); + String source = (String) raised.getSource(); + Assertions.assertEquals("tomcat.jdbc:type=ConnectionPool,name=" + poolName, source, + "a remote listener cannot correlate a notification with a deregistered " + + "name, so the source must at least identify the pool that " + + "reported the condition"); + Assertions.assertEquals(source, roundTrip(raised).getSource(), + "that source must survive the delivery a serialized client sees"); + } finally { + jmxPool.removeListener(listener); + ds.close(); + ds.postDeregister(); + } + } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
