This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 10.1.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 70a35f01b10b7bd59e02c7704d1f641f291e5f5d Author: opencode <[email protected]> AuthorDate: Sat Oct 31 07:48:41 2026 +0100 Publish the pool MBean's name to the threads that stamp it The pool's JMX resource records the ObjectName the MBean server hands it in preRegister, which runs on whichever thread performed the registration - in production, a container thread - while the name is read without any synchronization by ordinary pool worker threads: every notify() takes the notification source from it on the borrow/return/abandon/cleaner threads, and getObjectName() is read from connection and cleaner threads too. Nothing on those read paths gives a happens-before edge to the registration write: the volatile jmxPool reference on the pool side is written when the MBean is created, before the registration write it would have to carry, so a pool thread could keep seeing the stale null and stamp notifications with the string fallback long after registration - the pre-existing single read into a local only stopped a field change mid-notification, not stale visibility. Make the field volatile, the same edge the sibling DataSource resource has used for its own registration handle since upstream. A test starts its stamping worker before the registration happens - so no thread start or join hands the write over and the worker's read is the racing one - and pins that the name published by preRegister becomes visible to it and lands in the notification source, not the fallback. --- .../tomcat/jdbc/pool/jmx/ConnectionPool.java | 12 ++- .../tomcat/jdbc/pool/jmx/TestJmxRegistration.java | 87 ++++++++++++++++++++++ 2 files changed, 95 insertions(+), 4 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 4b74f4b1b6..a1bb0a8de2 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,11 @@ public class ConnectionPool extends NotificationBroadcasterSupport implements Co }); /** - * the ObjectName of this pool. + * 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. */ - private ObjectName oname = null; + private volatile ObjectName oname = null; /** * Creates a new JMX wrapper for the given connection pool. @@ -258,8 +260,10 @@ public class ConnectionPool extends NotificationBroadcasterSupport implements Co */ protected Object getNotificationSource() { // The registration handle is written by whichever thread registered the - // MBean; read it once so a single notification is sent with one source - // whatever happens to the field while the notification is on its way + // 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 ObjectName name = oname; if (name != null) { return name; 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 58c2d48798..da789cb1d9 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 @@ -21,11 +21,17 @@ import java.lang.reflect.Field; import java.sql.Connection; import java.util.HashSet; import java.util.Set; +import java.util.concurrent.BlockingQueue; +import java.util.concurrent.LinkedBlockingQueue; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; import javax.management.InstanceAlreadyExistsException; import javax.management.MBeanRegistration; import javax.management.MBeanServer; import javax.management.MalformedObjectNameException; +import javax.management.Notification; +import javax.management.NotificationListener; import javax.management.ObjectName; import org.junit.jupiter.api.Assertions; @@ -64,6 +70,12 @@ import org.apache.tomcat.jdbc.pool.interceptor.StatementCache; * which, running no {@code postDeregister()} at all, leaves * {@code postRegister(false)} as the only hook that could ever release the * pool's publication.</p> + * <p> + * The last test steps from what the helper reports to what the pool MBean + * itself remembers of its registration: the container writes that name on its + * own thread, while the pool raises its notifications on the threads that + * borrow, return and clean up connections, so the write has to become visible + * to those threads before their notifications can carry the published name.</p> */ class TestJmxRegistration extends JdbcPoolBaseTest { @@ -619,4 +631,79 @@ class TestJmxRegistration extends JdbcPoolBaseTest { } } } + + @Test + void aPoolThreadRaisesItsNotificationUnderTheNameTheRegistrationPublished() throws Exception { + MBeanServer server = server(); + DataSource ds = newDataSource(); + ds.getPoolProperties().setJmxEnabled(true); + ds.getPoolProperties().setInitialSize(0); + ds.createPool(); + ConnectionPool jmx = ds.getPool().getJmxPool(); + Assertions.assertNotNull(jmx, "an enabled pool has a JMX wrapper"); + Assertions.assertNull(jmx.getObjectName(), "this pool is not registered yet"); + + BlockingQueue<Notification> delivered = new LinkedBlockingQueue<>(); + NotificationListener listener = (n, handback) -> delivered.offer(n); + jmx.addListener(listener); + + // A thread of the pool's kind, already running while the container + // publishes the pool: the pool raises its notifications on exactly + // such threads - the ones that borrow, return and clean up connections + // - while the registration handle is written on the thread the MBean + // server called preRegister() on. Nothing here may order the read + // after the write by thread handoff, since start() and join() alone + // would give the write a happens-before edge and hide the visibility + // problem this pins; the worker therefore waits by re-reading the + // handle, which on a correct build stops as soon as the registration + // lands - a bound on the wait, not a stress attempt. + AtomicReference<ObjectName> observed = new AtomicReference<>(); + Thread poolThread = new Thread(() -> { + long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(30); + ObjectName name; + while ((name = jmx.getObjectName()) == null && System.nanoTime() < deadline) { + Thread.yield(); + } + if (name != null) { + jmx.notify(ConnectionPool.POOL_EMPTY, + "reported by a pool thread once it saw the registration"); + observed.set(name); + } + }, "test-pool-thread-before-registration"); + poolThread.setDaemon(true); + poolThread.start(); + + ObjectName dsName = uniqueName("notify-thread"); + ObjectName poolOname = null; + try { + // the container's registration, on this test's thread: the MBean + // server calls the pool wrapper's preRegister() here synchronously + ds.preRegister(server, dsName); + poolOname = ds.createObjectName(dsName); + + poolThread.join(TimeUnit.SECONDS.toMillis(60)); + Assertions.assertEquals(poolOname, observed.get(), + "a pool thread that was already running when the container registered the" + + " pool must see the registered name: as long as the registration" + + " never becomes visible to a thread, every notification it raises" + + " keeps carrying the string fallback of an unregistered pool, even" + + " long after the MBean server published the name"); + + Notification n = delivered.poll(30, TimeUnit.SECONDS); + Assertions.assertNotNull(n, "the pool thread handed its notification off for delivery"); + Assertions.assertEquals(poolOname, n.getSource(), + "a notification raised on a pool thread must be stamped with the name the" + + " MBean server published the pool under, not with the fallback of" + + " a pool that was never registered"); + } finally { + jmx.removeListener(listener); + ds.close(); + ds.postDeregister(); + // test hygiene: the publication is released by the close above; + // take away anything a regression would have left published + for (ObjectName stale : publishedFor(server, dsName)) { + server.unregisterMBean(stale); + } + } + } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
