From eaa28a9dbf73fc4d0ec84f09f31095049818ce96 Mon Sep 17 00:00:00 2001 From: Gary Russell Date: Thu, 31 Oct 2013 15:37:50 +0200 Subject: [PATCH] INT-3186 Fix Metrics For Direct MessageHandler When a simple MessageHandler is referenced from a ``, it is wrapped in an anonymous AbstractReplyProducingMessageHandler to facilitate proper handling of the `` and so that normal 'produced no reply` messages are logged. Previously, this caused 2 MBeans to be exposed, one for tha actual handler; one for the wrapper. This added more stack frames between the dispatcher and the handler (because 2 sets of statistics were captured). The root cause was that the MBean exporter was not aware of the relationship between these objects. https://jira.springsource.org/browse/INT-3186 Add code to the IntegrationMBeanExporter to suppress adding metrics to the wrapper. This involves detecting that it is a wrapper and not exposing it as an MBean and, when matching handlers with endpoints, checking that the wrapped handler is the actual handler for the endpoint. Update test to reflect the reduced number of stack frames between the dispatcher and handler. Also add tests to verify the detection of multiple references to the same ARPMH. INT-3186: `IntegrationMBeanExporter` optimization --- .../config/ServiceActivatorFactoryBean.java | 2 +- ...torDefaultFrameworkMethodTests-context.xml | 6 +- ...faultFrameworkMethodTests-fail-context.xml | 19 ++++++ ...eActivatorDefaultFrameworkMethodTests.java | 21 +++++++ .../monitor/IntegrationMBeanExporter.java | 61 +++++++++++++++---- ...torDefaultFrameworkMethodTests-context.xml | 6 +- ...faultFrameworkMethodTests-fail-context.xml | 24 ++++++++ ...eActivatorDefaultFrameworkMethodTests.java | 24 +++++++- 8 files changed, 144 insertions(+), 19 deletions(-) create mode 100644 spring-integration-core/src/test/java/org/springframework/integration/handler/ServiceActivatorDefaultFrameworkMethodTests-fail-context.xml create mode 100644 spring-integration-jmx/src/test/java/org/springframework/integration/jmx/ServiceActivatorDefaultFrameworkMethodTests-fail-context.xml diff --git a/spring-integration-core/src/main/java/org/springframework/integration/config/ServiceActivatorFactoryBean.java b/spring-integration-core/src/main/java/org/springframework/integration/config/ServiceActivatorFactoryBean.java index 77932a7973..5401c17422 100644 --- a/spring-integration-core/src/main/java/org/springframework/integration/config/ServiceActivatorFactoryBean.java +++ b/spring-integration-core/src/main/java/org/springframework/integration/config/ServiceActivatorFactoryBean.java @@ -73,7 +73,7 @@ public class ServiceActivatorFactoryBean extends AbstractStandardMessageHandlerF } /* * Return a reply-producing message handler so that we still get 'produced no reply' messages - * and the super class will inject the advice chain to advise the handler if needed. + * and the super class will inject the advice chain to advise the handler method if needed. */ handler = new AbstractReplyProducingMessageHandler() { diff --git a/spring-integration-core/src/test/java/org/springframework/integration/handler/ServiceActivatorDefaultFrameworkMethodTests-context.xml b/spring-integration-core/src/test/java/org/springframework/integration/handler/ServiceActivatorDefaultFrameworkMethodTests-context.xml index 5b2da86780..508e3cc386 100644 --- a/spring-integration-core/src/test/java/org/springframework/integration/handler/ServiceActivatorDefaultFrameworkMethodTests-context.xml +++ b/spring-integration-core/src/test/java/org/springframework/integration/handler/ServiceActivatorDefaultFrameworkMethodTests-context.xml @@ -22,19 +22,19 @@ - - - diff --git a/spring-integration-core/src/test/java/org/springframework/integration/handler/ServiceActivatorDefaultFrameworkMethodTests-fail-context.xml b/spring-integration-core/src/test/java/org/springframework/integration/handler/ServiceActivatorDefaultFrameworkMethodTests-fail-context.xml new file mode 100644 index 0000000000..abbc47f5fb --- /dev/null +++ b/spring-integration-core/src/test/java/org/springframework/integration/handler/ServiceActivatorDefaultFrameworkMethodTests-fail-context.xml @@ -0,0 +1,19 @@ + + + + + + + + + + diff --git a/spring-integration-core/src/test/java/org/springframework/integration/handler/ServiceActivatorDefaultFrameworkMethodTests.java b/spring-integration-core/src/test/java/org/springframework/integration/handler/ServiceActivatorDefaultFrameworkMethodTests.java index 4756857945..f788c76c3c 100644 --- a/spring-integration-core/src/test/java/org/springframework/integration/handler/ServiceActivatorDefaultFrameworkMethodTests.java +++ b/spring-integration-core/src/test/java/org/springframework/integration/handler/ServiceActivatorDefaultFrameworkMethodTests.java @@ -18,11 +18,16 @@ package org.springframework.integration.handler; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.fail; +import org.hamcrest.Matchers; import org.junit.Test; import org.junit.runner.RunWith; +import org.springframework.beans.factory.BeanCreationException; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.context.support.ClassPathXmlApplicationContext; import org.springframework.integration.Message; import org.springframework.integration.MessageChannel; import org.springframework.integration.channel.QueueChannel; @@ -149,6 +154,22 @@ public class ServiceActivatorDefaultFrameworkMethodTests { assertEquals("processorTestInputChannel,processorTestService", reply.getHeaders().get("history").toString()); } + @Test + public void testFailOnDoubleReference() { + try { + new ClassPathXmlApplicationContext(this.getClass().getSimpleName() + "-fail-context.xml", + this.getClass()); + fail("Expected exception due to 2 endpoints referencing the same bean"); + } + catch (Exception e) { + assertThat(e, Matchers.instanceOf(BeanCreationException.class)); + assertThat(e.getCause(), Matchers.instanceOf(BeanCreationException.class)); + assertThat(e.getCause().getCause(), Matchers.instanceOf(IllegalArgumentException.class)); + assertThat(e.getCause().getCause().getMessage(), + Matchers.containsString("An AbstractReplyProducingMessageHandler may only be referenced once")); + } + + } @SuppressWarnings("unused") private static class TestReplyingMessageHandler extends AbstractReplyProducingMessageHandler { diff --git a/spring-integration-jmx/src/main/java/org/springframework/integration/monitor/IntegrationMBeanExporter.java b/spring-integration-jmx/src/main/java/org/springframework/integration/monitor/IntegrationMBeanExporter.java index dcf5433cab..634aeede57 100644 --- a/spring-integration-jmx/src/main/java/org/springframework/integration/monitor/IntegrationMBeanExporter.java +++ b/spring-integration-jmx/src/main/java/org/springframework/integration/monitor/IntegrationMBeanExporter.java @@ -27,6 +27,7 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.atomic.AtomicReference; import java.util.concurrent.locks.ReentrantLock; import javax.management.DynamicMBean; @@ -80,6 +81,8 @@ import org.springframework.scheduling.concurrent.ThreadPoolTaskScheduler; import org.springframework.util.Assert; import org.springframework.util.PatternMatchUtils; import org.springframework.util.ReflectionUtils; +import org.springframework.util.ReflectionUtils.FieldCallback; +import org.springframework.util.ReflectionUtils.FieldFilter; /** *

