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 fac78d52898147fcd1eb5c08734d213f2f831bb6 Author: opencode <[email protected]> AuthorDate: Tue Oct 27 19:15:08 2026 +0100 Keep same-named pools' JMX registration until the last pool closes Closing one of several live pools configured with the same name ran an unconditional deregisterJmx(), unregistering the MBean published for the name and detaching the per-name notification broadcaster even though a surviving same-named pool still reported through them. The survivor's next borrow republished the MBean, but JMX client subscriptions added through the old registration could never be restored, so those clients silently stopped receiving slow/failed query notifications for good. SlowQueryReportJmx.poolClosed now releases the MBean and the broadcaster only when its own close freed the per-name statistics, i.e. when it deregistered the last live pool under the name. The decision is taken by SlowQueryReport.releaseStatistics() inside a compute on the name's live pool set, atomically against the concurrent removals of other same-named pools: across any number of concurrent closes and the interceptor definitions of one close, exactly one caller ever sees the release signal. A check outside the removal could not do this - two same-named pools closing concurrently could each see the other still live, both keep the registration, and leave the MBean, the registry entry and the broadcaster registered for ever with no live pool left, which a deterministic emulation of that interleaving reproduced. The registered MBean may delegate to an interceptor instance of an already closed pool, which is inert: its statistics and notifier resolve by pool name on every use. The unconditional deregisterJmx() remains for explicit slot release. The class javadoc now documents the name-scoped registration lifecycle and that a client must re-attach listeners after a full teardown, since JMX cannot carry a subscription across an unregistration. TestSlowQueryJmxNotifications gains a same-named-pool lifecycle test (fails without the ownership check), a concurrent-close test pinning the exactly-once, leak-free release of the name, and a pin for the javadoc- recommended two-thresholds configuration, whose two interceptor instances were confirmed to both report through the single MBean. --- .../jdbc/pool/interceptor/SlowQueryReport.java | 40 +++- .../jdbc/pool/interceptor/SlowQueryReportJmx.java | 45 +++- .../interceptor/TestSlowQueryJmxNotifications.java | 239 +++++++++++++++++++++ 3 files changed, 318 insertions(+), 6 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java index 46d6ebcaea..718502296a 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java @@ -247,11 +247,47 @@ public class SlowQueryReport extends AbstractQueryReport { */ @Override public void poolClosed(ConnectionPool pool) { - perPoolStats.computeIfPresent(pool.getName(), - (name, existing) -> existing.removeLivePool(pool) ? null : existing); + lastPoolOfNameClosed = releaseStatistics(pool); super.poolClosed(pool); } + /** + * Whether the {@link #poolClosed(ConnectionPool)} delivered to this + * instance deregistered the closing pool as the last live pool under + * its name, meaning everything else registered for that name - the + * interceptor's MBean and broadcaster, for instance - can and must + * now be released with it. The deciding {@link + * #releaseStatistics(ConnectionPool)} call is atomic, so across + * concurrent closes of same-named pools and across the interceptor + * definitions of one close exactly one caller ever sees {@code true}. + */ + protected boolean lastPoolOfNameClosed = false; + + /** + * Deregisters a closed pool from the live pools that the + * per-name statistics are held for, and releases the statistics + * once no live pool is registered under the name any more. + * <p> + * The decision is taken inside a {@code compute} on the name, which + * atomises it against the concurrent removals of other same-named + * pools: exactly one of any number of concurrent {@code poolClosed} + * deliveries sees {@code true} returned, so the per-name entries are + * never released twice and never left behind.</p> + * + * @param pool the connection pool that has been closed + * @return {@code true} if this call removed the last live pool + * registered under the name + */ + protected static boolean releaseStatistics(ConnectionPool pool) { + boolean[] last = new boolean[1]; + perPoolStats.computeIfPresent(pool.getName(), + (name, existing) -> { + last[0] = existing.removeLivePool(pool); + return last[0] ? null : existing; + }); + return last[0]; + } + /** * Retrieves or creates a QueryStats entry for the given SQL string. * diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReportJmx.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReportJmx.java index ca184fd995..b7c04cf79c 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReportJmx.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReportJmx.java @@ -47,7 +47,21 @@ import org.apache.tomcat.jdbc.pool.jmx.JmxUtil; * JMX-enabled slow query report interceptor. * Publishes slow and failed query statistics to JMX and provides * notifications when slow or failed queries are detected. - * + * <p> + * The MBean and the notification broadcaster are published for the pool + * <em>name</em>: the first interceptor instance to borrow a connection + * registers them, and every other instance sharing the name - the other + * connections of that pool, the second interceptor of the recommended + * two-thresholds configuration, or a second pool configured with the + * same name - reports through them. They stay published until the last + * live pool registered under the name closes; closing one of several + * same-named pools leaves the registration with the survivors.</p> + * <p> + * A JMX client's listener subscription belongs to the registration it + * was added through. JMX cannot carry a subscription across an + * unregistration, so after the last pool of a name has closed, a client + * must attach its listeners again to receive notifications from a pool + * started under that name anew.</p> */ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationEmitter, SlowQueryReportJmxMBean{ /** @@ -224,14 +238,32 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE /** - * Called when the connection pool is closed; deregisters JMX. + * Called when the connection pool is closed; releases the JMX + * registration of the pool's name - the MBean and the broadcaster its + * clients' listeners are attached to - only once that pool was the + * last live pool registered under the name. Both belong to the name + * and are shared by every live pool of that name, so taking them down + * while a survivor is still running would orphan the survivor's + * monitoring; releasing them on every close but a decided last one + * would unregister a shared registration that other closing or live + * pools still rely on. Whether this close is the last one is decided + * atomically by {@link SlowQueryReport#releaseStatistics(ConnectionPool)} + * on the way through {@code super.poolClosed}, so concurrent closes + * of same-named pools cannot all see a survivor and leave the name + * registered for ever. * @param pool the connection pool being closed */ @Override public void poolClosed(ConnectionPool pool) { this.poolName = pool.getName(); - deregisterJmx(); super.poolClosed(pool); + if (lastPoolOfNameClosed) { + // the statistics of the name were freed by this very close, + // so no live pool is registered under it any more - and the + // registered MBean, whose delegate may be an interceptor + // instance of an already closed pool, goes with them + deregisterJmx(); + } } /** @@ -386,7 +418,12 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE } /** - * Deregisters this interceptor from JMX. + * Releases the MBean registration and the shared broadcaster held + * for the current pool name unconditionally - even when the pool + * name is still owned by live pools. Normal pool shutdown releases + * through {@code super.poolClosed} plus {@link #lastPoolOfNameClosed} + * in {@link #poolClosed(ConnectionPool)}, which keeps the + * registration for any surviving same-named pool. */ protected void deregisterJmx() { try { diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryJmxNotifications.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryJmxNotifications.java index 9e61ef6635..b39eac77b7 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryJmxNotifications.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryJmxNotifications.java @@ -17,12 +17,16 @@ package org.apache.tomcat.jdbc.pool.interceptor; import java.lang.management.ManagementFactory; +import java.lang.reflect.Proxy; import java.sql.Connection; +import java.sql.SQLException; import java.sql.Statement; import java.util.List; import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.CountDownLatch; import java.util.stream.Collectors; +import javax.management.InstanceNotFoundException; import javax.management.ListenerNotFoundException; import javax.management.MBeanServer; import javax.management.Notification; @@ -34,6 +38,7 @@ import org.junit.jupiter.api.Test; import org.apache.tomcat.jdbc.JdbcPoolBaseTest; import org.apache.tomcat.jdbc.pool.DataSource; +import org.apache.tomcat.jdbc.pool.JdbcInterceptor; /** * JMX notifications of the {@code SlowQueryReportJmx} interceptor in @@ -49,6 +54,12 @@ import org.apache.tomcat.jdbc.pool.DataSource; * instance owns the MBean must leave the MBean and its listeners live, * and a released registration slot must be taken over by the next * borrow.</p> + * <p> + * The same registry is shared by live pools configured with the same + * name: closing one of those pools must not take the registered MBean - + * and with it the broadcaster its clients' listeners are attached to - + * away from the pool that is still running, while concurrent closes of + * such pools must still release the name exactly once.</p> */ class TestSlowQueryJmxNotifications extends JdbcPoolBaseTest { @@ -210,4 +221,232 @@ class TestSlowQueryJmxNotifications extends JdbcPoolBaseTest { } } } + + /* A live pool that reports every query through the JMX interceptor under the given name. */ + private DataSource reportingPoolNamed(String name) { + DataSource ds = newDataSource(); // jmxEnabled=false removes the + // pools' own ObjectName clash; the interceptor registers by name + ds.getPoolProperties().setName(name); + ds.setJdbcInterceptors("org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReportJmx" + + "(notifyPool=false,threshold=-1)"); + ds.getPoolProperties().setInitialSize(0); + return ds; + } + + @Test + void closingOneSameNamedPoolKeepsTheSurvivorsMBeanAndListenerLive() throws Exception { + String poolName = "JmxDuplicate-" + System.nanoTime(); + DataSource ownerPool = reportingPoolNamed(poolName); + DataSource survivorPool = reportingPoolNamed(poolName); + + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + ObjectName oname = new ObjectName("tomcat.jdbc:type=" + + SlowQueryReportJmx.class.getName() + ",name=" + poolName); + List<Notification> received = new CopyOnWriteArrayList<>(); + NotificationListener listener = (notification, handback) -> received.add(notification); + + // the owner pool's first borrow wins the per-name registry slot; + // the survivor's interceptor instance shares the registered MBean + // and its broadcaster + Connection ownerConnection = ownerPool.getConnection(); + Connection survivorConnection = survivorPool.getConnection(); + SlowQueryReportJmx owningInstance = interceptorOf(ownerConnection, SlowQueryReportJmx.class); + try { + Assertions.assertSame(SlowQueryReportJmx.mbeans.get(poolName), owningInstance, + "the first pool's interceptor instance must own the MBean registration"); + server.addNotificationListener(oname, listener, null, null); + + try (Statement st = survivorConnection.createStatement()) { + st.execute("SELECT 1"); + } + Assertions.assertTrue(received.stream().anyMatch(n -> "SELECT 1".equals(n.getMessage())), + "while both pools are live, the sharing pool's queries must reach the " + + "listener attached to the owning pool's MBean, received: " + received); + + // close the pool whose instance happens to own the registration, + // while the survivor keeps running + ownerConnection.close(); + ownerConnection = null; + ownerPool.close(); + awaitSessions(1); // the owner pool's physical session is really gone + + try (Statement st = survivorConnection.createStatement()) { + st.execute("SELECT 2"); + } + Assertions.assertTrue(received.stream().anyMatch(n -> "SELECT 2".equals(n.getMessage())), + "a query of the surviving pool must still reach the listener that " + + "subscribed before the other same-named pool closed, received: " + + received); + Assertions.assertTrue(server.isRegistered(oname), + "closing one of two live pools of the same name must not unregister the " + + "MBean the surviving pool still reports through"); + Assertions.assertSame(SlowQueryReportJmx.mbeans.get(poolName), owningInstance, + "the registration must stay in place until the last pool of the name closes"); + + survivorPool.close(); + Assertions.assertFalse(server.isRegistered(oname), + "closing the last live pool of the name must release the MBean"); + } finally { + if (ownerConnection != null) { + try { + ownerConnection.close(); + } catch (SQLException ignore) { + // pool already closed + } + } + try { + survivorConnection.close(); + } catch (SQLException ignore) { + // pool already closed + } + try { + server.removeNotificationListener(oname, listener); + } catch (ListenerNotFoundException | InstanceNotFoundException ignore) { + // the teardown under test may already have dropped the binding + } + } + } + + @Test + void bothInterceptorInstancesOfTheTwoThresholdsConfigurationNotifyThroughTheMBean() + throws Exception { + String poolName = "JmxTwoThresholds-" + System.nanoTime(); + DataSource ds = newDataSource(); + ds.setName(poolName); + // the configuration the class javadoc recommends: the interceptor + // twice on one pool; each connection's chain then holds two + // instances of it, only one of which can win the registration + ds.setJdbcInterceptors("org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReportJmx" + + "(notifyPool=false,threshold=-1);" + + "org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReportJmx" + + "(notifyPool=false,threshold=-1)"); + ds.getPoolProperties().setInitialSize(0); + + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + ObjectName oname = new ObjectName("tomcat.jdbc:type=" + + SlowQueryReportJmx.class.getName() + ",name=" + poolName); + List<Notification> received = new CopyOnWriteArrayList<>(); + NotificationListener listener = (notification, handback) -> received.add(notification); + + try (Connection con = ds.getConnection(); + Statement st = con.createStatement()) { + Assertions.assertTrue(server.isRegistered(oname), + "the interceptor MBean must be registered once the pool is in use"); + server.addNotificationListener(oname, listener, null, null); + + // the registration must be owned by one of the two instances + // of this very chain - the other shares its broadcaster + SlowQueryReportJmxMBean owner = SlowQueryReportJmx.mbeans.get(poolName); + int chainInstances = 0; + boolean ownerIsFromThisChain = false; + JdbcInterceptor interceptor = (JdbcInterceptor) Proxy.getInvocationHandler(con); + while (interceptor != null) { + if (interceptor instanceof SlowQueryReportJmx) { + chainInstances++; + ownerIsFromThisChain |= (interceptor == owner); + } + interceptor = interceptor.getNext(); + } + Assertions.assertEquals(2, chainInstances, + "the chain must hold both configured interceptor instances"); + Assertions.assertTrue(ownerIsFromThisChain, + "the registration must be owned by one of the pool's own instances"); + + st.execute("SELECT 1"); + } + + // both instances of the chain see the one query; both must notify + // through the single registered MBean - none of them into a + // broadcaster of its own + List<String> messages = received.stream().map(Notification::getMessage) + .collect(Collectors.toList()); + Assertions.assertEquals(2, messages.size(), + "both interceptor instances of the two-thresholds configuration must " + + "report through the registered MBean, received: " + messages); + Assertions.assertTrue(messages.stream().allMatch(m -> m.contains("SELECT 1")), + "both notifications are for the one query, received: " + messages); + + ds.close(); + Assertions.assertFalse(server.isRegistered(oname), + "closing the only pool of the name must release the MBean"); + } + + @Test + void concurrentClosesOfSameNamedPoolsReleaseTheNameExactlyOnce() throws Exception { + String poolName = "JmxRace-" + System.nanoTime(); + DataSource first = reportingPoolNamed(poolName); + DataSource second = reportingPoolNamed(poolName); + DataSource third = reportingPoolNamed(poolName); + + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + ObjectName oname = new ObjectName("tomcat.jdbc:type=" + + SlowQueryReportJmx.class.getName() + ",name=" + poolName); + + Connection one = first.getConnection(); + Connection two = second.getConnection(); + Connection three = third.getConnection(); + Assertions.assertTrue(server.isRegistered(oname), + "the MBean must be registered while the same-named pools run"); + // return every connection first, so the concurrent closes race only + // on the per-name bookkeeping, not on pooled connections + one.close(); + two.close(); + three.close(); + + // a start gate releases all three closes together: each closing + // thread may well see one of the others still live when it checks, + // so the release of the name must be decided atomically, not by + // a check that every one of them can lose against the others + CountDownLatch startGate = new CountDownLatch(1); + final DataSource[] pools = {first, second, third}; + Throwable[] failures = new Throwable[pools.length]; + Thread[] closers = new Thread[pools.length]; + for (int i = 0; i < pools.length; i++) { + final DataSource pool = pools[i]; + final int slot = i; + closers[i] = new Thread(() -> { + try { + startGate.await(); + pool.close(); + } catch (Throwable x) { + failures[slot] = x; + } + }, "same-named-pool-closer-" + i); + closers[i].start(); + } + startGate.countDown(); + for (Thread closer : closers) { + closer.join(AWAIT_TIMEOUT_MS); + Assertions.assertFalse(closer.isAlive(), + "no close may be left waiting on the name - the release " + + "must be race-free, not deadlock- or livelock-prone"); + } + for (Throwable failure : failures) { + Assertions.assertNull(failure, + "concurrent close must not fail: " + failure); + } + + awaitSessions(0); // every physical session of every pool is really gone + Assertions.assertFalse(server.isRegistered(oname), + "the last of the concurrent closes must release the MBean, even where " + + "every closing pool saw another live pool when it checked"); + Assertions.assertNull(SlowQueryReportJmx.mbeans.get(poolName), + "the per-name MBean registry entry must be released by one of the closes"); + Assertions.assertNull(SlowQueryReportJmx.notifiers.get(poolName), + "the per-name broadcaster entry must be released by one of the closes"); + Assertions.assertNull(SlowQueryReport.getPoolStats(poolName), + "the statistics must be released with the last live pool of the name"); + + // exactly-once release means the name's slots were really freed, not + // merely flagged: a pool restarted under the freed name must be able + // to re-register, and must release the name again when it closes + DataSource restarted = reportingPoolNamed(poolName); + try (Connection borrowed = restarted.getConnection()) { + Assertions.assertTrue(server.isRegistered(oname), + "a pool started under the concurrently freed name must re-register the MBean"); + } + restarted.close(); + Assertions.assertFalse(server.isRegistered(oname), + "closing the restarted pool must release the registration again"); + } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
