This is an automated email from the ASF dual-hosted git repository.
markt-asf pushed a commit to branch 11.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/11.0.x by this push:
new 1a345a214e Review "Fix name-scoped removal of attribute change
notification ..."
1a345a214e is described below
commit 1a345a214ee299c66b022a525a44a39acffbd234
Author: Mark Thomas <[email protected]>
AuthorDate: Tue Nov 3 16:52:17 2026 +0000
Review "Fix name-scoped removal of attribute change notification ..."
The Javadoc for ModelMBeanNotificationBroadcaster.html#
removeAttributeChangeNotificationListener(
javax.management.NotificationListener,java.lang.String)
requires a null attribute name to remove the listener from all
registrations.
Don't modify filters in place. Handle filter sub-classes (unlikely)
Co-authored by: Claude / Sonnet 5.5
---
.../util/modeler/BaseNotificationBroadcaster.java | 102 ++++++++--
.../modeler/TestBaseModelMBeanNotifications.java | 211 +++++++++++++++++++++
2 files changed, 293 insertions(+), 20 deletions(-)
diff --git
a/java/org/apache/tomcat/util/modeler/BaseNotificationBroadcaster.java
b/java/org/apache/tomcat/util/modeler/BaseNotificationBroadcaster.java
index 8e58b976d8..78152e6fc4 100644
--- a/java/org/apache/tomcat/util/modeler/BaseNotificationBroadcaster.java
+++ b/java/org/apache/tomcat/util/modeler/BaseNotificationBroadcaster.java
@@ -18,8 +18,11 @@ package org.apache.tomcat.util.modeler;
import java.util.ArrayList;
+import java.util.HashSet;
import java.util.Iterator;
+import java.util.Set;
+import javax.management.AttributeChangeNotification;
import javax.management.ListenerNotFoundException;
import javax.management.MBeanNotificationInfo;
import javax.management.Notification;
@@ -71,21 +74,33 @@ public class BaseNotificationBroadcaster implements
NotificationBroadcaster {
synchronized (entries) {
- // Optimization to coalesce attribute name filters
- if (filter instanceof BaseAttributeFilter newFilter) {
+ /*
+ * Optimization to coalesce attribute name filters. The filters
are never modified. A filter may be shared
+ * with other registrations or broadcasters so the entry is given
a new filter instead. Only plain
+ * BaseAttributeFilter instances are coalesced. A sub-class may
change how notifications are filtered, which
+ * a plain filter would not preserve.
+ */
+ if (isPlainAttributeFilter(filter)) {
+ BaseAttributeFilter newFilter = (BaseAttributeFilter) filter;
for (BaseNotificationBroadcasterEntry item : entries) {
- if ((item.listener == listener) && (item.filter instanceof
BaseAttributeFilter oldFilter) &&
+ if ((item.listener == listener) &&
isPlainAttributeFilter(item.filter) &&
(item.handback == handback)) {
String[] newNames = newFilter.getNames();
- String[] oldNames = oldFilter.getNames();
+ String[] oldNames = ((BaseAttributeFilter)
item.filter).getNames();
if (newNames.length == 0) {
- oldFilter.clear();
- } else {
if (oldNames.length != 0) {
- for (String newName : newNames) {
- oldFilter.addAttribute(newName);
- }
+ // Now accepts all attribute names
+ item.filter = new BaseAttributeFilter(null);
}
+ } else if (oldNames.length != 0) {
+ BaseAttributeFilter combined = new
BaseAttributeFilter(null);
+ for (String oldName : oldNames) {
+ combined.addAttribute(oldName);
+ }
+ for (String newName : newNames) {
+ combined.addAttribute(newName);
+ }
+ item.filter = combined;
}
return;
}
@@ -126,7 +141,8 @@ public class BaseNotificationBroadcaster implements
NotificationBroadcaster {
/**
- * Remove an attribute change notification event listener for a specific
attribute name from this MBean. Other
+ * Remove an attribute change notification event listener for a specific
attribute name from this MBean, or all
+ * attribute change registrations for the listener if the name is
<code>null</code>. For a specific name, 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
@@ -134,7 +150,7 @@ public class BaseNotificationBroadcaster implements
NotificationBroadcaster {
*
* @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
+ * all attribute change registrations for the listener
*/
public void removeAttributeNotificationListener(NotificationListener
listener, String name) {
@@ -142,19 +158,38 @@ public class BaseNotificationBroadcaster implements
NotificationBroadcaster {
Iterator<BaseNotificationBroadcasterEntry> iterator =
entries.iterator();
while (iterator.hasNext()) {
BaseNotificationBroadcasterEntry item = iterator.next();
- if (item.listener != listener || !(item.filter instanceof
BaseAttributeFilter filter)) {
+ if (item.listener != listener) {
continue;
}
- String[] names = filter.getNames();
if (name == null) {
- if (names.length == 0) {
- // The accept-all registration
+ iterator.remove();
+ } else if (item.filter instanceof BaseAttributeFilter filter) {
+ String[] names = filter.getNames();
+ if (!isPlainAttributeFilter(filter)) {
+ if (names.length > 0) {
+ // Keep the subclass's filtering behavior and its
shared state intact. The subclass may
+ // accept names beyond those reported by
getNames(), so exclude only the requested name
+ // from this entry rather than replacing the
filter or removing the entry.
+ item.excludeAttribute(name);
+ }
+ } else if (names.length == 1 && name.equals(names[0])) {
iterator.remove();
+ } else if (names.length > 1) {
+ // The filter may be shared with other registrations
or broadcasters. Replace this entry's
+ // filter rather than changing the shared filter.
+ BaseAttributeFilter replacement = new
BaseAttributeFilter(null);
+ boolean removed = false;
+ for (String attributeName : names) {
+ if (name.equals(attributeName)) {
+ removed = true;
+ } else {
+ replacement.addAttribute(attributeName);
+ }
+ }
+ if (removed) {
+ item.filter = replacement;
+ }
}
- } else if (names.length == 1 && names[0].equals(name)) {
- iterator.remove();
- } else {
- filter.removeAttribute(name);
}
}
}
@@ -162,6 +197,15 @@ public class BaseNotificationBroadcaster implements
NotificationBroadcaster {
}
+ /**
+ * Is the filter a plain <code>BaseAttributeFilter</code> rather than an
instance of a sub-class, which may change
+ * how notifications are filtered?
+ */
+ private static boolean isPlainAttributeFilter(NotificationFilter filter) {
+ return filter != null && filter.getClass() ==
BaseAttributeFilter.class;
+ }
+
+
/**
* Send the specified notification to all interested listeners.
*
@@ -171,7 +215,7 @@ public class BaseNotificationBroadcaster implements
NotificationBroadcaster {
synchronized (entries) {
for (BaseNotificationBroadcasterEntry item : entries) {
- if ((item.filter != null) &&
(!item.filter.isNotificationEnabled(notification))) {
+ if (!item.isNotificationEnabled(notification)) {
continue;
}
item.listener.handleNotification(notification, item.handback);
@@ -201,4 +245,22 @@ class BaseNotificationBroadcasterEntry {
public NotificationListener listener;
+ // Accessed only while holding the broadcaster's entries lock.
+ private Set<String> excludedAttributes;
+
+ void excludeAttribute(String name) {
+ if (excludedAttributes == null) {
+ excludedAttributes = new HashSet<>();
+ }
+ excludedAttributes.add(name);
+ }
+
+ boolean isNotificationEnabled(Notification notification) {
+ if (excludedAttributes != null && notification instanceof
AttributeChangeNotification acn &&
+ excludedAttributes.contains(acn.getAttributeName())) {
+ return false;
+ }
+ return filter == null || filter.isNotificationEnabled(notification);
+ }
+
}
diff --git
a/test/org/apache/tomcat/util/modeler/TestBaseModelMBeanNotifications.java
b/test/org/apache/tomcat/util/modeler/TestBaseModelMBeanNotifications.java
index 73e2d36e67..2cd20e6b42 100644
--- a/test/org/apache/tomcat/util/modeler/TestBaseModelMBeanNotifications.java
+++ b/test/org/apache/tomcat/util/modeler/TestBaseModelMBeanNotifications.java
@@ -124,6 +124,217 @@ public class TestBaseModelMBeanNotifications {
Assert.assertTrue(listener.received.isEmpty());
}
+ @Test
+ public void testNullNameRemovalRemovesAllAttributeRegistrations() throws
Exception {
+ BaseModelMBean mbean = new BaseModelMBean();
+ TesterNotificationListener listener = new TesterNotificationListener();
+ TesterNotificationListener otherListener = new
TesterNotificationListener();
+ // Cover coalesced names, a separate handback and an accept-all
registration.
+ mbean.addAttributeChangeNotificationListener(listener, "A", "hb1");
+ mbean.addAttributeChangeNotificationListener(listener, "B", "hb1");
+ mbean.addAttributeChangeNotificationListener(listener, "C", "hb2");
+ mbean.addAttributeChangeNotificationListener(listener, null, "hb3");
+ mbean.addAttributeChangeNotificationListener(otherListener, "A",
"other");
+
+ mbean.removeAttributeChangeNotificationListener(listener, null);
+ send(mbean, "A");
+ send(mbean, "B");
+ send(mbean, "C");
+ send(mbean, "D");
+
+ Assert.assertTrue(listener.received.isEmpty());
+ Assert.assertEquals(List.of("A:other"), otherListener.received);
+ }
+
+ @Test
+ public void testNullNameRemovalKeepsGeneralNotifications() throws
Exception {
+ BaseModelMBean mbean = new BaseModelMBean();
+ TesterNotificationListener listener = new TesterNotificationListener();
+ mbean.addNotificationListener(listener, notification -> true, "hb");
+
+ mbean.removeAttributeChangeNotificationListener(listener, null);
+ send(mbean, "A");
+ Assert.assertTrue(listener.received.isEmpty());
+
+ mbean.sendNotification("message");
+ Assert.assertEquals(List.of("jmx.modelmbean.generic:hb"),
listener.received);
+ }
+
+ @Test
+ public void testSharedFilterRemovalKeepsOtherListener() throws Exception {
+ BaseModelMBean mbean = new BaseModelMBean();
+ TesterNotificationListener listener1 = new
TesterNotificationListener();
+ TesterNotificationListener listener2 = new
TesterNotificationListener();
+ BaseAttributeFilter filter = new BaseAttributeFilter("A");
+ filter.addAttribute("B");
+ // The same filter may be supplied for independent registrations.
+ mbean.addNotificationListener(listener1, filter, "hb1");
+ mbean.addNotificationListener(listener2, filter, "hb2");
+
+ mbean.removeAttributeChangeNotificationListener(listener1, "A");
+ send(mbean, "A");
+ send(mbean, "B");
+
+ Assert.assertEquals(List.of("B:hb1"), listener1.received);
+ Assert.assertEquals(List.of("A:hb2", "B:hb2"), listener2.received);
+ }
+
+ /*
+ * A filter may be shared between registrations
(BaseModelMBean.addNotificationListener() uses the same filter for
+ * the general and the attribute registration). Coalescing registrations
must not change the shared filter.
+ */
+ @Test
+ public void testCoalescingNamesDoesNotChangeSharedFilter() throws
Exception {
+ BaseModelMBean mbean = new BaseModelMBean();
+ TesterNotificationListener listener1 = new
TesterNotificationListener();
+ TesterNotificationListener listener2 = new
TesterNotificationListener();
+ BaseAttributeFilter filter = new BaseAttributeFilter("A");
+ mbean.addNotificationListener(listener1, filter, "hb1");
+ mbean.addNotificationListener(listener2, filter, "hb2");
+
+ // Same listener and handback so the registrations are coalesced
+ mbean.addAttributeChangeNotificationListener(listener1, "B", "hb1");
+ send(mbean, "A");
+ send(mbean, "B");
+
+ Assert.assertEquals(List.of("A:hb1", "B:hb1"), listener1.received);
+ Assert.assertEquals(List.of("A:hb2"), listener2.received);
+ Assert.assertArrayEquals(new String[] { "A" }, filter.getNames());
+ }
+
+ @Test
+ public void testCoalescingAcceptAllDoesNotChangeSharedFilter() throws
Exception {
+ BaseModelMBean mbean = new BaseModelMBean();
+ TesterNotificationListener listener1 = new
TesterNotificationListener();
+ TesterNotificationListener listener2 = new
TesterNotificationListener();
+ BaseAttributeFilter filter = new BaseAttributeFilter("A");
+ mbean.addNotificationListener(listener1, filter, "hb1");
+ mbean.addNotificationListener(listener2, filter, "hb2");
+
+ mbean.addAttributeChangeNotificationListener(listener1, null, "hb1");
+ send(mbean, "Z");
+
+ Assert.assertEquals(List.of("Z:hb1"), listener1.received);
+ Assert.assertTrue(listener2.received.isEmpty());
+ Assert.assertArrayEquals(new String[] { "A" }, filter.getNames());
+ }
+
+ @Test
+ public void testCoalescingKeepsAcceptAll() throws Exception {
+ BaseModelMBean mbean = new BaseModelMBean();
+ TesterNotificationListener listener = new TesterNotificationListener();
+ mbean.addAttributeChangeNotificationListener(listener, null, "hb");
+ // The accept-all registration is not narrowed by a later named
registration
+ mbean.addAttributeChangeNotificationListener(listener, "A", "hb");
+ send(mbean, "Z");
+
+ Assert.assertEquals(List.of("Z:hb"), listener.received);
+ }
+
+ /*
+ * A sub-class of the filter may change how notifications are filtered.
Its registration is never coalesced with
+ * another registration as that would replace it with a plain filter.
+ */
+ @Test
+ public void testSubclassFilterIsNotCoalesced() throws Exception {
+ BaseModelMBean mbean = new BaseModelMBean();
+ TesterNotificationListener listener = new TesterNotificationListener();
+ BaseAttributeFilter rejectAll = new BaseAttributeFilter("A") {
+ private static final long serialVersionUID = 1L;
+
+ @Override
+ public boolean isNotificationEnabled(Notification notification) {
+ return false;
+ }
+ };
+ mbean.addNotificationListener(listener, rejectAll, "hb");
+ mbean.addAttributeChangeNotificationListener(listener, "B", "hb");
+ send(mbean, "A");
+ send(mbean, "B");
+
+ Assert.assertEquals(List.of("B:hb"), listener.received);
+ }
+
+ @Test
+ public void testSharedSubclassFilterRemovalKeepsOtherListener() throws
Exception {
+ BaseModelMBean mbean = new BaseModelMBean();
+ TesterNotificationListener listener1 = new
TesterNotificationListener();
+ TesterNotificationListener listener2 = new
TesterNotificationListener();
+ BaseAttributeFilter filter = new BaseAttributeFilter("A") {
+ private static final long serialVersionUID = 1L;
+ };
+ filter.addAttribute("B");
+ mbean.addNotificationListener(listener1, filter, "hb1");
+ mbean.addNotificationListener(listener2, filter, "hb2");
+
+ mbean.removeAttributeChangeNotificationListener(listener1, "A");
+ send(mbean, "A");
+ send(mbean, "B");
+ Assert.assertEquals(List.of("B:hb1"), listener1.received);
+ Assert.assertEquals(List.of("A:hb2", "B:hb2"), listener2.received);
+
+ // Removing the remaining name must not turn the registration into an
accept-all filter.
+ listener1.received.clear();
+ listener2.received.clear();
+ mbean.removeAttributeChangeNotificationListener(listener1, "B");
+ send(mbean, "A");
+ send(mbean, "B");
+ send(mbean, "C");
+ Assert.assertTrue(listener1.received.isEmpty());
+ Assert.assertEquals(List.of("A:hb2", "B:hb2"), listener2.received);
+ }
+
+ @Test
+ public void testSubclassFilterRemovalPreservesCustomFiltering() throws
Exception {
+ BaseModelMBean mbean = new BaseModelMBean();
+ TesterNotificationListener listener = new TesterNotificationListener();
+ BaseAttributeFilter filter = new BaseAttributeFilter("A") {
+ private static final long serialVersionUID = 1L;
+
+ @Override
+ public boolean isNotificationEnabled(Notification notification) {
+ return super.isNotificationEnabled(notification) &&
+ !"C".equals(((AttributeChangeNotification)
notification).getAttributeName());
+ }
+ };
+ filter.addAttribute("B");
+ filter.addAttribute("C");
+ mbean.addNotificationListener(listener, filter, "hb");
+
+ mbean.removeAttributeChangeNotificationListener(listener, "A");
+ send(mbean, "A");
+ send(mbean, "B");
+ send(mbean, "C");
+ Assert.assertEquals(List.of("B:hb"), listener.received);
+
+ // Changes to the original subclass filter still apply to the
registration.
+ filter.addAttribute("D");
+ send(mbean, "D");
+ Assert.assertEquals(List.of("B:hb", "D:hb"), listener.received);
+ }
+
+ @Test
+ public void testSubclassFilterRemovalKeepsOtherAcceptedNames() throws
Exception {
+ BaseModelMBean mbean = new BaseModelMBean();
+ TesterNotificationListener listener = new TesterNotificationListener();
+ BaseAttributeFilter filter = new BaseAttributeFilter("A") {
+ private static final long serialVersionUID = 1L;
+
+ @Override
+ public boolean isNotificationEnabled(Notification notification) {
+ return notification instanceof AttributeChangeNotification acn
&&
+ ("A".equals(acn.getAttributeName()) ||
"B".equals(acn.getAttributeName()));
+ }
+ };
+ mbean.addNotificationListener(listener, filter, "hb");
+
+ // A subclass may accept more names than getNames() reports.
+ mbean.removeAttributeChangeNotificationListener(listener, "A");
+ send(mbean, "A");
+ send(mbean, "B");
+ Assert.assertEquals(List.of("B:hb"), listener.received);
+ }
+
@Test
public void testUnknownNameRemovalIsNoOp() throws Exception {
BaseModelMBean mbean = new BaseModelMBean();
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
