6776225: JMX.isNotificationSource wrong when DynamicWrapperMBean + SendNotification injection

Reviewed-by: dfuchs, jfdenise

jdk/src/share/classes/com/sun/jmx/mbeanserver/MBeanIntrospector.java

@@ -80,7 +80,7 @@ import javax.management.ReflectionException;
  * ancestor with ConvertingMethod.  But that would mean an extra object
  * for every Method in every Standard MBean interface.
  */
-abstract class MBeanIntrospector<M> {
+public abstract class MBeanIntrospector<M> {
     static final class PerInterfaceMap<M>
             extends WeakHashMap<Class<?>, WeakReference<PerInterface<M>>> {}
 
@@ -557,7 +557,7 @@ abstract class MBeanIntrospector<M> {
         return findNotificationsFromAnnotations(moi.getClass());
     }
 
-    private static MBeanNotificationInfo[] findNotificationsFromAnnotations(
+    public static MBeanNotificationInfo[] findNotificationsFromAnnotations(
             Class<?> mbeanClass) {
         Class<?> c = getAnnotatedNotificationInfoClass(mbeanClass);
         if (c == null)

jdk/src/share/classes/javax/management/JMX.java

@@ -51,8 +51,6 @@ public class JMX {
      * this class.
      */
     static final JMX proof = new JMX();
-    private static final ClassLogger logger =
-        new ClassLogger("javax.management.misc", "JMX");
 
     private JMX() {}
 
@@ -824,11 +822,16 @@ public class JMX {
      */
     public static boolean isNotificationSource(Object mbean)
             throws NotCompliantMBeanException {
-        if (mbean instanceof NotificationBroadcaster)
+        for (int i = 0; i < 2; i++) {
-            return true;
+            if (mbean instanceof NotificationBroadcaster ||
-        Object resource = (mbean instanceof DynamicWrapperMBean) ?
+                    MBeanInjector.injectsSendNotification(mbean))
-            ((DynamicWrapperMBean) mbean).getWrappedObject() : mbean;
+                return true;
-        return (MBeanInjector.injectsSendNotification(resource));
+            if (mbean instanceof DynamicWrapperMBean)
+                mbean = ((DynamicWrapperMBean) mbean).getWrappedObject();
+            else
+                break;
+        }
+        return false;
     }
 
     /**

jdk/src/share/classes/javax/management/StandardEmitterMBean.java

@@ -26,6 +26,7 @@
 package javax.management;
 
 import com.sun.jmx.mbeanserver.MBeanInjector;
+import com.sun.jmx.mbeanserver.MBeanIntrospector;
 import static javax.management.JMX.MBeanOptions;
 
 /**
@@ -195,10 +196,12 @@ public class StandardEmitterMBean extends StandardMBean
                                     MBeanOptions options,
                                     NotificationEmitter emitter) {
         super(implementation, mbeanInterface, options);
+        MBeanNotificationInfo[] defaultMBNIs = defaultMBNIs(implementation);
         if (emitter == null)
-            emitter = defaultEmitter();
+            emitter = defaultEmitter(defaultMBNIs);
         this.emitter = emitter;
-        this.notificationInfo = emitter.getNotificationInfo();
+        this.notificationInfo =
+                firstNonEmpty(emitter.getNotificationInfo(), defaultMBNIs);
         injectEmitter();
     }
 
@@ -320,15 +323,23 @@ public class StandardEmitterMBean extends StandardMBean
     protected StandardEmitterMBean(Class<?> mbeanInterface, MBeanOptions options,
                                    NotificationEmitter emitter) {
         super(mbeanInterface, options);
+        MBeanNotificationInfo[] defaultMBNIs = defaultMBNIs(this);
         if (emitter == null)
-            emitter = defaultEmitter();
+            emitter = defaultEmitter(defaultMBNIs);
         this.emitter = emitter;
-        this.notificationInfo = emitter.getNotificationInfo();
+        this.notificationInfo =
+                firstNonEmpty(emitter.getNotificationInfo(), defaultMBNIs);
         injectEmitter();
     }
 
-    private NotificationEmitter defaultEmitter() {
+    private static MBeanNotificationInfo[] defaultMBNIs(Object mbean) {
-        MBeanNotificationInfo[] mbnis = getNotificationInfo();
+        return MBeanIntrospector.findNotificationsFromAnnotations(
+                mbean.getClass());
+    }
+
+    private NotificationEmitter defaultEmitter(MBeanNotificationInfo[] defaultMBNIs) {
+        MBeanNotificationInfo[] mbnis =
+                firstNonEmpty(getNotificationInfo(), defaultMBNIs);
         // Will be null unless getNotificationInfo() is overridden,
         // since the notificationInfo field has not been set at this point.
         if (mbnis == null)
@@ -336,6 +347,14 @@ public class StandardEmitterMBean extends StandardMBean
         return new NotificationBroadcasterSupport(mbnis);
     }
 
+    private static <T> T[] firstNonEmpty(T[]... items) {
+        for (T[] t : items) {
+            if (t != null && t.length != 0)
+                return t;
+        }
+        return null;
+    }
+
     private void injectEmitter() {
         if (emitter instanceof SendNotification) {
             try {

jdk/src/share/classes/javax/management/StandardMBean.java

@@ -1058,10 +1058,6 @@ public class StandardMBean implements DynamicWrapperMBean, MBeanRegistration {
         cachedMBeanInfo = info;
     }
 
-    private boolean isMXBean() {
-        return mbean.isMXBean();
-    }
-
     private static <T> boolean identicalArrays(T[] a, T[] b) {
         if (a == b)
             return true;
@@ -1466,7 +1462,7 @@ public class StandardMBean implements DynamicWrapperMBean, MBeanRegistration {
             // Check for "MBeanNotificationInfo[] getNotificationInfo()"
             // method.
             //
-            // This method is only taken into account for the MBeanInfo
+            // This method is taken into account for the MBeanInfo
             // immutability checks if and only if the given subclass is
             // StandardEmitterMBean itself or can be assigned to
             // StandardEmitterMBean.

jdk/test/javax/management/MBeanServer/DynamicWrapperMBeanTest.java

@@ -23,14 +23,27 @@
 
 /*
  * @test DynamicWrapperMBeanTest
- * @bug 6624232
+ * @bug 6624232 6776225
  * @summary Test the DynamicWrapperMBean interface
  * @author Eamonn McManus
  */
 
 import java.lang.management.ManagementFactory;
+import javax.annotation.Resource;
+import javax.management.JMX;
+import javax.management.ListenerNotFoundException;
+import javax.management.MBeanNotificationInfo;
 import javax.management.MBeanServer;
+import javax.management.Notification;
+import javax.management.NotificationBroadcaster;
+import javax.management.NotificationBroadcasterSupport;
+import javax.management.NotificationEmitter;
+import javax.management.NotificationFilter;
+import javax.management.NotificationInfo;
+import javax.management.NotificationListener;
 import javax.management.ObjectName;
+import javax.management.SendNotification;
+import javax.management.StandardEmitterMBean;
 import javax.management.StandardMBean;
 import javax.management.modelmbean.ModelMBeanInfo;
 import javax.management.modelmbean.ModelMBeanInfoSupport;
@@ -39,6 +52,25 @@ import javax.management.modelmbean.RequiredModelMBean;
 import static javax.management.StandardMBean.Options;
 
 public class DynamicWrapperMBeanTest {
+    private static String failure;
+
+    public static void main(String[] args) throws Exception {
+        wrapTest();
+        notifTest();
+
+        if (failure == null)
+            System.out.println("TEST PASSED");
+        else
+            throw new Exception("TEST FAILED: " + failure);
+    }
+
+    private static final Options wrappedVisOpts = new Options();
+    private static final Options wrappedInvisOpts = new Options();
+    static {
+        wrappedVisOpts.setWrappedObjectVisible(true);
+        wrappedInvisOpts.setWrappedObjectVisible(false);
+    }
+
     public static interface WrappedMBean {
         public void sayHello();
     }
@@ -48,19 +80,13 @@ public class DynamicWrapperMBeanTest {
         }
     }
 
-    private static String failure;
+    private static void wrapTest() throws Exception {
-
-    public static void main(String[] args) throws Exception {
         if (Wrapped.class.getClassLoader() ==
                 StandardMBean.class.getClassLoader()) {
             throw new Exception(
                     "TEST ERROR: Resource and StandardMBean have same ClassLoader");
         }
 
-        Options wrappedVisOpts = new Options();
-        wrappedVisOpts.setWrappedObjectVisible(true);
-        Options wrappedInvisOpts = new Options();
-        wrappedInvisOpts.setWrappedObjectVisible(false);
         assertEquals("Options withWrappedObjectVisible(false)",
                      new Options(), wrappedInvisOpts);
 
@@ -138,8 +164,223 @@ public class DynamicWrapperMBeanTest {
         assertEquals("isInstanceOf(RequiredModelMBean) for invisible resource",
                 true, mbs.isInstanceOf(invisibleName, RequiredModelMBean.class.getName()));
 
-        if (failure != null)
+        mbs.unregisterMBean(visibleName);
-            throw new Exception("TEST FAILED: " + failure);
+        mbs.unregisterMBean(invisibleName);
+    }
+
+    private static enum WrapType {
+        NBS("NotificationBroadcasterSupport"),
+        INJ("@Resource SendNotification"),
+        STD_MBEAN_NBS("StandardMBean delegating to NotificationBroadcasterSupport"),
+        STD_MBEAN_INJ("StandardMBean delegating to @Resource SendNotification"),
+        STD_MBEAN_SUB_NBS("StandardMBean subclass implementing NotificationBroadcaster"),
+        STD_MBEAN_SUB_INJ("StandardMBean subclass with @Resource SendNotification"),
+        STD_EMIT_MBEAN_NBS("StandardEmitterMBean delegating to NotificationBroadcasterSupport"),
+        STD_EMIT_MBEAN_INJ("StandardEmitterMBean delegating to @Resource SendNotification"),
+        STD_EMIT_MBEAN_SUB("StandardEmitterMBean subclass"),
+        STD_EMIT_MBEAN_SUB_INJ("StandardEmitterMBean subclass with @Resource SendNotification");
+
+        WrapType(String s) {
+            this.s = s;
+        }
+
+        @Override
+        public String toString() {
+            return s;
+        }
+
+        private final String s;
+    }
+
+    @NotificationInfo(
+        types = {"foo", "bar"}
+    )
+    public static interface BroadcasterMBean {
+        public void send(Notification n);
+    }
+
+    public static class Broadcaster
+            extends NotificationBroadcasterSupport implements BroadcasterMBean {
+        public void send(Notification n) {
+            super.sendNotification(n);
+        }
+    }
+
+    public static interface SendNotifMBean extends BroadcasterMBean {
+    }
+
+    public static class SendNotif implements SendNotifMBean {
+        @Resource
+        private volatile SendNotification sendNotif;
+
+        public void send(Notification n) {
+            sendNotif.sendNotification(n);
+        }
+    }
+
+    public static class StdBroadcaster
+            extends StandardMBean
+            implements BroadcasterMBean, NotificationBroadcaster {
+        private final NotificationBroadcasterSupport nbs =
+                new NotificationBroadcasterSupport();
+
+        public StdBroadcaster() throws Exception {
+            super(BroadcasterMBean.class);
+        }
+
+        public void send(Notification n) {
+            nbs.sendNotification(n);
+        }
+
+        public void addNotificationListener(NotificationListener listener,
+                NotificationFilter filter, Object handback) {
+            nbs.addNotificationListener(listener, filter, handback);
+        }
+
+        public MBeanNotificationInfo[] getNotificationInfo() {
+            return null;
+        }
+
+        public void removeNotificationListener(NotificationListener listener)
+                throws ListenerNotFoundException {
+            nbs.removeNotificationListener(listener);
+        }
+    }
+
+    public static class StdSendNotif
+            extends StandardMBean implements SendNotifMBean {
+        @Resource
+        private volatile SendNotification sendNotif;
+
+        public StdSendNotif() throws Exception {
+            super(SendNotifMBean.class);
+        }
+
+        public void send(Notification n) {
+            sendNotif.sendNotification(n);
+        }
+    }
+
+    public static class StdEmitterBroadcaster // :-)
+            extends StandardEmitterMBean
+            implements BroadcasterMBean {
+
+        public StdEmitterBroadcaster() throws Exception {
+            super(BroadcasterMBean.class, null);
+        }
+
+        public void send(Notification n) {
+            super.sendNotification(n);
+        }
+    }
+
+    // This case is unlikely - if you're using @Resource SendNotification
+    // then there's no point in using StandardEmitterMBean, since
+    // StandardMBean would suffice.
+    public static class StdEmitterSendNotif
+            extends StandardEmitterMBean implements SendNotifMBean {
+        @Resource
+        private volatile SendNotification sendNotif;
+
+        public StdEmitterSendNotif() {
+            super(SendNotifMBean.class, null);
+        }
+
+        public void send(Notification n) {
+            sendNotif.sendNotification(n);
+        }
+    }
+
+    // Test that JMX.isNotificationSource and
+    // mbs.isInstanceOf("NotificationBroadcaster") work correctly even when
+    // the MBean is a broadcaster by virtue of its wrapped resource.
+    // Test that we find the MBeanNotificationInfo[] from the @NotificationInfo
+    // annotation on BroadcasterMBean.  We cover a large number of different
+    // MBean types, but all ultimately implement that interface.
+    private static void notifTest() throws Exception {
+        System.out.println("===Testing notification senders===");
+
+        for (WrapType wrapType : WrapType.values()) {
+            System.out.println("---" + wrapType);
+
+            final Object mbean;
+
+            switch (wrapType) {
+            case NBS:
+                // An MBean that extends NotificationBroadcasterSupport
+                mbean = new Broadcaster();
+                break;
+            case INJ:
+                // An MBean that injects SendNotification
+                mbean = new SendNotif();
+                break;
+            case STD_MBEAN_NBS:
+                // A StandardMBean that delegates to a NotificationBroadcasterSupport
+                mbean = new StandardMBean(
+                        new Broadcaster(), BroadcasterMBean.class, wrappedVisOpts);
+                break;
+            case STD_MBEAN_INJ:
+                // A StandardMBean that delegates to an object that injects
+                // SendNotification
+                mbean = new StandardMBean(
+                        new SendNotif(), BroadcasterMBean.class, wrappedVisOpts);
+                break;
+            case STD_EMIT_MBEAN_NBS: {
+                // A StandardEmitterMBean that delegates to a NotificationBroadcasterSupport
+                Broadcaster broadcaster = new Broadcaster();
+                mbean = new StandardEmitterMBean(
+                        broadcaster, BroadcasterMBean.class, wrappedVisOpts,
+                        broadcaster);
+                break;
+            }
+            case STD_EMIT_MBEAN_INJ: {
+                // A StandardEmitterMBean that delegates to an object that injects
+                // SendNotification
+                SendNotif sendNotif = new SendNotif();
+                mbean = new StandardEmitterMBean(
+                        sendNotif, BroadcasterMBean.class, wrappedVisOpts,
+                        null);
+                break;
+            }
+            case STD_MBEAN_SUB_NBS:
+                // A subclass of StandardMBean that implements NotificationBroadcaster
+                mbean = new StdBroadcaster();
+                break;
+            case STD_MBEAN_SUB_INJ:
+                // A subclass of StandardMBean that injects SendNotification
+                mbean = new StdSendNotif();
+                break;
+            case STD_EMIT_MBEAN_SUB:
+                // A subclass of StandardEmitterMBean
+                mbean = new StdEmitterBroadcaster();
+                break;
+            case STD_EMIT_MBEAN_SUB_INJ:
+                // A subclass of StandardEmitterMBean that injects SendNotification
+                // (which is a rather strange thing to do and probably a user
+                // misunderstanding but we should do the right thing anyway).
+                mbean = new StdEmitterSendNotif();
+                break;
+            default:
+                throw new AssertionError();
+            }
+
+            MBeanServer mbs = ManagementFactory.getPlatformMBeanServer();
+
+            final ObjectName name = new ObjectName("a:type=Sender");
+            mbs.registerMBean(mbean, name);
+            boolean isBroadcaster = mbs.isInstanceOf(
+                    name, NotificationBroadcaster.class.getName());
+            assertEquals("JMX.isNotificationSource(mbean)",
+                    true, JMX.isNotificationSource(mbean));
+            assertEquals("isInstanceOf(NotificationBroadcaster)",
+                    true, isBroadcaster);
+            MBeanNotificationInfo[] mbnis =
+                    mbs.getMBeanInfo(name).getNotifications();
+            assertEquals("MBeanNotificationInfo not empty",
+                    true, (mbnis.length > 0));
+
+            mbs.unregisterMBean(name);
+        }
     }
 
     private static void assertEquals(String what, Object expect, Object actual) {