From 111537418855fd2742332e8cee78bcf415bcd1bf Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Mon, 7 Jul 2014 21:45:40 +0200 Subject: [PATCH] MBeanExporter should not implement SmartLifecycle but rather receive a ContextRefreshedEvent-like callback This commit removes the immediate package dependency cycle between the context and jmx packages. A specific callback arrangement will follow in time for 4.1 RC1; at this point, it's temporarily back to registration kicked off by afterPropertiesSet again. Issue: SPR-8045 --- .../jmx/export/MBeanExporter.java | 182 ++++++------------ .../jmx/AbstractMBeanServerTests.java | 12 +- .../jmx/export/MBeanExporterTests.java | 77 +++----- .../jmx/export/autodetectNoAutoStartup.xml | 17 -- 4 files changed, 89 insertions(+), 199 deletions(-) delete mode 100644 spring-context/src/test/resources/org/springframework/jmx/export/autodetectNoAutoStartup.xml diff --git a/spring-context/src/main/java/org/springframework/jmx/export/MBeanExporter.java b/spring-context/src/main/java/org/springframework/jmx/export/MBeanExporter.java index 7ed30c42eca..44e6b63eb59 100644 --- a/spring-context/src/main/java/org/springframework/jmx/export/MBeanExporter.java +++ b/spring-context/src/main/java/org/springframework/jmx/export/MBeanExporter.java @@ -50,7 +50,6 @@ import org.springframework.beans.factory.InitializingBean; import org.springframework.beans.factory.ListableBeanFactory; import org.springframework.beans.factory.config.ConfigurableBeanFactory; import org.springframework.beans.factory.config.ConfigurableListableBeanFactory; -import org.springframework.context.SmartLifecycle; import org.springframework.core.Constants; import org.springframework.jmx.export.assembler.AutodetectCapableMBeanInfoAssembler; import org.springframework.jmx.export.assembler.MBeanInfoAssembler; @@ -99,7 +98,7 @@ import org.springframework.util.ObjectUtils; * @see MBeanExporterListener */ public class MBeanExporter extends MBeanRegistrationSupport implements MBeanExportOperations, - BeanClassLoaderAware, BeanFactoryAware, InitializingBean, DisposableBean, SmartLifecycle { + BeanClassLoaderAware, BeanFactoryAware, InitializingBean, DisposableBean { /** * Autodetection mode indicating that no autodetection should be used. @@ -154,6 +153,12 @@ public class MBeanExporter extends MBeanRegistrationSupport implements MBeanExpo /** The strategy to use for creating ObjectNames for an object */ private ObjectNamingStrategy namingStrategy = new KeyNamingStrategy(); + /** Indicates whether Spring should modify generated ObjectNames */ + private boolean ensureUniqueRuntimeObjectNames = true; + + /** Indicates whether Spring should expose the managed resource ClassLoader in the MBean */ + private boolean exposeManagedResourceClassLoader = true; + /** A set of bean names that should be excluded from autodetection */ private Set excludedBeans; @@ -167,28 +172,12 @@ public class MBeanExporter extends MBeanRegistrationSupport implements MBeanExpo private final Map registeredNotificationListeners = new LinkedHashMap(); - /** Indicates whether Spring should modify generated ObjectNames */ - private boolean ensureUniqueRuntimeObjectNames = true; - - /** Indicates whether Spring should expose the managed resource ClassLoader in the MBean */ - private boolean exposeManagedResourceClassLoader = true; - - /** Indicate whether to auto-startup within the container-managed lifecycle */ - private boolean autoStartup = true; - - /** Indicate the phase to use within the container-managed lifecycle */ - private int phase = Integer.MAX_VALUE; - /** Stores the ClassLoader to use for generating lazy-init proxies */ private ClassLoader beanClassLoader = ClassUtils.getDefaultClassLoader(); /** Stores the BeanFactory for use in autodetection process */ private ListableBeanFactory beanFactory; - private boolean running = false; - - private final Object lifecycleMonitor = new Object(); - /** * Supply a {@code Map} of beans to be registered with the JMX @@ -295,6 +284,31 @@ public class MBeanExporter extends MBeanRegistrationSupport implements MBeanExpo this.namingStrategy = namingStrategy; } + /** + * Indicates whether Spring should ensure that {@link ObjectName ObjectNames} + * generated by the configured {@link ObjectNamingStrategy} for + * runtime-registered MBeans ({@link #registerManagedResource}) should get + * modified: to ensure uniqueness for every instance of a managed {@code Class}. + *

