From 07afb9e470c9881057e9dc8f6397b66717097924 Mon Sep 17 00:00:00 2001 From: coffeys Date: Fri, 5 Nov 2010 17:15:44 +0000 Subject: [PATCH] 6957378: JMX memory leak Reviewed-by: emcmanus, kevinw --- .../remote/internal/ServerNotifForwarder.java | 35 ++- .../mandatory/notif/DeadListenerTest.java | 208 ++++++++++++++++++ 2 files changed, 242 insertions(+), 1 deletion(-) create mode 100644 test/javax/management/remote/mandatory/notif/DeadListenerTest.java diff --git a/src/share/classes/com/sun/jmx/remote/internal/ServerNotifForwarder.java b/src/share/classes/com/sun/jmx/remote/internal/ServerNotifForwarder.java index ba3165c2a..ba0714116 100644 --- a/src/share/classes/com/sun/jmx/remote/internal/ServerNotifForwarder.java +++ b/src/share/classes/com/sun/jmx/remote/internal/ServerNotifForwarder.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2002, 2008, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2002, 2010, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -45,6 +45,8 @@ import javax.management.InstanceNotFoundException; import javax.management.ListenerNotFoundException; import javax.management.MBeanPermission; import javax.management.MBeanServer; +import javax.management.MBeanServerDelegate; +import javax.management.MBeanServerNotification; import javax.management.Notification; import javax.management.NotificationBroadcaster; import javax.management.NotificationFilter; @@ -272,6 +274,7 @@ public class ServerNotifForwarder { nr = notifBuffer.fetchNotifications(bufferFilter, startSequenceNumber, t, maxNotifications); + snoopOnUnregister(nr); } catch (InterruptedException ire) { nr = new NotificationResult(0L, 0L, new TargetedNotification[0]); } @@ -283,6 +286,34 @@ public class ServerNotifForwarder { return nr; } + // The standard RMI connector client will register a listener on the MBeanServerDelegate + // in order to be told when MBeans are unregistered. We snoop on fetched notifications + // so that we can know too, and remove the corresponding entry from the listenerMap. + // See 6957378. + private void snoopOnUnregister(NotificationResult nr) { + Set delegateSet = listenerMap.get(MBeanServerDelegate.DELEGATE_NAME); + if (delegateSet == null || delegateSet.isEmpty()) { + return; + } + for (TargetedNotification tn : nr.getTargetedNotifications()) { + Integer id = tn.getListenerID(); + for (IdAndFilter idaf : delegateSet) { + if (idaf.id == id) { + // This is a notification from the MBeanServerDelegate. + Notification n = tn.getNotification(); + if (n instanceof MBeanServerNotification && + n.getType().equals(MBeanServerNotification.UNREGISTRATION_NOTIFICATION)) { + MBeanServerNotification mbsn = (MBeanServerNotification) n; + ObjectName gone = mbsn.getMBeanName(); + synchronized (listenerMap) { + listenerMap.remove(gone); + } + } + } + } + } + } + public void terminate() { if (logger.traceOn()) { logger.trace("terminate", "Be called."); @@ -418,10 +449,12 @@ public class ServerNotifForwarder { return this.filter; } + @Override public int hashCode() { return id.hashCode(); } + @Override public boolean equals(Object o) { return ((o instanceof IdAndFilter) && ((IdAndFilter) o).getId().equals(getId())); diff --git a/test/javax/management/remote/mandatory/notif/DeadListenerTest.java b/test/javax/management/remote/mandatory/notif/DeadListenerTest.java new file mode 100644 index 000000000..d5fd91e52 --- /dev/null +++ b/test/javax/management/remote/mandatory/notif/DeadListenerTest.java @@ -0,0 +1,208 @@ +/* + * Copyright (c) 2010, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +/* + * @test + * @bug 6957378 + * @summary Test that a listener can be removed remotely from an MBean that no longer exists. + * @author Eamonn McManus + */ + +import com.sun.jmx.remote.internal.ServerNotifForwarder; +import java.io.IOException; +import java.lang.management.ManagementFactory; +import java.lang.reflect.Field; +import java.lang.reflect.Method; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.concurrent.atomic.AtomicInteger; +import javax.management.ListenerNotFoundException; +import javax.management.MBeanServer; +import javax.management.MBeanServerConnection; +import javax.management.MBeanServerDelegate; +import javax.management.Notification; +import javax.management.NotificationBroadcasterSupport; +import javax.management.NotificationFilterSupport; +import javax.management.NotificationListener; +import javax.management.ObjectName; +import javax.management.remote.JMXConnector; +import javax.management.remote.JMXConnectorFactory; +import javax.management.remote.JMXServiceURL; +import javax.management.remote.rmi.RMIConnection; +import javax.management.remote.rmi.RMIConnectionImpl; +import javax.management.remote.rmi.RMIConnectorServer; +import javax.management.remote.rmi.RMIJRMPServerImpl; +import javax.security.auth.Subject; + +public class DeadListenerTest { + public static void main(String[] args) throws Exception { + final ObjectName delegateName = MBeanServerDelegate.DELEGATE_NAME; + + MBeanServer mbs = ManagementFactory.getPlatformMBeanServer(); + Noddy mbean = new Noddy(); + ObjectName name = new ObjectName("d:k=v"); + mbs.registerMBean(mbean, name); + + JMXServiceURL url = new JMXServiceURL("service:jmx:rmi:///"); + SnoopRMIServerImpl rmiServer = new SnoopRMIServerImpl(); + RMIConnectorServer cs = new RMIConnectorServer(url, null, rmiServer, mbs); + cs.start(); + JMXServiceURL addr = cs.getAddress(); + assertTrue("No connections in new connector server", rmiServer.connections.isEmpty()); + + JMXConnector cc = JMXConnectorFactory.connect(addr); + MBeanServerConnection mbsc = cc.getMBeanServerConnection(); + assertTrue("One connection on server after client connect", rmiServer.connections.size() == 1); + RMIConnectionImpl connection = rmiServer.connections.get(0); + Method getServerNotifFwdM = RMIConnectionImpl.class.getDeclaredMethod("getServerNotifFwd"); + getServerNotifFwdM.setAccessible(true); + ServerNotifForwarder serverNotifForwarder = (ServerNotifForwarder) getServerNotifFwdM.invoke(connection); + Field listenerMapF = ServerNotifForwarder.class.getDeclaredField("listenerMap"); + listenerMapF.setAccessible(true); + @SuppressWarnings("unchecked") + Map> listenerMap = (Map>) listenerMapF.get(serverNotifForwarder); + assertTrue("Server listenerMap initially empty", mapWithoutKey(listenerMap, delegateName).isEmpty()); + + CountListener count1 = new CountListener(); + mbsc.addNotificationListener(name, count1, null, null); + + CountListener count2 = new CountListener(); + NotificationFilterSupport dummyFilter = new NotificationFilterSupport(); + dummyFilter.enableType(""); + mbsc.addNotificationListener(name, count2, dummyFilter, "noddy"); + + assertTrue("One entry in listenerMap for two listeners on same MBean", mapWithoutKey(listenerMap, delegateName).size() == 1); + Set set = listenerMap.get(name); + assertTrue("Set in listenerMap for MBean has two elements", set != null && set.size() == 2); + + assertTrue("Initial value of count1 == 0", count1.count() == 0); + assertTrue("Initial value of count2 == 0", count2.count() == 0); + + Notification notif = new Notification("type", name, 0); + + mbean.sendNotification(notif); + + // Make sure notifs are working normally. + long deadline = System.currentTimeMillis() + 2000; + while ((count1.count() != 1 || count2.count() != 1) && System.currentTimeMillis() < deadline) { + Thread.sleep(10); + } + assertTrue("New value of count1 == 1", count1.count() == 1); + assertTrue("Initial value of count2 == 1", count2.count() == 1); + + // Make sure that removing a nonexistent listener from an existent MBean produces ListenerNotFoundException + CountListener count3 = new CountListener(); + try { + mbsc.removeNotificationListener(name, count3); + assertTrue("Remove of nonexistent listener succeeded but should not have", false); + } catch (ListenerNotFoundException e) { + // OK: expected + } + + // Make sure that removing a nonexistent listener from a nonexistent MBean produces ListenerNotFoundException + ObjectName nonexistent = new ObjectName("foo:bar=baz"); + assertTrue("Nonexistent is nonexistent", !mbs.isRegistered(nonexistent)); + try { + mbsc.removeNotificationListener(nonexistent, count3); + assertTrue("Remove of listener from nonexistent MBean succeeded but should not have", false); + } catch (ListenerNotFoundException e) { + // OK: expected + } + + // Now unregister our MBean, and check that notifs it sends no longer go anywhere. + mbs.unregisterMBean(name); + mbean.sendNotification(notif); + Thread.sleep(200); + assertTrue("New value of count1 == 1", count1.count() == 1); + assertTrue("Initial value of count2 == 1", count2.count() == 1); + + // Check that there is no trace of the listeners any more in ServerNotifForwarder.listenerMap. + // THIS DEPENDS ON JMX IMPLEMENTATION DETAILS. + // If the JMX implementation changes, the code here may have to change too. + Set setForUnreg = listenerMap.get(name); + assertTrue("No trace of unregistered MBean: " + setForUnreg, setForUnreg == null); + + // Remove attempts should fail. + try { + mbsc.removeNotificationListener(name, count1); + assertTrue("Remove of count1 listener should have failed", false); + } catch (ListenerNotFoundException e) { + // OK: expected + } + try { + mbsc.removeNotificationListener(name, count2, dummyFilter, "noddy"); + assertTrue("Remove of count2 listener should have failed", false); + } catch (ListenerNotFoundException e) { + // OK: expected + } + } + + private static Map mapWithoutKey(Map map, K key) { + Map copy = new HashMap(map); + copy.remove(key); + return copy; + } + + public static interface NoddyMBean {} + + public static class Noddy extends NotificationBroadcasterSupport implements NoddyMBean {} + + public static class CountListener implements NotificationListener { + final AtomicInteger count; + + public CountListener() { + this.count = new AtomicInteger(); + } + + int count() { + return count.get(); + } + + public void handleNotification(Notification notification, Object handback) { + count.incrementAndGet(); + } + } + + private static void assertTrue(String what, boolean cond) { + if (!cond) { + throw new AssertionError("Assertion failed: " + what); + } + } + + private static class SnoopRMIServerImpl extends RMIJRMPServerImpl { + final List connections = new ArrayList(); + SnoopRMIServerImpl() throws IOException { + super(0, null, null, null); + } + + @Override + protected RMIConnection makeClient(String id, Subject subject) throws IOException { + RMIConnectionImpl conn = (RMIConnectionImpl) super.makeClient(id, subject); + connections.add(conn); + return conn; + } + } +} -- GitLab