@@ -106,6 +109,7 @@ import org.springframework.util.ReflectionUtils; * @author Helena Edelson * @author Oleg Zhurakousky * @author Gary Russell + * @author Artem Bilan */ @ManagedResource public class IntegrationMBeanExporter extends MBeanExporter implements BeanPostProcessor, BeanFactoryAware, @@ -217,7 +221,7 @@ public class IntegrationMBeanExporter extends MBeanExporter implements BeanPostP @Override public void setBeanFactory(BeanFactory beanFactory) throws BeansException { super.setBeanFactory(beanFactory); - Assert.isTrue(beanFactory instanceof ListableBeanFactory, "A ListableBeanFactory is required."); + Assert.isInstanceOf(ListableBeanFactory.class, beanFactory, "A ListableBeanFactory is required."); this.beanFactory = (ListableBeanFactory) beanFactory; } @@ -246,6 +250,12 @@ public class IntegrationMBeanExporter extends MBeanExporter implements BeanPostP } if (bean instanceof MessageHandler) { + if (this.handlerInAnonymousWrapper(bean) != null) { + if (logger.isDebugEnabled()) { + logger.debug("Skipping " + beanName + " because it wraps another handler"); + } + return bean; + } SimpleMessageHandlerMetrics monitor = new SimpleMessageHandlerMetrics((MessageHandler) bean); Object advised = applyHandlerInterceptor(bean, monitor, beanClassLoader); handlers.add(monitor); @@ -282,6 +292,33 @@ public class IntegrationMBeanExporter extends MBeanExporter implements BeanPostP } + private MessageHandler handlerInAnonymousWrapper(final Object bean) { + if (bean != null && bean.getClass().isAnonymousClass()) { + final AtomicReference wrapped = new AtomicReference(); + ReflectionUtils.doWithFields(bean.getClass(), new FieldCallback() { + + @Override + public void doWith(Field field) throws IllegalArgumentException, IllegalAccessException { + field.setAccessible(true); + Object handler = field.get(bean); + if (handler instanceof MessageHandler) { + wrapped.set((MessageHandler) handler); + } + } + }, new FieldFilter() { + + @Override + public boolean matches(Field field) { + return wrapped.get() == null && field.getName().startsWith("val$"); + } + }); + return wrapped.get(); + } + else { + return null; + } + } + /** * Copy of private method in super class. Needed so we can avoid using the bean factory to extract the bean again, * and risk it being a proxy (which it almost certainly is by now). @@ -987,20 +1024,22 @@ public class IntegrationMBeanExporter extends MBeanExporter implements BeanPostP String source = "endpoint"; Object endpoint = null; + MessageHandler messageHandler = monitor.getMessageHandler(); + for (String beanName : names) { endpoint = beanFactory.getBean(beanName); - Object field = null; try { - field = extractTarget(getField(endpoint, "handler")); + Object field = extractTarget(getField(endpoint, "handler")); + if (field == messageHandler || + this.extractTarget(this.handlerInAnonymousWrapper(field)) == messageHandler) { + name = beanName; + endpointName = beanName; + break; + } } catch (Exception e) { logger.trace("Could not get handler from bean = " + beanName); } - if (field == monitor.getMessageHandler()) { - name = beanName; - endpointName = beanName; - break; - } } if (name != null && endpoint != null && name.startsWith("_org.springframework.integration")) { name = getInternalComponentName(name); @@ -1044,11 +1083,11 @@ public class IntegrationMBeanExporter extends MBeanExporter implements BeanPostP } if (name == null) { - if (monitor.getMessageHandler() instanceof NamedComponent) { - name = ((NamedComponent) monitor.getMessageHandler()).getComponentName(); + if (messageHandler instanceof NamedComponent) { + name = ((NamedComponent) messageHandler).getComponentName(); } if (name == null) { - name = monitor.getMessageHandler().toString(); + name = messageHandler.toString(); } source = "handler"; } diff --git a/spring-integration-jmx/src/test/java/org/springframework/integration/jmx/ServiceActivatorDefaultFrameworkMethodTests-context.xml b/spring-integration-jmx/src/test/java/org/springframework/integration/jmx/ServiceActivatorDefaultFrameworkMethodTests-context.xml index a222fe42dc..0e5a5f7c13 100644 --- a/spring-integration-jmx/src/test/java/org/springframework/integration/jmx/ServiceActivatorDefaultFrameworkMethodTests-context.xml +++ b/spring-integration-jmx/src/test/java/org/springframework/integration/jmx/ServiceActivatorDefaultFrameworkMethodTests-context.xml @@ -27,19 +27,19 @@ - - - diff --git a/spring-integration-jmx/src/test/java/org/springframework/integration/jmx/ServiceActivatorDefaultFrameworkMethodTests-fail-context.xml b/spring-integration-jmx/src/test/java/org/springframework/integration/jmx/ServiceActivatorDefaultFrameworkMethodTests-fail-context.xml new file mode 100644 index 0000000000..942235110a --- /dev/null +++ b/spring-integration-jmx/src/test/java/org/springframework/integration/jmx/ServiceActivatorDefaultFrameworkMethodTests-fail-context.xml @@ -0,0 +1,24 @@ + + + + + + + + + + + + + diff --git a/spring-integration-jmx/src/test/java/org/springframework/integration/jmx/ServiceActivatorDefaultFrameworkMethodTests.java b/spring-integration-jmx/src/test/java/org/springframework/integration/jmx/ServiceActivatorDefaultFrameworkMethodTests.java index 90ad33084d..0f8db9084a 100644 --- a/spring-integration-jmx/src/test/java/org/springframework/integration/jmx/ServiceActivatorDefaultFrameworkMethodTests.java +++ b/spring-integration-jmx/src/test/java/org/springframework/integration/jmx/ServiceActivatorDefaultFrameworkMethodTests.java @@ -18,11 +18,16 @@ package org.springframework.integration.jmx; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.fail; +import org.hamcrest.Matchers; import org.junit.Test; import org.junit.runner.RunWith; +import org.springframework.beans.factory.BeanCreationException; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.context.support.ClassPathXmlApplicationContext; import org.springframework.integration.Message; import org.springframework.integration.MessageChannel; import org.springframework.integration.channel.QueueChannel; @@ -151,6 +156,23 @@ public class ServiceActivatorDefaultFrameworkMethodTests { assertEquals("processorTestInputChannel,processorTestService", reply.getHeaders().get("history").toString()); } + @Test + public void testFailOnDoubleReference() { + try { + new ClassPathXmlApplicationContext(this.getClass().getSimpleName() + "-fail-context.xml", + this.getClass()); + fail("Expected exception due to 2 endpoints referencing the same bean"); + } + catch (Exception e) { + assertThat(e, Matchers.instanceOf(BeanCreationException.class)); + assertThat(e.getCause(), Matchers.instanceOf(BeanCreationException.class)); + assertThat(e.getCause().getCause(), Matchers.instanceOf(IllegalArgumentException.class)); + assertThat(e.getCause().getCause().getMessage(), + Matchers.containsString("An AbstractReplyProducingMessageHandler may only be referenced once")); + } + + } + private interface Foo { public String foo(String in); @@ -181,7 +203,7 @@ public class ServiceActivatorDefaultFrameworkMethodTests { public void handleMessage(Message requestMessage) { Exception e = new RuntimeException(); StackTraceElement[] st = e.getStackTrace(); - assertEquals("doDispatch", st[28].getMethodName()); + assertEquals("doDispatch", st[16].getMethodName()); } }