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 aed989cbdca764913fd125ce11d6fbdb5461ba6b Author: opencode <[email protected]> AuthorDate: Thu Oct 15 10:12:47 2026 +0200 Fix name-scoped removal of attribute change notification listeners BaseModelMBean.removeAttributeChangeNotificationListener() previously removed every attribute change registration for the given listener instead of only the registration for the requested attribute name (the long-standing FIXME in the method). Add a removal method to BaseNotificationBroadcaster that is scoped to an attribute name. Since addNotificationListener() coalesces registrations with the same listener and handback into a single entry with a multi-name BaseAttributeFilter, removal drops the name from the filter and only removes the entry when no explicit name remains, so a single-name filter is never silently turned into an accept-all filter. Registrations that accept all attribute names (created with a null attribute name) can only be removed by passing a null name, mirroring the registration semantics. Entries registered with other filters are left untouched. Also add a unit test covering the coalesced, per-handback, accept-all and non-attribute-filter cases. --- .../apache/tomcat/util/modeler/BaseModelMBean.java | 7 +- .../util/modeler/BaseNotificationBroadcaster.java | 38 ++++++ .../modeler/TestBaseModelMBeanNotifications.java | 141 +++++++++++++++++++++ 3 files changed, 184 insertions(+), 2 deletions(-) diff --git a/java/org/apache/tomcat/util/modeler/BaseModelMBean.java b/java/org/apache/tomcat/util/modeler/BaseModelMBean.java index ad5566cb6e..d1f09b57ae 100644 --- a/java/org/apache/tomcat/util/modeler/BaseModelMBean.java +++ b/java/org/apache/tomcat/util/modeler/BaseModelMBean.java @@ -517,9 +517,12 @@ public class BaseModelMBean implements DynamicMBean, MBeanRegistration, ModelMBe throw new IllegalArgumentException(sm.getString("baseModelMBean.nullListener")); } - // FIXME - currently this removes *all* notifications for this listener + if (log.isTraceEnabled()) { + log.trace("removeAttributeNotificationListener " + listener); + } + if (attributeBroadcaster != null) { - attributeBroadcaster.removeNotificationListener(listener); + attributeBroadcaster.removeAttributeNotificationListener(listener, name); } } diff --git a/java/org/apache/tomcat/util/modeler/BaseNotificationBroadcaster.java b/java/org/apache/tomcat/util/modeler/BaseNotificationBroadcaster.java index ac287c85dd..8e58b976d8 100644 --- a/java/org/apache/tomcat/util/modeler/BaseNotificationBroadcaster.java +++ b/java/org/apache/tomcat/util/modeler/BaseNotificationBroadcaster.java @@ -18,6 +18,7 @@ package org.apache.tomcat.util.modeler; import java.util.ArrayList; +import java.util.Iterator; import javax.management.ListenerNotFoundException; import javax.management.MBeanNotificationInfo; @@ -124,6 +125,43 @@ public class BaseNotificationBroadcaster implements NotificationBroadcaster { } + /** + * Remove an attribute change notification event listener for a specific attribute name from this MBean. Other + * registrations of the same listener are left untouched. If a coalesced filter ends up accepting no explicit + * names, the corresponding entry is removed entirely rather than being turned into an accept-all filter. A + * registration that accepts all attribute names (a filter with no explicit names, created when the listener was + * added with a <code>null</code> attribute name) can only be removed by passing a <code>null</code> name here. + * + * @param listener The listener to be removed + * @param name The attribute name for which the registration should be removed, or <code>null</code> to remove + * a registration that accepts all attribute names + */ + public void removeAttributeNotificationListener(NotificationListener listener, String name) { + + synchronized (entries) { + Iterator<BaseNotificationBroadcasterEntry> iterator = entries.iterator(); + while (iterator.hasNext()) { + BaseNotificationBroadcasterEntry item = iterator.next(); + if (item.listener != listener || !(item.filter instanceof BaseAttributeFilter filter)) { + continue; + } + String[] names = filter.getNames(); + if (name == null) { + if (names.length == 0) { + // The accept-all registration + iterator.remove(); + } + } else if (names.length == 1 && names[0].equals(name)) { + iterator.remove(); + } else { + filter.removeAttribute(name); + } + } + } + + } + + /** * Send the specified notification to all interested listeners. * diff --git a/test/org/apache/tomcat/util/modeler/TestBaseModelMBeanNotifications.java b/test/org/apache/tomcat/util/modeler/TestBaseModelMBeanNotifications.java new file mode 100644 index 0000000000..4d92b2c934 --- /dev/null +++ b/test/org/apache/tomcat/util/modeler/TestBaseModelMBeanNotifications.java @@ -0,0 +1,141 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomcat.util.modeler; + +import java.util.ArrayList; +import java.util.List; + +import javax.management.Attribute; +import javax.management.AttributeChangeNotification; +import javax.management.Notification; +import javax.management.NotificationFilter; +import javax.management.NotificationListener; + +import org.junit.Assert; +import org.junit.Test; + +public class TestBaseModelMBeanNotifications { + + private static final class TesterNotificationListener implements NotificationListener { + + private final List<String> received = new ArrayList<>(); + + @Override + public void handleNotification(Notification notification, Object handback) { + String name = notification instanceof AttributeChangeNotification ? + ((AttributeChangeNotification) notification).getAttributeName() : notification.getType(); + received.add(name + ":" + handback); + } + } + + private static void send(BaseModelMBean mbean, String attributeName) throws Exception { + mbean.sendAttributeChangeNotification(new Attribute(attributeName, "old"), + new Attribute(attributeName, "new")); + } + + @Test + public void testCoalescedRemovalKeepsOtherNames() throws Exception { + BaseModelMBean mbean = new BaseModelMBean(); + TesterNotificationListener listener = new TesterNotificationListener(); + // Same listener and handback: these registrations are coalesced into a + // single entry with a filter accepting both names + mbean.addAttributeChangeNotificationListener(listener, "A", "hb"); + mbean.addAttributeChangeNotificationListener(listener, "B", "hb"); + mbean.removeAttributeChangeNotificationListener(listener, "A"); + send(mbean, "A"); + send(mbean, "B"); + Assert.assertEquals(List.of("B:hb"), listener.received); + } + + @Test + public void testSingleNameRemovalDoesNotAcceptAll() throws Exception { + BaseModelMBean mbean = new BaseModelMBean(); + TesterNotificationListener listener = new TesterNotificationListener(); + mbean.addAttributeChangeNotificationListener(listener, "A", "hb"); + mbean.removeAttributeChangeNotificationListener(listener, "A"); + // Removing the only accepted name must remove the entry entirely rather + // than turning the (now empty) filter into an accept-all filter + send(mbean, "A"); + send(mbean, "B"); + Assert.assertTrue(listener.received.isEmpty()); + } + + @Test + public void testSeparateHandbacksRemovedIndependently() throws Exception { + BaseModelMBean mbean = new BaseModelMBean(); + TesterNotificationListener listener = new TesterNotificationListener(); + mbean.addAttributeChangeNotificationListener(listener, "A", "hb1"); + mbean.addAttributeChangeNotificationListener(listener, "B", "hb2"); + mbean.removeAttributeChangeNotificationListener(listener, "A"); + send(mbean, "A"); + send(mbean, "B"); + Assert.assertEquals(List.of("B:hb2"), listener.received); + } + + @Test + public void testAcceptAllRemoval() throws Exception { + BaseModelMBean mbean = new BaseModelMBean(); + TesterNotificationListener listener = new TesterNotificationListener(); + // A null attribute name registers an accept-all listener + mbean.addAttributeChangeNotificationListener(listener, null, "hb"); + // A specific name can not be removed from an accept-all registration + mbean.removeAttributeChangeNotificationListener(listener, "A"); + send(mbean, "A"); + Assert.assertEquals(List.of("A:hb"), listener.received); + // The accept-all registration is removed by a null name + mbean.removeAttributeChangeNotificationListener(listener, null); + listener.received.clear(); + send(mbean, "A"); + Assert.assertTrue(listener.received.isEmpty()); + } + + @Test + public void testUnknownNameRemovalIsNoOp() throws Exception { + BaseModelMBean mbean = new BaseModelMBean(); + TesterNotificationListener listener = new TesterNotificationListener(); + mbean.addAttributeChangeNotificationListener(listener, "A", "hb"); + mbean.removeAttributeChangeNotificationListener(listener, "Z"); + send(mbean, "A"); + Assert.assertEquals(List.of("A:hb"), listener.received); + } + + @Test + public void testOtherListenerRemovalDoesNotAffectThisListener() throws Exception { + BaseModelMBean mbean = new BaseModelMBean(); + TesterNotificationListener listener1 = new TesterNotificationListener(); + TesterNotificationListener listener2 = new TesterNotificationListener(); + mbean.addAttributeChangeNotificationListener(listener1, "A", "hb"); + mbean.addAttributeChangeNotificationListener(listener2, "A", "hb"); + mbean.removeAttributeChangeNotificationListener(listener1, "A"); + send(mbean, "A"); + Assert.assertTrue(listener1.received.isEmpty()); + Assert.assertEquals(List.of("A:hb"), listener2.received); + } + + @Test + public void testNonAttributeFilterIsNotRemovedByName() throws Exception { + BaseModelMBean mbean = new BaseModelMBean(); + TesterNotificationListener listener = new TesterNotificationListener(); + NotificationFilter acceptAll = notification -> true; + // Registered with a general (non attribute) filter via the + // NotificationBroadcaster method + mbean.addNotificationListener(listener, acceptAll, "hb"); + mbean.removeAttributeChangeNotificationListener(listener, "A"); + send(mbean, "A"); + Assert.assertEquals(List.of("A:hb"), listener.received); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