The default value is {@code true}. + * @see #registerManagedResource + * @see JmxUtils#appendIdentityToObjectName(javax.management.ObjectName, Object) + */ + public void setEnsureUniqueRuntimeObjectNames(boolean ensureUniqueRuntimeObjectNames) { + this.ensureUniqueRuntimeObjectNames = ensureUniqueRuntimeObjectNames; + } + + /** + * Indicates whether or not the managed resource should be exposed on the + * {@link Thread#getContextClassLoader() thread context ClassLoader} before + * allowing any invocations on the MBean to occur. + *

The default value is {@code true}, exposing a {@link SpringModelMBean} + * which performs thread context ClassLoader management. Switch this flag off to + * expose a standard JMX {@link javax.management.modelmbean.RequiredModelMBean}. + */ + public void setExposeManagedResourceClassLoader(boolean exposeManagedResourceClassLoader) { + this.exposeManagedResourceClassLoader = exposeManagedResourceClassLoader; + } + /** * Set the list of names for beans that should be excluded from autodetection. */ @@ -358,61 +372,6 @@ public class MBeanExporter extends MBeanRegistrationSupport implements MBeanExpo notificationListeners.toArray(new NotificationListenerBean[notificationListeners.size()]); } - /** - * Indicates whether Spring should ensure that {@link ObjectName ObjectNames} - * generated by the configured {@link ObjectNamingStrategy} for - * runtime-registered MBeans ({@link #registerManagedResource}) should get - * modified: to ensure uniqueness for every instance of a managed {@code Class}. - *

The default value is {@code true}. - * @see #registerManagedResource - * @see JmxUtils#appendIdentityToObjectName(javax.management.ObjectName, Object) - */ - public void setEnsureUniqueRuntimeObjectNames(boolean ensureUniqueRuntimeObjectNames) { - this.ensureUniqueRuntimeObjectNames = ensureUniqueRuntimeObjectNames; - } - - /** - * Indicates whether or not the managed resource should be exposed on the - * {@link Thread#getContextClassLoader() thread context ClassLoader} before - * allowing any invocations on the MBean to occur. - *

The default value is {@code true}, exposing a {@link SpringModelMBean} - * which performs thread context ClassLoader management. Switch this flag off to - * expose a standard JMX {@link javax.management.modelmbean.RequiredModelMBean}. - */ - public void setExposeManagedResourceClassLoader(boolean exposeManagedResourceClassLoader) { - this.exposeManagedResourceClassLoader = exposeManagedResourceClassLoader; - } - - /** - * Set whether to automatically export MBeans after initialization. - *

Default is "true"; set this to "false" to allow for manual startup - * through the {@link #start()} method. - */ - public void setAutoStartup(boolean autoStartup) { - this.autoStartup = autoStartup; - } - - @Override - public boolean isAutoStartup() { - return this.autoStartup; - } - - /** - * Specify the phase in which the MBeans should be exported to the - * JMX domain. The startup order proceeds from lowest to highest, and - * the shutdown order is the reverse of that. By default this value - * is {@code Integer.MAX_VALUE} meaning that MBeans are exported - * as late as possible and removed from the domain as soon as possible. - */ - public void setPhase(int phase) { - this.phase = phase; - } - - @Override - public int getPhase() { - return this.phase; - } - @Override public void setBeanClassLoader(ClassLoader classLoader) { this.beanClassLoader = classLoader; @@ -436,6 +395,11 @@ public class MBeanExporter extends MBeanRegistrationSupport implements MBeanExpo } } + + //--------------------------------------------------------------------- + // Lifecycle in bean factory: automatically register/unregister beans + //--------------------------------------------------------------------- + @Override public void afterPropertiesSet() { // If no server was provided then try to find one. This is useful in an environment @@ -443,62 +407,36 @@ public class MBeanExporter extends MBeanRegistrationSupport implements MBeanExpo if (this.server == null) { this.server = JmxUtils.locateMBeanServer(); } + register(); // TODO: to be replaced with some ContextRefreshedEvent-like callback } - - //--------------------------------------------------------------------- - // Implementation of SmartLifecycle interface - //--------------------------------------------------------------------- - - @Override - public void start() { - synchronized (this.lifecycleMonitor) { - try { - registerBeans(); - registerNotificationListeners(); - } - catch (RuntimeException ex) { - // Unregister beans already registered by this exporter. - doStop(); - throw ex; - } - this.running = true; - } - } - - @Override - public void stop() { - synchronized (this.lifecycleMonitor) { - doStop(); - } - } - - @Override - public void stop(Runnable callback) { - synchronized (this.lifecycleMonitor) { - doStop(); - callback.run(); - } - } - - @Override - public boolean isRunning() { - synchronized (this.lifecycleMonitor) { - return this.running; + /** + * Kick off bean registration automatically when deployed in an {@code ApplicationContext}. + * @see #registerBeans() + */ + public void register() { + try { + logger.info("Registering beans for JMX exposure on startup"); + registerBeans(); + registerNotificationListeners(); + } + catch (RuntimeException ex) { + // Unregister beans already registered by this exporter. + unregisterNotificationListeners(); + unregisterBeans(); + throw ex; } } + /** + * Unregisters all beans that this exported has exposed via JMX + * when the enclosing {@code ApplicationContext} is destroyed. + */ @Override public void destroy() { - synchronized (this.lifecycleMonitor) { - doStop(); - } - } - - private void doStop() { + logger.info("Unregistering JMX-exposed beans on shutdown"); unregisterNotificationListeners(); unregisterBeans(); - this.running = false; } @@ -568,8 +506,6 @@ public class MBeanExporter extends MBeanRegistrationSupport implements MBeanExpo * implementation of the {@code ObjectNamingStrategy} interface being used. */ protected void registerBeans() { - logger.info("Registering beans for JMX exposure"); - // The beans property may be null, for example if we are relying solely on autodetection. if (this.beans == null) { this.beans = new HashMap(); @@ -587,7 +523,7 @@ public class MBeanExporter extends MBeanRegistrationSupport implements MBeanExpo } if (mode == AUTODETECT_MBEAN || mode == AUTODETECT_ALL) { // Autodetect any beans that are already MBeans. - logger.info("Autodetecting user-defined JMX MBeans"); + logger.debug("Autodetecting user-defined JMX MBeans"); autodetectMBeans(); } // Allow the assembler a chance to vote for bean inclusion. diff --git a/spring-context/src/test/java/org/springframework/jmx/AbstractMBeanServerTests.java b/spring-context/src/test/java/org/springframework/jmx/AbstractMBeanServerTests.java index 9676a69adf0..87984ab7378 100644 --- a/spring-context/src/test/java/org/springframework/jmx/AbstractMBeanServerTests.java +++ b/spring-context/src/test/java/org/springframework/jmx/AbstractMBeanServerTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2013 the original author or authors. + * Copyright 2002-2014 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -22,6 +22,7 @@ import javax.management.ObjectName; import org.junit.After; import org.junit.Before; + import org.springframework.beans.factory.xml.XmlBeanDefinitionReader; import org.springframework.context.ConfigurableApplicationContext; import org.springframework.context.support.GenericApplicationContext; @@ -52,12 +53,14 @@ public abstract class AbstractMBeanServerTests { protected MBeanServer server; + @Before public final void setUp() throws Exception { this.server = MBeanServerFactory.createMBeanServer(); try { onSetUp(); - } catch (Exception ex) { + } + catch (Exception ex) { releaseServer(); throw ex; } @@ -94,13 +97,10 @@ public abstract class AbstractMBeanServerTests { /** * Start the specified {@link MBeanExporter}. - * - * @see org.springframework.beans.factory.InitializingBean#afterPropertiesSet() - * @see org.springframework.context.Lifecycle#start() */ protected void start(MBeanExporter exporter) { exporter.afterPropertiesSet(); - exporter.start(); + // exporter.register(); } protected void assertIsRegistered(String message, ObjectName objectName) { diff --git a/spring-context/src/test/java/org/springframework/jmx/export/MBeanExporterTests.java b/spring-context/src/test/java/org/springframework/jmx/export/MBeanExporterTests.java index 532034b64c4..4afc0f58350 100644 --- a/spring-context/src/test/java/org/springframework/jmx/export/MBeanExporterTests.java +++ b/spring-context/src/test/java/org/springframework/jmx/export/MBeanExporterTests.java @@ -21,7 +21,6 @@ import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; - import javax.management.Attribute; import javax.management.InstanceNotFoundException; import javax.management.JMException; @@ -40,11 +39,8 @@ import org.junit.rules.ExpectedException; import org.springframework.aop.framework.ProxyFactory; import org.springframework.beans.factory.support.BeanDefinitionBuilder; import org.springframework.beans.factory.support.DefaultListableBeanFactory; -import org.springframework.beans.factory.xml.XmlBeanDefinitionReader; import org.springframework.context.ConfigurableApplicationContext; import org.springframework.context.support.ClassPathXmlApplicationContext; -import org.springframework.context.support.GenericApplicationContext; -import org.springframework.core.io.ClassPathResource; import org.springframework.jmx.AbstractMBeanServerTests; import org.springframework.jmx.IJmxTestBean; import org.springframework.jmx.JmxTestBean; @@ -78,22 +74,10 @@ public final class MBeanExporterTests extends AbstractMBeanServerTests { private static final String OBJECT_NAME = "spring:test=jmxMBeanAdaptor"; - @SuppressWarnings({ "rawtypes", "unchecked" }) - @Test - public void testRegisterNonNotificationListenerType() throws Exception { - Map listeners = new HashMap(); - // put a non-NotificationListener instance in as a value... - listeners.put("*", this); - MBeanExporter exporter = new MBeanExporter(); - thrown.expect(ClassCastException.class); - exporter.setNotificationListenerMappings(listeners); - } - - @SuppressWarnings({ "rawtypes", "unchecked" }) @Test public void testRegisterNullNotificationListenerType() throws Exception { - Map listeners = new HashMap(); + Map listeners = new HashMap(); // put null in as a value... listeners.put("*", null); MBeanExporter exporter = new MBeanExporter(); @@ -102,10 +86,9 @@ public final class MBeanExporterTests extends AbstractMBeanServerTests { exporter.setNotificationListenerMappings(listeners); } - @SuppressWarnings({ "rawtypes", "unchecked" }) @Test public void testRegisterNotificationListenerForNonExistentMBean() throws Exception { - Map listeners = new HashMap(); + Map listeners = new HashMap(); NotificationListener dummyListener = new NotificationListener() { @Override public void handleNotification(Notification notification, Object handback) { @@ -139,7 +122,7 @@ public final class MBeanExporterTests extends AbstractMBeanServerTests { ObjectNameManager.getInstance(OBJECT_NAME)); } finally { - exporter.stop(); + exporter.destroy(); } } @@ -162,7 +145,7 @@ public final class MBeanExporterTests extends AbstractMBeanServerTests { assertFalse("Assembler should not have been invoked", asm.invoked); } finally { - exporter.stop(); + exporter.destroy(); } } @@ -178,7 +161,8 @@ public final class MBeanExporterTests extends AbstractMBeanServerTests { assertNotNull(instance); instance = server.getObjectInstance(ObjectNameManager.getInstance("spring:mbean3=true")); assertNotNull(instance); - } finally { + } + finally { ctx.close(); } } @@ -194,7 +178,8 @@ public final class MBeanExporterTests extends AbstractMBeanServerTests { thrown.expect(InstanceNotFoundException.class); server.getObjectInstance(ObjectNameManager.getInstance("spring:mbean=false")); - } finally { + } + finally { ctx.close(); } } @@ -215,7 +200,8 @@ public final class MBeanExporterTests extends AbstractMBeanServerTests { assertNotNull(server.getObjectInstance(oname)); name = (String) server.getAttribute(oname, "Name"); assertEquals("Invalid name returned", "Juergen Hoeller", name); - } finally { + } + finally { ctx.close(); } } @@ -225,31 +211,8 @@ public final class MBeanExporterTests extends AbstractMBeanServerTests { ConfigurableApplicationContext ctx = load("autodetectNoMBeans.xml"); try { ctx.getBean("exporter"); - } finally { - ctx.close(); } - } - - @Test - public void testAutoStartupToFalse() throws Exception { - ConfigurableApplicationContext ctx = load("autodetectNoAutoStartup.xml"); - try { - MBeanExporter exporter = ctx.getBean("exporter", MBeanExporter.class); - MBeanServer server = ctx.getBean("server", MBeanServer.class); - - ObjectName on = ObjectNameManager.getInstance("spring:mbean=true"); - try { - server.getObjectInstance(on); - fail("MBeans should not have been exported with autoStartup set to false"); - } - catch (InstanceNotFoundException e) { - // expected - } - - // Export manually - exporter.start(); - assertNotNull(server.getObjectInstance(on)); // Should be exposed now. - } finally { + finally { ctx.close(); } } @@ -262,9 +225,9 @@ public final class MBeanExporterTests extends AbstractMBeanServerTests { MBeanExporter exporter = new MBeanExporter(); exporter.setBeans(getBeanMap()); exporter.setServer(server); - exporter.setListeners(new MBeanExporterListener[] { listener1, listener2 }); + exporter.setListeners(listener1, listener2); start(exporter); - exporter.stop(); + exporter.destroy(); assertListener(listener1); assertListener(listener2); @@ -279,7 +242,7 @@ public final class MBeanExporterTests extends AbstractMBeanServerTests { ProxyFactory factory = new ProxyFactory(); factory.setTarget(bean); factory.addAdvice(new NopInterceptor()); - factory.setInterfaces(new Class[] { IJmxTestBean.class }); + factory.setInterfaces(IJmxTestBean.class); IJmxTestBean proxy = (IJmxTestBean) factory.getProxy(); String name = "bean:mmm=whatever"; @@ -585,13 +548,13 @@ public final class MBeanExporterTests extends AbstractMBeanServerTests { exporter.setBeans(getBeanMap()); exporter.setServer(this.server); MockMBeanExporterListener listener = new MockMBeanExporterListener(); - exporter.setListeners(new MBeanExporterListener[] { listener }); + exporter.setListeners(listener); start(exporter); assertIsRegistered("The bean was not registered with the MBeanServer", ObjectNameManager.getInstance(OBJECT_NAME)); this.server.unregisterMBean(new ObjectName(OBJECT_NAME)); - exporter.stop(); + exporter.destroy(); assertEquals("Listener should not have been invoked (MBean previously unregistered by external agent)", 0, listener.getUnregistered().size()); } @@ -698,6 +661,7 @@ public final class MBeanExporterTests extends AbstractMBeanServerTests { assertEquals("Incorrect ObjectName in unregister", desired, listener.getUnregistered().get(0)); } + private static class InvokeDetectAssembler implements MBeanInfoAssembler { private boolean invoked = false; @@ -709,6 +673,7 @@ public final class MBeanExporterTests extends AbstractMBeanServerTests { } } + private static class MockMBeanExporterListener implements MBeanExporterListener { private List registered = new ArrayList(); @@ -734,6 +699,7 @@ public final class MBeanExporterTests extends AbstractMBeanServerTests { } } + private static class SelfNamingTestBean implements SelfNaming { private ObjectName objectName; @@ -748,11 +714,13 @@ public final class MBeanExporterTests extends AbstractMBeanServerTests { } } + public static interface PersonMBean { String getName(); } + public static class Person implements PersonMBean { private String name; @@ -767,6 +735,7 @@ public final class MBeanExporterTests extends AbstractMBeanServerTests { } } + public static final class StubNotificationListener implements NotificationListener { private List notifications = new ArrayList(); @@ -781,6 +750,7 @@ public final class MBeanExporterTests extends AbstractMBeanServerTests { } } + private static class RuntimeExceptionThrowingConstructorBean { @SuppressWarnings("unused") @@ -789,6 +759,7 @@ public final class MBeanExporterTests extends AbstractMBeanServerTests { } } + private static final class NamedBeanAutodetectCapableMBeanInfoAssemblerStub extends SimpleReflectiveMBeanInfoAssembler implements AutodetectCapableMBeanInfoAssembler { diff --git a/spring-context/src/test/resources/org/springframework/jmx/export/autodetectNoAutoStartup.xml b/spring-context/src/test/resources/org/springframework/jmx/export/autodetectNoAutoStartup.xml deleted file mode 100644 index d385c07c485..00000000000 --- a/spring-context/src/test/resources/org/springframework/jmx/export/autodetectNoAutoStartup.xml +++ /dev/null @@ -1,17 +0,0 @@ - - - - - - - - - - - - - - - \ No newline at end of file