INT-4218: MessagingMethodInvokerHelp Improvements

JIRA: https://jira.spring.io/browse/INT-4218

* Add `InvocableHandlerMethod` invocation threshold logic to give up eventually in favor of permanent expression evaluation
* Add `ParametersWrapper.toString()` for better logging messages
* Log `InvocableHandlerMethod` invocation failure message in favor of SpEL only once when `failedAttempts` is exceeded already
* Switch to `spelOnly` mode after that
* Refactor `MessagingMethodInvokerHelper.processInternal()` to separate SpEL or Ivocable logic via their own invocation methods
* Move `MessagingMethodInvokerHelper.HandlerMethod` static fields to the `MessagingMethodInvokerHelper` level since it doesn't matter from this class perspective but can be reused in other places from top level of the `MessagingMethodInvokerHelper` class
* Catch `IllegalArgumentException` with the `java.lang.ClassCastException@...` message.
See http://stackoverflow.com/questions/16042591/reflections-illegalargumentexception-causes for more info

Address PR comments

* Make `handlerMethod.failedAttempts` conditional expression as `>=` to avoid race conditions in multi-threaded environment
* Analyze StackTrace for the class of the `IllegalStateExpception` avoid SpEL fall back in case of user exception, not reflection invocation
* Fix race condition in the `TcpNioConnectionTests`, when atomic value is set after `latch.countDown()`

spring-integration-core/src/main/java/org/springframework/integration/util/MessagingMethodInvokerHelper.java

@@ -108,6 +108,25 @@ public class MessagingMethodInvokerHelper<T> extends AbstractExpressionEvaluator
 
 	private static final Log logger = LogFactory.getLog(MessagingMethodInvokerHelper.class);
 
+	// Number of times to try an InvocableHandlerMethod before giving up in favor of an expression.
+	private static final int FAILED_ATTEMPTS_THRESHOLD = 100;
+
+	private static final SpelExpressionParser EXPRESSION_PARSER = new SpelExpressionParser();
+
+	private static final ParameterNameDiscoverer PARAMETER_NAME_DISCOVERER =
+			new LocalVariableTableParameterNameDiscoverer();
+
+	private static final TypeDescriptor messageTypeDescriptor = TypeDescriptor.valueOf(Message.class);
+
+	@SuppressWarnings("unused")
+	private static final Collection<Message<?>> dummyMessages = Collections.emptyList();
+
+	private static final TypeDescriptor messageListTypeDescriptor = new TypeDescriptor(
+			ReflectionUtils.findField(MessagingMethodInvokerHelper.class, "dummyMessages"));
+
+	private static final TypeDescriptor messageArrayTypeDescriptor = TypeDescriptor.valueOf(Message[].class);
+
+
 	private final DefaultMessageHandlerMethodFactory messageHandlerMethodFactory =
 			new DefaultMessageHandlerMethodFactory();
 
@@ -390,37 +409,13 @@ public class MessagingMethodInvokerHelper<T> extends AbstractExpressionEvaluator
 		Expression expression = candidate.expression;
 		Assert.notNull(candidate, "No candidate methods found for messages.");
 
-		T result = null;
-		try {
-			if (this.useSpelInvoker || candidate.spelOnly) {
-				result = invokeExpression(expression, parameters);
-			}
-			else {
-				result = candidate.invoke(parameters);
-			}
-		}
-		catch (MethodArgumentResolutionException | MessageConversionException | IllegalStateException e) {
-			if (e instanceof MessageConversionException) {
-				if (e.getCause() instanceof ConversionFailedException &&
-						!(e.getCause().getCause() instanceof ConverterNotFoundException)) {
-					throw e;
-				}
-			}
-			else if (e instanceof IllegalStateException) {
-				if (e.getCause() instanceof IllegalArgumentException
-						&& !"argument type mismatch".equals(e.getCause().getMessage())) {
-					throw e;
-				}
-			}
-			if (logger.isInfoEnabled()) {
-				logger.info("Failed to invoke [ " + candidate.invocableHandlerMethod +
-						"] with provided arguments [ " + parameters + " ]. \n" +
-						"Falling back to SpEL invocation for expression [ " +
-						expression.getExpressionString() + " ]");
-			}
-
+		T result;
+		if (this.useSpelInvoker || candidate.spelOnly) {
 			result = invokeExpression(expression, parameters);
 		}
+		else {
+			result = invokeHandlerMethod(candidate, parameters);
+		}
 
 		if (result != null && this.expectedType != null) {
 			return (T) getEvaluationContext(true)
@@ -432,6 +427,44 @@ public class MessagingMethodInvokerHelper<T> extends AbstractExpressionEvaluator
 		}
 	}
 
