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 876c3278262777bccde4e48380ab76bca30d667b Author: opencode <[email protected]> AuthorDate: Thu Nov 5 21:40:50 2026 +0100 Do not retry refused JMX unregistrations JMX is a best-effort management channel, and under Catalina each pool component has a unique name. The pending-unregistration queue retried a refused name on every later unregistration, which could take down a replacement MBean that had taken the name after the refused one went down. Drop the retry queue and its capacity and logging, log a refusal once, and document in JmxUtil that registrations and unregistrations are attempted once and not retried. Keep the explicit second call working: a caller that asks again for the same name still reaches the MBean server. Update the tests to cover both the explicit retry and the absence of an automatic one. --- .../org/apache/tomcat/jdbc/pool/jmx/JmxUtil.java | 78 +++++----------------- .../tomcat/jdbc/pool/util/LocalStrings.properties | 1 - .../tomcat/jdbc/pool/jmx/TestJmxRegistration.java | 32 ++++++++- webapps/docs/changelog.xml | 9 +++ 4 files changed, 55 insertions(+), 65 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/JmxUtil.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/JmxUtil.java index 99abf6f632..34a08eb236 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/JmxUtil.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/JmxUtil.java @@ -18,8 +18,6 @@ package org.apache.tomcat.jdbc.pool.jmx; import java.lang.management.ManagementFactory; -import java.util.Iterator; -import java.util.concurrent.ConcurrentLinkedQueue; import javax.management.InstanceNotFoundException; import javax.management.MalformedObjectNameException; @@ -31,6 +29,17 @@ import org.apache.tomcat.jdbc.pool.util.MessageFactory; /** * Utility class for JMX registration of JDBC pool components. + * <p> + * JMX is a best-effort management channel, not part of the pool's operation. A registration or an unregistration is + * attempted once and a failure is logged; nothing is retried later from this class. + * </p> + * <p> + * Under Catalina the names are unique - the container assigns them, and a name identifies one live component - so a + * refused registration is a configuration or server problem that an operator reads from the log, not one the pool + * should keep hammering. Retrying a refused unregistration by name would be worse than leaving it: by the time the + * retry ran, the name could belong to a replacement MBean, and the retry would silently take down the replacement's + * management surface. + * </p> */ public class JmxUtil { @@ -40,17 +49,6 @@ public class JmxUtil { private static final Log log = LogFactory.getLog(JmxUtil.class); - /** How many refused unregistrations are kept for a later retry. */ - private static final int MAX_pendingUnregistrations = 64; - - /** - * Names whose unregistration the MBean server refused. Every later unregistration retries them, so a transient - * refusal does not leave the MBean published - and its component pinned - for the life of the JVM. The queue is - * bounded: a permanent refusal cannot grow it without limit. - */ - private static final ConcurrentLinkedQueue<ObjectName> pendingUnregistrations = - new ConcurrentLinkedQueue<>(); - /** * Register an object as an MBean. * @@ -103,10 +101,11 @@ public class JmxUtil { * handle regardless, held by nothing for the rest of the JVM's * life (an MBean whose {@code postDeregister()} throws reports * the same failure although the server already took it down; - * the pool's own MBeans never do) + * the pool's own MBeans never do). A refusal is reported here + * and not remembered for a later retry: see the class + * documentation. */ public static boolean unregisterJmx(ObjectName oname) { - retryPendingUnregistrations(); if (oname == null) { return true; } @@ -120,57 +119,14 @@ public class JmxUtil { return true; } catch (Exception e) { // Normally the MBean kept its name and that name is now owned by - // nobody, so remember it for a later retry and say so, with the - // name, so an operator can take it down if it stays refused. - rememberPendingUnregistration(oname); + // nobody. Say so, with the name, so an operator can take it down; + // this class does not retry it, because the name may already + // describe a replacement MBean by the time a retry ran. log.error(MessageFactory.get("jmxUtil.jmxUnregistrationRefused", oname), e); return false; } } - /** - * Retries the unregistrations the server refused earlier. A name that has since become unregistrable is removed - * from the pending set; a name that is still refused stays there silently, the first refusal already having been - * reported. - */ - private static void retryPendingUnregistrations() { - if (pendingUnregistrations.isEmpty()) { - return; - } - for (Iterator<ObjectName> it = pendingUnregistrations.iterator(); it.hasNext();) { - ObjectName pending = it.next(); - try { - ManagementFactory.getPlatformMBeanServer().unregisterMBean(pending); - pendingUnregistrations.remove(pending); - } catch (InstanceNotFoundException e) { - // The MBean went down through another path after all - pendingUnregistrations.remove(pending); - } catch (Exception e) { - // Still refused; the first refusal was already reported and a - // retry must not flood the log - if (log.isDebugEnabled()) { - log.debug("Jmx unregistration of [" + pending + "] still refused: " + e); - } - } - } - } - - /** - * Remembers a refused name for a later retry, up to the bounded capacity. - * - * @param oname the name whose unregistration was refused - */ - private static void rememberPendingUnregistration(ObjectName oname) { - if (pendingUnregistrations.contains(oname)) { - return; - } - if (pendingUnregistrations.size() >= MAX_pendingUnregistrations) { - log.warn(MessageFactory.get("jmxUtil.jmxUnregistrationRetryLimit", oname)); - return; - } - pendingUnregistrations.add(oname); - } - /* * Concatenates base and keyprop into one object name, so the key * property must carry its own leading comma and a value the ObjectName diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties index 76bb710add..44f3d82f17 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties @@ -159,7 +159,6 @@ jmxConnectionPool.droppingNotification=Dropping notification of type [{0}] for p jmxUtil.jmxRegistrationFailed=Jmx registration of [{2}] under base [{0}] and key property [{1}] failed. jmxUtil.jmxAlreadyUnregistered=Jmx unregistration found no MBean under that name. jmxUtil.jmxUnregistrationRefused=Jmx unregistration of [{0}] was refused. The MBean is normally still registered under that name, and its caller releases the name regardless, so nothing will ever unregister it again: it keeps a live management surface and pins the component behind it for the life of the JVM. -jmxUtil.jmxUnregistrationRetryLimit=Jmx unregistration of [{0}] was refused and cannot be remembered for a retry: the retry set is full. The MBean may stay registered, pinning its component. poolProperties.invalidInterceptorProperty=Invalid interceptor property ''{0}'', missing ''='', in jdbcInterceptors value [{1}] poolProperties.credentialClearStripsStringEntry=Clearing the ''{0}'' credential also removed the entry the connectionProperties string contributes for that key. The string still names it, so every connection established from now on goes without that credential, and a connectionProperties reconfigure that re-names the key applies it again. Take the credential out of the connectionProperties string to retire it. 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 c5f9a6fd46..1f948fdd5a 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 @@ -917,7 +917,7 @@ class TestJmxRegistration extends JdbcPoolBaseTest { } @Test - void aRefusedUnregistrationIsRetriedOnALaterCall() throws Exception { + void aRefusedUnregistrationSucceedsWhenTheCallerAsksAgain() throws Exception { MBeanServer server = ManagementFactory.getPlatformMBeanServer(); ObjectName name = uniqueName("retry-unregister"); RefusingUnregister.REFUSALS.set(1); @@ -929,9 +929,35 @@ class TestJmxRegistration extends JdbcPoolBaseTest { "precondition: the refused unregistration leaves the MBean registered"); Assertions.assertTrue(JmxUtil.unregisterJmx(name), - "a later unregistration must retry the pending name"); + "a later call for the same name must try again"); Assertions.assertFalse(server.isRegistered(name), - "the retried unregistration must take the MBean down"); + "the second call must take the MBean down"); + } finally { + if (server.isRegistered(name)) { + server.unregisterMBean(name); + } + } + } + + @Test + void aRefusedUnregistrationIsNotRetriedByAnUnrelatedCall() throws Exception { + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + ObjectName name = uniqueName("no-retry-unregister"); + RefusingUnregister.REFUSALS.set(1); + server.registerMBean(new RefusingUnregister(), name); + try { + Assertions.assertFalse(JmxUtil.unregisterJmx(name), + "precondition: the first refusal is reported"); + Assertions.assertTrue(server.isRegistered(name), + "precondition: the refused MBean stays registered"); + + // JMX is a best-effort management channel. A refusal is logged for + // the operator and is not remembered: a later retry by name could + // belong to a replacement MBean that took the name in the meantime. + Assertions.assertTrue(JmxUtil.unregisterJmx(null), + "releasing nothing is still the requested end state"); + Assertions.assertTrue(server.isRegistered(name), + "an unrelated unregistration must not retry the refused name"); } finally { if (server.isRegistered(name)) { server.unregisterMBean(name); diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index cfcce332fb..d713ee4671 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -1387,6 +1387,15 @@ name is documented as having to be unique among live pools, and a duplicate is reported at error level. (remm) </fix> + <fix> + Remove the retry of refused JMX unregistrations from the JDBC pool's + JMX helper. JMX is a best-effort management channel and Catalina gives + every pool component a name of its own, so a refusal is logged for the + operator rather than remembered and retried later. A retry by name + could unregister a replacement MBean that had taken the name in the + meantime, silently removing the replacement's management surface. + (remm) + </fix> </changelog> </subsection> <subsection name="Other"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
