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 4b61a5c55a80ec8e899ee3509d04998f164a6138 Author: opencode <[email protected]> AuthorDate: Thu Nov 5 17:31:33 2026 +0100 Retry refused JMX unregistrations on later calls JmxUtil.unregisterJmx() logged a refused unregistration and returned false, but every caller drops its ObjectName regardless. The MBean therefore stayed registered with nothing ever retrying it, pinning the component behind it for the life of the JVM. Remember refused names in a bounded concurrent queue and retry the pending set on every later unregistration. A successful retry or an already-gone MBean removes the entry; a permanent refusal stays queued silently after the first error, and overflow logs a localized warning. Add a test with an MBean whose preDeregister refuses once. --- .../org/apache/tomcat/jdbc/pool/jmx/JmxUtil.java | 61 +++++++++++++++++++++- .../tomcat/jdbc/pool/util/LocalStrings.properties | 1 + .../tomcat/jdbc/pool/jmx/TestJmxRegistration.java | 54 +++++++++++++++++++ 3 files changed, 115 insertions(+), 1 deletion(-) 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 04eb606fac..99abf6f632 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,6 +18,8 @@ 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; @@ -38,6 +40,17 @@ 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. * @@ -93,6 +106,7 @@ public class JmxUtil { * the pool's own MBeans never do) */ public static boolean unregisterJmx(ObjectName oname) { + retryPendingUnregistrations(); if (oname == null) { return true; } @@ -106,12 +120,57 @@ public class JmxUtil { return true; } catch (Exception e) { // Normally the MBean kept its name and that name is now owned by - // nobody: say so, with the name, so an operator can take it down + // 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); 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 117cbb1130..08030473e9 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,6 +159,7 @@ 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 bc81fc1b0e..c5f9a6fd46 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 @@ -24,6 +24,7 @@ import java.util.Set; import java.util.concurrent.BlockingQueue; import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import javax.management.InstanceAlreadyExistsException; @@ -914,4 +915,57 @@ class TestJmxRegistration extends JdbcPoolBaseTest { "a post-start write would otherwise be stored without creating or removing anything"); Assertions.assertTrue(ds.isJmxEnabled(), "the refused write must not change the configuration"); } + + @Test + void aRefusedUnregistrationIsRetriedOnALaterCall() throws Exception { + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + ObjectName name = uniqueName("retry-unregister"); + RefusingUnregister.REFUSALS.set(1); + server.registerMBean(new RefusingUnregister(), name); + try { + Assertions.assertFalse(JmxUtil.unregisterJmx(name), + "the first refusal must be reported to the caller"); + Assertions.assertTrue(server.isRegistered(name), + "precondition: the refused unregistration leaves the MBean registered"); + + Assertions.assertTrue(JmxUtil.unregisterJmx(name), + "a later unregistration must retry the pending name"); + Assertions.assertFalse(server.isRegistered(name), + "the retried unregistration must take the MBean down"); + } finally { + if (server.isRegistered(name)) { + server.unregisterMBean(name); + } + } + } + + /** Standard MBean interface for {@link RefusingUnregister}. */ + public interface RefusingUnregisterMBean { + } + + /** MBean whose preDeregister refuses the first unregistration. */ + public static class RefusingUnregister implements RefusingUnregisterMBean, MBeanRegistration { + + static final AtomicInteger REFUSALS = new AtomicInteger(); + + @Override + public ObjectName preRegister(MBeanServer server, ObjectName name) { + return name; + } + + @Override + public void postRegister(Boolean registrationDone) { + } + + @Override + public void preDeregister() throws Exception { + if (REFUSALS.compareAndSet(1, 0)) { + throw new IllegalStateException("simulated unregistration refusal"); + } + } + + @Override + public void postDeregister() { + } + } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