+	@SuppressWarnings("unchecked")
+	private T invokeHandlerMethod(HandlerMethod handlerMethod, ParametersWrapper parameters) throws Exception {
+		try {
+			return (T) handlerMethod.invoke(parameters);
+		}
+		catch (MethodArgumentResolutionException | MessageConversionException | IllegalStateException e) {
+			if (e instanceof MessageConversionException) {
+				if (e.getCause() instanceof ConversionFailedException &&
+						!(e.getCause().getCause() instanceof ConverterNotFoundException)) {
+					throw e;
+				}
+			}
+			else if (e instanceof IllegalStateException) {
+				if (!(e.getCause() instanceof IllegalArgumentException) ||
+						!e.getStackTrace()[0].getClassName().equals(InvocableHandlerMethod.class.getName()) ||
+						(!"argument type mismatch".equals(e.getCause().getMessage()) &&
+						// JVM generates GeneratedMethodAccessor### after several calls with less error checking
+						!e.getCause().getMessage().startsWith("java.lang.ClassCastException@"))) {
+					throw e;
+				}
+			}
+
+			Expression expression = handlerMethod.expression;
+
+			if (++handlerMethod.failedAttempts >= FAILED_ATTEMPTS_THRESHOLD) {
+				handlerMethod.spelOnly = true;
+				if (logger.isInfoEnabled()) {
+					logger.info("Failed to invoke [ " + handlerMethod.invocableHandlerMethod +
+							"] with provided arguments [ " + parameters + " ]. \n" +
+							"Falling back to SpEL invocation for expression [ " +
+							expression.getExpressionString() + " ]");
+				}
+			}
+
+			return invokeExpression(expression, parameters);
+		}
+	}
+
 	@SuppressWarnings("unchecked")
 	private T invokeExpression(Expression expression, ParametersWrapper parameters) throws Exception {
 		try {
@@ -745,21 +778,6 @@ public class MessagingMethodInvokerHelper<T> extends AbstractExpressionEvaluator
 	 */
 	private static class HandlerMethod {
 
-		private static final SpelExpressionParser EXPRESSION_PARSER = new SpelExpressionParser();
-
-		private static final ParameterNameDiscoverer PARAMETER_NAME_DISCOVERER =
-				new LocalVariableTableParameterNameDiscoverer();
-
-		private static final TypeDescriptor messageTypeDescriptor = TypeDescriptor.valueOf(Message.class);
-
-		private static final TypeDescriptor messageListTypeDescriptor = new TypeDescriptor(
-				ReflectionUtils.findField(HandlerMethod.class, "dummyMessages"));
-
-		private static final TypeDescriptor messageArrayTypeDescriptor = TypeDescriptor.valueOf(Message[].class);
-
-		@SuppressWarnings("unused")
-		private static final Collection<Message<?>> dummyMessages = Collections.emptyList();
-
 		private final Expression expression;
 
 		private final InvocableHandlerMethod invocableHandlerMethod;
@@ -774,6 +792,11 @@ public class MessagingMethodInvokerHelper<T> extends AbstractExpressionEvaluator
 
 		private volatile boolean spelOnly;
 
+		// The number of times InvocableHandlerMethod was attempted and failed - enables us to eventually
+		// give up trying to call it when it just doesn't seem to be possible.
+		// Switching to spelOnly afterwards forever.
+		private volatile int failedAttempts = 0;
+
 		HandlerMethod(InvocableHandlerMethod invocableHandlerMethod, boolean canProcessMessageList) {
 			this.invocableHandlerMethod = invocableHandlerMethod;
 			this.canProcessMessageList = canProcessMessageList;
@@ -1050,6 +1073,20 @@ public class MessagingMethodInvokerHelper<T> extends AbstractExpressionEvaluator
 			return this.messages.getClass();
 		}
 
+		@Override
+		public String toString() {
+			final StringBuilder sb = new StringBuilder("ParametersWrapper{");
+			if (this.messages != null) {
+				sb.append("messages=").append(this.messages)
+						.append(", headers=").append(this.headers);
+			}
+			else {
+				sb.append("message=").append(this.message);
+			}
+			return sb.append('}')
+					.toString();
+		}
+
 	}
 
 	@SuppressWarnings("serial")

spring-integration-core/src/test/java/org/springframework/integration/handler/MethodInvokingMessageProcessorAnnotationTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2017 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.
@@ -17,6 +17,7 @@
 package org.springframework.integration.handler;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
@@ -38,6 +39,7 @@ import org.junit.Assert;
 import org.junit.Test;
 
 import org.springframework.integration.support.MessageBuilder;
+import org.springframework.integration.test.util.TestUtils;
 import org.springframework.messaging.Message;
 import org.springframework.messaging.MessageHandlingException;
 import org.springframework.messaging.MessageHeaders;
@@ -52,6 +54,7 @@ import org.springframework.messaging.support.GenericMessage;
  * @author Iwein Fuld
  * @author Oleg Zhurakousky
  * @author Gary Russell
+ * @author Artem Bilan
  */
 @SuppressWarnings({ "rawtypes", "unchecked" })
 public class MethodInvokingMessageProcessorAnnotationTests {
@@ -102,7 +105,7 @@ public class MethodInvokingMessageProcessorAnnotationTests {
 		Method method = TestService.class.getMethod("requiredHeader", Integer.class);
 		MethodInvokingMessageProcessor processor = new MethodInvokingMessageProcessor(testService, method);
 		Message<String> messageWithHeader = MessageBuilder.withPayload("foo")
-				.setHeader("num", new Integer(123)).build();
+				.setHeader("num", 123).build();
 		GenericMessage<String> messageWithoutHeader = new GenericMessage<String>("foo");
 
 		processor.processMessage(messageWithHeader);
@@ -113,7 +116,7 @@ public class MethodInvokingMessageProcessorAnnotationTests {
 	public void fromMessageWithRequiredHeaderProvided() throws Exception {
 		Method method = TestService.class.getMethod("requiredHeader", Integer.class);
 		Message<String> message = MessageBuilder.withPayload("foo")
-				.setHeader("num", new Integer(123)).build();
+				.setHeader("num", 123).build();
 		MethodInvokingMessageProcessor processor = new MethodInvokingMessageProcessor(testService, method);
 		Object result = processor.processMessage(message);
 		assertEquals(123, result);
@@ -133,7 +136,7 @@ public class MethodInvokingMessageProcessorAnnotationTests {
 		Method method = TestService.class.getMethod("optionalAndRequiredHeader", String.class, Integer.class);
 		MethodInvokingMessageProcessor processor = new MethodInvokingMessageProcessor(testService, method);
 		Message<String> message = MessageBuilder.withPayload("foo")
-				.setHeader("num", new Integer(123)).build();
+				.setHeader("num", 123).build();
 		Object result = processor.processMessage(message);
 		assertEquals("null123", result);
 	}
@@ -143,7 +146,7 @@ public class MethodInvokingMessageProcessorAnnotationTests {
 		Method method = TestService.class.getMethod("optionalAndRequiredHeader", String.class, Integer.class);
 		MethodInvokingMessageProcessor processor = new MethodInvokingMessageProcessor(testService, method);
 		Message<String> message = MessageBuilder.withPayload("foo")
-				.setHeader("num", new Integer(123))
+				.setHeader("num", 123)
 				.setHeader("prop", "bar")
 				.build();
 		Object result = processor.processMessage(message);
@@ -156,10 +159,21 @@ public class MethodInvokingMessageProcessorAnnotationTests {
 		MethodInvokingMessageProcessor processor = new MethodInvokingMessageProcessor(testService, method);
 		Message<String> message = MessageBuilder.withPayload("test")
 				.setHeader("prop1", "foo").setHeader("prop2", "bar").build();
+		assertFalse(TestUtils.getPropertyValue(processor, "delegate.handlerMethod.spelOnly", Boolean.class));
+		for (int i = 0; i < 99; i++) {
+			Object result = processor.processMessage(message);
+			Properties props = (Properties) result;
+			assertEquals("foo", props.getProperty("prop1"));
+			assertEquals("bar", props.getProperty("prop2"));
+			assertFalse(TestUtils.getPropertyValue(processor, "delegate.handlerMethod.spelOnly", Boolean.class));
+		}
+
 		Object result = processor.processMessage(message);
 		Properties props = (Properties) result;
 		assertEquals("foo", props.getProperty("prop1"));
 		assertEquals("bar", props.getProperty("prop2"));
+
+		assertTrue(TestUtils.getPropertyValue(processor, "delegate.handlerMethod.spelOnly", Boolean.class));
 	}
 
 	@Test
@@ -210,8 +224,8 @@ public class MethodInvokingMessageProcessorAnnotationTests {
 		Method method = TestService.class.getMethod("mapHeaders", Map.class);
 		MethodInvokingMessageProcessor processor = new MethodInvokingMessageProcessor(testService, method);
 		Message<String> message = MessageBuilder.withPayload("test")
-				.setHeader("attrib1", new Integer(123))
-				.setHeader("attrib2", new Integer(456)).build();
+				.setHeader("attrib1", 123)
+				.setHeader("attrib2", 456).build();
 		Map<String, Object> result = (Map<String, Object>) processor.processMessage(message);
 		assertEquals(123, result.get("attrib1"));
 		assertEquals(456, result.get("attrib2"));
@@ -225,8 +239,8 @@ public class MethodInvokingMessageProcessorAnnotationTests {
 		payload.put("attrib1", 88);
 		payload.put("attrib2", 99);
 		Message<Map<String, Integer>> message = MessageBuilder.withPayload(payload)
-				.setHeader("attrib1", new Integer(123))
-				.setHeader("attrib2", new Integer(456)).build();
+				.setHeader("attrib1", 123)
+				.setHeader("attrib2", 456).build();
 		Map<String, Integer> result = (Map<String, Integer>) processor.processMessage(message);
 		assertEquals(2, result.size());
 		assertEquals(new Integer(88), result.get("attrib1"));
@@ -337,11 +351,19 @@ public class MethodInvokingMessageProcessorAnnotationTests {
 	}
 
 
+	private Message<?> getMessage() {
+		MessageBuilder<Employee> builder = MessageBuilder.withPayload(employee);
+		builder.setHeader("day", "monday");
+		builder.setHeader("month", "September");
+		return builder.build();
+	}
+
 	@SuppressWarnings("unused")
 	private static class MultipleMappingAnnotationTestBean {
 
 		public void test(@Payload("payload") @Header("foo") String s) {
 		}
+
 	}
 
 
@@ -379,7 +401,8 @@ public class MethodInvokingMessageProcessorAnnotationTests {
 			return lastName + ", " + firstName;
 		}
 
-		public String optionalAndRequiredHeader(@Header(required = false) String prop, @Header(value = "num", required = true) Integer num) {
+		public String optionalAndRequiredHeader(@Header(required = false) String prop,
+				@Header(value = "num", required = true) Integer num) {
 			return prop + num;
 		}
 
@@ -444,13 +467,7 @@ public class MethodInvokingMessageProcessorAnnotationTests {
 			ids.add(id);
 			return "foo";
 		}
-	}
 
-	private Message<?> getMessage() {
-		MessageBuilder<Employee> builder = MessageBuilder.withPayload(employee);
-		builder.setHeader("day", "monday");
-		builder.setHeader("month", "September");
-		return builder.build();
 	}
 
 
@@ -472,6 +489,7 @@ public class MethodInvokingMessageProcessorAnnotationTests {
 		public String getLname() {
 			return lname;
 		}
+
 	}
 
 }

spring-integration-core/src/test/java/org/springframework/integration/handler/MethodInvokingMessageProcessorTests.java

@@ -17,6 +17,7 @@
 package org.springframework.integration.handler;
 
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.instanceOf;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
@@ -244,13 +245,13 @@ public class MethodInvokingMessageProcessorTests {
 	public void payloadAndHeaderAnnotationMethodParametersAndObjectAsReturnValue() {
 		MethodInvokingMessageProcessor processor = new MethodInvokingMessageProcessor(new TestBean(),
 				"acceptPayloadAndHeaderAndReturnObject");
-		Message<?> request = MessageBuilder.withPayload("testing").setHeader("number", new Integer(123)).build();
+		Message<?> request = MessageBuilder.withPayload("testing").setHeader("number", 123).build();
 		Object result = processor.processMessage(request);
 		assertEquals("testing-123", result);
 	}
 
 	@Test
-	public void testVoidMethodsIncludedbyDefault() {
+	public void testVoidMethodsIncludedByDefault() {
 		MethodInvokingMessageProcessor processor = new MethodInvokingMessageProcessor(new TestBean(),
 				"testVoidReturningMethods");
 		assertNull(processor.processMessage(MessageBuilder.withPayload("Something").build()));
@@ -271,8 +272,8 @@ public class MethodInvokingMessageProcessorTests {
 		AnnotatedTestService service = new AnnotatedTestService();
 		Method method = service.getClass().getMethod("integerMethod", Integer.class);
 		MethodInvokingMessageProcessor processor = new MethodInvokingMessageProcessor(service, method);
-		Object result = processor.processMessage(new GenericMessage<Integer>(new Integer(123)));
-		assertEquals(new Integer(123), result);
+		Object result = processor.processMessage(new GenericMessage<>(123));
+		assertEquals(123, result);
 	}
 
 	@Test
@@ -281,7 +282,7 @@ public class MethodInvokingMessageProcessorTests {
 		Method method = service.getClass().getMethod("integerMethod", Integer.class);
 		MethodInvokingMessageProcessor processor = new MethodInvokingMessageProcessor(service, method);
 		Object result = processor.processMessage(new GenericMessage<String>("456"));
-		assertEquals(new Integer(456), result);
+		assertEquals(456, result);
 	}
 
 	@Test(expected = MessageHandlingException.class)
@@ -733,10 +734,37 @@ public class MethodInvokingMessageProcessorTests {
 		compilerConfigAccessor.setPropertyValue("compilerMode", SpelCompilerMode.OFF);
 	}
 
+
+	@Test
+	public void testNoSpElFallbackWhenUserException() {
+		class A {
+
+			@SuppressWarnings("unused")
+			public void myMethod(Object payload) {
+				throw new IllegalStateException(new IllegalArgumentException("argument type mismatch"));
+			}
+		}
+
+		MethodInvokingMessageProcessor processor = new MethodInvokingMessageProcessor(new A(), "myMethod");
+
+		try {
+			processor.processMessage(new GenericMessage<>("foo"));
+		}
+		catch (Exception e) {
+			assertThat(e.getCause(), instanceOf(IllegalStateException.class));
+			assertThat(e.getCause().getCause(), instanceOf(IllegalArgumentException.class));
+			assertEquals(A.class.getName(), e.getCause().getStackTrace()[0].getClassName());
+		}
+
+		assertEquals(0,
+				TestUtils.getPropertyValue(processor, "delegate.handlerMethod.failedAttempts"));
+
+	}
+
 	private DirectFieldAccessor compileImmediate(MethodInvokingMessageProcessor processor) {
 		// Update the parser configuration compiler mode
 		SpelParserConfiguration config = TestUtils.getPropertyValue(processor,
-				"delegate.handlerMethod.EXPRESSION_PARSER.configuration", SpelParserConfiguration.class);
+				"delegate.EXPRESSION_PARSER.configuration", SpelParserConfiguration.class);
 		DirectFieldAccessor accessor = new DirectFieldAccessor(config);
 		accessor.setPropertyValue("compilerMode", SpelCompilerMode.IMMEDIATE);
 		return accessor;

spring-integration-ip/src/test/java/org/springframework/integration/ip/tcp/connection/TcpNioConnectionTests.java

@@ -716,8 +716,8 @@ public class TcpNioConnectionTests {
 			@Override
 			public boolean onMessage(Message<?> message) {
 				if (!(message instanceof ErrorMessage)) {
-					assemblerLatch.countDown();
 					assembler.set(Thread.currentThread());
+					assemblerLatch.countDown();
 				}
 				return false;
 			}