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 1a09ae8f787eaba075fe35ed2f1018db38dee093 Author: opencode <[email protected]> AuthorDate: Tue Oct 27 19:33:12 2026 +0100 Retry slow query JMX registration when the MBeanServer refuses it SlowQueryReportJmx.registerJmx() took the per-pool-name mbeans registry slot before attempting the publish and then delegated the publish to JmxUtil.registerJmx(), which swallows every exception and returns a name regardless. A publish the MBeanServer refused - name held by a stale or foreign MBean (InstanceAlreadyExistsException), an MBeanRegistrationException/NotCompliantMBeanException from a subclass, a SecurityException - therefore left the registry claiming an MBean that was never published. Since the slot is the only registration gate, every later borrow skipped re-registration for the life of the pool, with no retry even after the obstacle was gone: monitoring silently dead while the registry said otherwise. Register through the platform MBeanServer directly (same server and name JmxUtil used, but the failure is now observable here without touching JmxUtil), and on failure free the slot with a conditional remove - so a slot a later owner took over is never clobbered - and let every later borrow retry. InstanceAlreadyExistsException is logged as a retryable warning, other refusals as errors. Because retries now happen per borrow, only the first refusal of a name (or a new failing ObjectName under the name) is logged with its exception; identical repeats log at debug without a stack trace, so a persistent refusal (a foreign MBean on the name, a security restriction) does not log its stack trace once per borrow. The marker for that throttle is dropped when a registration under the name succeeds and when the name's release runs through deregisterJmx(), so a later failure cycle reports loudly again. deregisterJmx() likewise releases the name's broadcaster even when the slot was empty: a pool whose registration was refused still reported through it, and neither the broadcaster nor the marker may outlive the name. TestSlowQueryJmxNotifications gains two tests: one pins that a refused publish frees the registry slot so a later borrow publishes once the obstacle is gone (fails pre-fix with the slot latched on the never- published instance), and one pins the persistent-refusal lifecycle - repeated refusals keep the slot free and retryable, and closing the never-registered pool releases the slot, the broadcaster and the throttle marker, so a fresh same-named pool starts clean. --- .../jdbc/pool/interceptor/SlowQueryReportJmx.java | 80 +++++++++- .../interceptor/TestSlowQueryJmxNotifications.java | 173 +++++++++++++++++++++ 2 files changed, 251 insertions(+), 2 deletions(-) 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 b7c04cf79c..21030aa65f 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 @@ -16,12 +16,14 @@ */ package org.apache.tomcat.jdbc.pool.interceptor; +import java.lang.management.ManagementFactory; import java.util.Iterator; import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicLong; +import javax.management.InstanceAlreadyExistsException; import javax.management.ListenerNotFoundException; import javax.management.MBeanNotificationInfo; import javax.management.MalformedObjectNameException; @@ -108,6 +110,20 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE protected static final ConcurrentHashMap<String,NotificationBroadcasterSupport> notifiers = new ConcurrentHashMap<>(); + /** + * Registry of the first registration failure seen for each pool name, + * holding the ObjectName that failed. Registration is retried by every + * borrow of such a pool, but only the first failure of a name is + * logged with its exception, so a persistent refusal - a foreign MBean + * holding the name, a security restriction - is not reported with a + * stack trace once per borrow. The marker is dropped when a + * registration under the name succeeds and when the name's release + * runs through {@link #deregisterJmx()}, so a later failure cycle is + * reported loudly once again. + */ + protected static final ConcurrentHashMap<String,ObjectName> registrationFailuresLogged = + new ConcurrentHashMap<>(); + //==============================JMX STUFF======================== /** @@ -424,14 +440,22 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE * through {@code super.poolClosed} plus {@link #lastPoolOfNameClosed} * in {@link #poolClosed(ConnectionPool)}, which keeps the * registration for any surviving same-named pool. + * <p> + * The broadcaster and the registration-failure marker belong to the + * name as much as the MBean does, so the release drops them even + * when this pool never held the registry slot: a pool whose every + * registration attempt was refused still reported through the + * broadcaster, and its throttle marker must not outlive the name and + * hush the first failure of a later failure cycle under the name.</p> */ protected void deregisterJmx() { try { if (mbeans.remove(poolName)!=null) { ObjectName oname = getObjectName(getClass(),poolName); JmxUtil.unregisterJmx(oname); - notifiers.remove(poolName); } + notifiers.remove(poolName); + registrationFailuresLogged.remove(poolName); } catch (MalformedObjectNameException | RuntimeOperationsException e) { log.warn("Jmx deregistration failed.",e); } @@ -459,6 +483,21 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE /** * Registers this interceptor with JMX. + * <p> + * The registration is attempted under the protection of the per-name + * registry slot: the slot is taken with {@code putIfAbsent} so only + * one instance per pool name registers, and it is released again when + * the MBeanServer refuses the MBean. A failed publish must not keep + * the slot, because the slot is the only registration gate: every + * later borrow would find it occupied and skip the registration, so + * the pool would claim an MBean for its whole life that was never + * published - and would not re-register even after whatever made the + * MBeanServer refuse it is gone. Releasing the slot lets a later + * borrow retry; {@code JmxUtil.registerJmx} cannot be used for the + * attempt because it swallows every exception and returns a name + * whatever happened, so the failure of its own call is unobservable + * to the caller. Only the first refusal of a name is logged with its + * exception, so a persistent refusal is not reported once per borrow. */ protected void registerJmx() { try { @@ -468,7 +507,44 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE } else if (getCompositeType()!=null) { ObjectName oname = getObjectName(getClass(),poolName); if (mbeans.putIfAbsent(poolName, this)==null) { - JmxUtil.registerJmx(oname, null, this); + try { + ManagementFactory.getPlatformMBeanServer().registerMBean(this, oname); + // a registration under the name works again - drop + // the marker so a later failure cycle reports loudly + registrationFailuresLogged.remove(poolName); + } catch (Exception x) { + mbeans.remove(poolName, this); + if (oname.equals(registrationFailuresLogged.putIfAbsent(poolName, oname))) { + // the loud report went out with the first + // refusal of this name; repeats ride along at + // debug, without a stack trace, so a persistent + // refusal stays quiet under borrow load + if (log.isDebugEnabled()) { + log.debug("Jmx registration of [" + oname + + "] failed again: " + x); + } + } else { + // first refusal for the name, or the name now + // fails under a different ObjectName: report it + // loudly and remember to throttle the repeats + registrationFailuresLogged.put(poolName, oname); + if (x instanceof InstanceAlreadyExistsException) { + // retryable: the name may be held by a stale + // or foreign MBean that a later borrow finds + // gone + log.warn("Jmx registration of [" + oname + + "] failed: the name is already in use by another MBean." + + " The registration is retried by later borrows;" + + " repeated failures are logged at debug level.", x); + } else { + log.error("Jmx registration of [" + oname + + "] failed, no JMX data will be exposed for the query stats" + + " until a registration succeeds. The registration is" + + " retried by later borrows; repeated failures are" + + " logged at debug level.", x); + } + } + } } } else { log.warn(SlowQueryReport.class.getName()+ "- No JMX support, composite type was not found."); 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 b39eac77b7..e64b0a79b7 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 @@ -60,6 +60,12 @@ import org.apache.tomcat.jdbc.pool.JdbcInterceptor; * 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> + * <p> + * The registry also holds the pool's claim to a registration, so a + * publish the MBeanServer refused must not keep the claim: every later + * borrow retries, and closing a pool that never got published must + * release the name's broadcaster and failure-report bookkeeping along + * with the slot.</p> */ class TestSlowQueryJmxNotifications extends JdbcPoolBaseTest { @@ -371,6 +377,173 @@ class TestSlowQueryJmxNotifications extends JdbcPoolBaseTest { "closing the only pool of the name must release the MBean"); } + /* + * A foreign MBean interface and resource. Registered under the very + * ObjectName the interceptor computes, it makes the interceptor's + * publish attempt fail with InstanceAlreadyExistsException even + * though the per-name registry let it through - the least invasive + * deterministic way to make registerMBean refuse the MBean. + */ + public interface ObstacleMBean { + String getHeldBy(); + } + + /* The resource behind {@link ObstacleMBean}. */ + public static class Obstacle implements ObstacleMBean { + @Override + public String getHeldBy() { + return "registration-obstacle"; + } + } + + @Test + void failedRegistrationFreesTheSlotSoALaterBorrowCanStillPublish() throws Exception { + String poolName = "JmxRegFail-" + System.nanoTime(); + DataSource ds = newDataSource(); + ds.setName(poolName); + ds.setJdbcInterceptors("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); + + Obstacle obstacle = new Obstacle(); + server.registerMBean(obstacle, oname); + try { + try (Connection first = ds.getConnection()) { + // The borrow tried to publish and the MBeanServer refused + // it: the name was taken outside the per-name registry, so + // registry and MBeanServer now disagree. + Assertions.assertEquals("registration-obstacle", + server.getAttribute(oname, "HeldBy"), + "the interceptor's publish must have collided with the foreign MBean"); + Assertions.assertNull(SlowQueryReportJmx.mbeans.get(poolName), + "a registration that the MBeanServer refused must not leave the per-name " + + "registry claiming an MBean that was never published - the slot is " + + "the only registration gate, and every later borrow would skip " + + "re-registration behind it for the life of the pool"); + + // With the obstacle gone, a later borrow must still get the + // MBean published: the refused attempt must not have latched + // the registration for the life of the pool. + server.unregisterMBean(oname); + try (Connection next = ds.getConnection()) { + Assertions.assertTrue(server.isRegistered(oname), + "the borrow after the obstacle was removed must publish the MBean"); + // the attribute of the interceptor's MBean interface, + // answered by the registered resource: not the obstacle, + // which exposes no such attribute + Assertions.assertDoesNotThrow(() -> server.getAttribute(oname, "SlowQueriesCD"), + "the published MBean must be the interceptor's, answering its statistics query"); + Assertions.assertSame(SlowQueryReportJmx.mbeans.get(poolName), + interceptorOf(next, SlowQueryReportJmx.class), + "the retrying borrow's interceptor instance must own the slot"); + } + } + ds.close(); + Assertions.assertFalse(server.isRegistered(oname), + "closing the pool must release the registration that the retry published"); + Assertions.assertNull(SlowQueryReportJmx.mbeans.get(poolName), + "closing the pool must release the per-name registry slot"); + } finally { + // remove the obstacle if the test never got as far as removing + // it itself; on the green path nothing is registered any more + try { + server.unregisterMBean(oname); + } catch (InstanceNotFoundException ignore) { + // nothing left to clean up under this name + } + } + } + + @Test + void persistentRegistrationRefusalKeepsRetryingAndCleansUpOnClose() throws Exception { + String poolName = "JmxRegRefuse-" + System.nanoTime(); + DataSource refusingPool = reportingPoolNamed(poolName); + + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + ObjectName oname = new ObjectName("tomcat.jdbc:type=" + + SlowQueryReportJmx.class.getName() + ",name=" + poolName); + + Obstacle obstacle = new Obstacle(); + server.registerMBean(obstacle, oname); + try { + // the first refused borrow: the slot must be freed again, and + // the failure marked as reported - the loud report of this name + // (warn/error) has gone out, its repeats ride at debug without + // a stack trace (not asserted: the module's test infrastructure + // captures no log, so the throttle is pinned through its + // bookkeeping only) + Connection first = refusingPool.getConnection(); + Assertions.assertNull(SlowQueryReportJmx.mbeans.get(poolName), + "a refused registration must not latch the per-name slot"); + Assertions.assertEquals(oname, SlowQueryReportJmx.registrationFailuresLogged.get(poolName), + "the first refusal of the name must be marked as reported for the throttle"); + + // later borrows keep retrying behind the freed slot: still + // refused (the obstacle holds the name), still no latch, and the + // identical marker keeps their logging throttled + try (Connection second = refusingPool.getConnection(); + Connection third = refusingPool.getConnection()) { + Assertions.assertNull(SlowQueryReportJmx.mbeans.get(poolName), + "retried refusals must keep leaving the slot free for the next borrow"); + Assertions.assertEquals(oname, SlowQueryReportJmx.registrationFailuresLogged.get(poolName), + "repeated failures for the same name must not re-mark it loudly"); + Assertions.assertEquals("registration-obstacle", + server.getAttribute(oname, "HeldBy"), + "the retried publishes must still be colliding with the obstacle"); + // a slow query on the refused pool still reaches the name's + // broadcaster through getNotifier() - state a closing pool + // must release together with the name even though no MBean + // registration of this pool ever succeeded + try (Statement st = first.createStatement()) { + st.execute("SELECT 1"); + } + Assertions.assertNotNull(SlowQueryReportJmx.notifiers.get(poolName), + "reporting a query must have created the name's broadcaster"); + } + first.close(); + + // close a pool that never got published: releasing the name must + // free the broadcaster it reported through and the throttle + // marker even though the registry slot was empty all along, so + // neither outlives the name - a new failure cycle under the name + // has to start marked loudly again + refusingPool.close(); + Assertions.assertNull(SlowQueryReportJmx.mbeans.get(poolName), + "closing the pool must release the per-name registry slot"); + Assertions.assertNull(SlowQueryReportJmx.notifiers.get(poolName), + "closing the pool must release the name's broadcaster the refused" + + " registration had still made it report through"); + Assertions.assertNull(SlowQueryReportJmx.registrationFailuresLogged.get(poolName), + "releasing the name must release its throttle marker"); + + // the name stayed retryable through every refusal: once the + // obstacle is gone, a fresh pool under the name publishes at once + // and publishes cleanly - success marks nothing in the throttle + server.unregisterMBean(oname); + DataSource revivedPool = reportingPoolNamed(poolName); + try (Connection borrowed = revivedPool.getConnection()) { + Assertions.assertTrue(server.isRegistered(oname), + "the first borrow of a pool started after the obstacle is gone" + + " must publish the MBean"); + Assertions.assertNull(SlowQueryReportJmx.registrationFailuresLogged.get(poolName), + "a succeeding registration must not leave a throttle marker behind"); + } + revivedPool.close(); + Assertions.assertFalse(server.isRegistered(oname), + "closing the revived pool must release the registration again"); + } finally { + try { + server.unregisterMBean(oname); + } catch (InstanceNotFoundException ignore) { + // nothing registered left to clean up under this name + } + } + } + @Test void concurrentClosesOfSameNamedPoolsReleaseTheNameExactlyOnce() throws Exception { String poolName = "JmxRace-" + System.nanoTime(); --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
