This is an automated email from the ASF dual-hosted git repository.
markt-asf pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/9.0.x by this push:
new a9b9b28f6a Review "Fix name-scoped removal of attribute change
notification ..."
a9b9b28f6a is described below
commit a9b9b28f6a29ca57128d7fa0707b33faa3a490e8
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 | 104 +++++++---
.../modeler/TestBaseModelMBeanNotifications.java | 212 +++++++++++++++++++++
2 files changed, 294 insertions(+), 22 deletions(-)
diff --git
a/java/org/apache/tomcat/util/modeler/BaseNotificationBroadcaster.java
b/java/org/apache/tomcat/util/modeler/BaseNotificationBroadcaster.java
index dcd0224012..a57f20cd6f 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,23 +74,33 @@ public class BaseNotificationBroadcaster implements
NotificationBroadcaster {
synchronized (entries) {
- // Optimization to coalesce attribute name filters
- if (filter instanceof BaseAttributeFilter) {
+ /*
+ * 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) &&
+ if ((item.listener == listener) &&
isPlainAttributeFilter(item.filter) &&
(item.handback == handback)) {
- BaseAttributeFilter oldFilter = (BaseAttributeFilter)
item.filter;
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;
}
@@ -128,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
@@ -136,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) {
@@ -144,20 +158,39 @@ 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)) {
+ if (item.listener != listener) {
continue;
}
- BaseAttributeFilter filter = (BaseAttributeFilter) item.filter;
- String[] names = filter.getNames();
if (name == null) {
- if (names.length == 0) {
- // The accept-all registration
+ iterator.remove();
+ } else if (item.filter instanceof BaseAttributeFilter) {
+ BaseAttributeFilter filter = (BaseAttributeFilter)
item.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);
}
}
}
@@ -165,6 +198,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.
*
@@ -174,7 +216,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);
@@ -204,4 +246,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 &&
+ excludedAttributes.contains(((AttributeChangeNotification)
notification).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 c851f96fce..0e074966d2 100644
--- a/test/org/apache/tomcat/util/modeler/TestBaseModelMBeanNotifications.java
+++ b/test/org/apache/tomcat/util/modeler/TestBaseModelMBeanNotifications.java
@@ -131,6 +131,218 @@ 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 &&
+ ("A".equals(((AttributeChangeNotification)
notification).getAttributeName()) ||
+ "B".equals(((AttributeChangeNotification)
notification).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]
