From d928105ef1d8b5ddaf388e0c0db7d45d8040348d Mon Sep 17 00:00:00 2001 From: Gary Russell Date: Wed, 23 Jan 2013 14:54:39 -0500 Subject: [PATCH] INT-2894 Do Not Leak ThrowableHolderException The AbstractRequestHandlerAdvice uses an internal wrapper for Throwable, in a similar fashion to the TransactionInterceptor. Exceptions are unwrapped properly when exiting the invoke method so callers are unaware of the wrapping. The technique is used to avoid user errors (for example catching OOM Errors and not propagating). User code (subclasses) only catch Exceptions. However, in the case of the ExpressionEvaluatingRequestHandlerAdvice, the exception is included in any ErrorMessage sent to a failureChannel. User code should not have to navigate these internal exceptions within the cause tree. Add a method to the abstract class unwrapExceptionIfNecessary, which will unwrap the root cause exception if possible. Add a method to the abstract class unwrapThrowableIfNecessary, which will unwrap the root cause Throwable if possible. Update tests to reduce the cause() traversals to reflect the ThrowableHolderExceptions are no longer in the tree. Add a test to verify a Throwable (such as an OOM) is properly propagated. Add a test to verify that when the root cause is a Throwable (and not an Exception), the ErrorMessage sent to the failureChannel does not include the ThrowableHolderException. qualified inner class @links (when merging) --- .../advice/AbstractRequestHandlerAdvice.java | 34 +++++- ...ressionEvaluatingRequestHandlerAdvice.java | 7 +- .../RequestHandlerCircuitBreakerAdvice.java | 2 +- .../advice/RequestHandlerRetryAdvice.java | 3 +- .../advice/AdvisedMessageHandlerTests.java | 113 ++++++++++++++++-- 5 files changed, 142 insertions(+), 17 deletions(-) diff --git a/spring-integration-core/src/main/java/org/springframework/integration/handler/advice/AbstractRequestHandlerAdvice.java b/spring-integration-core/src/main/java/org/springframework/integration/handler/advice/AbstractRequestHandlerAdvice.java index 5095bb8dd3..c2adba4420 100644 --- a/spring-integration-core/src/main/java/org/springframework/integration/handler/advice/AbstractRequestHandlerAdvice.java +++ b/spring-integration-core/src/main/java/org/springframework/integration/handler/advice/AbstractRequestHandlerAdvice.java @@ -111,8 +111,38 @@ public abstract class AbstractRequestHandlerAdvice extends IntegrationObjectSupp protected abstract Object doInvoke(ExecutionCallback callback, Object target, Message message) throws Exception; /** - * Called by subclasses in doInvoke() to proceed() the invocation. - * + * Unwrap the cause of a {@link AbstractRequestHandlerAdvice.ThrowableHolderException}. + * @param e The exception. + * @return The cause, or e, if not a {@link AbstractRequestHandlerAdvice.ThrowableHolderException} + */ + protected Exception unwrapExceptionIfNecessary(Exception e) { + Exception actualException = e; + if (e instanceof ThrowableHolderException) { + if (e.getCause() instanceof Exception) { + actualException = (Exception) e.getCause(); + } + } + return actualException; + } + + /** + * Unwrap the cause of a {@link AbstractRequestHandlerAdvice.ThrowableHolderException}. + * @param e The exception. + * @return The cause, or e, if not a {@link AbstractRequestHandlerAdvice.ThrowableHolderException} + */ + protected Throwable unwrapThrowableIfNecessary(Exception e) { + Throwable actualThrowable = e; + if (e instanceof ThrowableHolderException) { + actualThrowable = e.getCause(); + } + return actualThrowable; + } + + /** + * Called by subclasses in doInvoke() to proceed() the invocation. Callers + * unwrap {@link AbstractRequestHandlerAdvice.ThrowableHolderException}s and use + * the cause for evaluation and re-throwing purposes. + * See {@link AbstractRequestHandlerAdvice#unwrapExceptionIfNecessary(Exception)}. */ protected interface ExecutionCallback { diff --git a/spring-integration-core/src/main/java/org/springframework/integration/handler/advice/ExpressionEvaluatingRequestHandlerAdvice.java b/spring-integration-core/src/main/java/org/springframework/integration/handler/advice/ExpressionEvaluatingRequestHandlerAdvice.java index 56ab4a0329..9d19ea4c17 100644 --- a/spring-integration-core/src/main/java/org/springframework/integration/handler/advice/ExpressionEvaluatingRequestHandlerAdvice.java +++ b/spring-integration-core/src/main/java/org/springframework/integration/handler/advice/ExpressionEvaluatingRequestHandlerAdvice.java @@ -120,14 +120,15 @@ public class ExpressionEvaluatingRequestHandlerAdvice extends AbstractRequestHan return result; } catch (Exception e) { + Exception actualException = this.unwrapExceptionIfNecessary(e); if (this.onFailureExpression != null) { - Object evalResult = this.evaluateFailureExpression(message, e); + Object evalResult = this.evaluateFailureExpression(message, actualException); if (this.returnFailureExpressionResult) { return evalResult; } } if (!this.trapException) { - throw e; + throw actualException; } return null; } @@ -163,7 +164,7 @@ public class ExpressionEvaluatingRequestHandlerAdvice extends AbstractRequestHan } if (evalResult != null && this.failureChannel != null) { MessagingException messagingException = new MessageHandlingExpressionEvaluatingAdviceException(message, - "Handler Failed", exception, evalResult); + "Handler Failed", this.unwrapThrowableIfNecessary(exception), evalResult); ErrorMessage resultMessage = new ErrorMessage(messagingException); this.messagingTemplate.send(this.failureChannel, resultMessage); } diff --git a/spring-integration-core/src/main/java/org/springframework/integration/handler/advice/RequestHandlerCircuitBreakerAdvice.java b/spring-integration-core/src/main/java/org/springframework/integration/handler/advice/RequestHandlerCircuitBreakerAdvice.java index 926c8f019c..8d75c74d7e 100644 --- a/spring-integration-core/src/main/java/org/springframework/integration/handler/advice/RequestHandlerCircuitBreakerAdvice.java +++ b/spring-integration-core/src/main/java/org/springframework/integration/handler/advice/RequestHandlerCircuitBreakerAdvice.java @@ -68,7 +68,7 @@ public class RequestHandlerCircuitBreakerAdvice extends AbstractRequestHandlerAd catch (Exception e) { metadata.getFailures().incrementAndGet(); metadata.setLastFailure(System.currentTimeMillis()); - throw e; + throw this.unwrapExceptionIfNecessary(e); } } diff --git a/spring-integration-core/src/main/java/org/springframework/integration/handler/advice/RequestHandlerRetryAdvice.java b/spring-integration-core/src/main/java/org/springframework/integration/handler/advice/RequestHandlerRetryAdvice.java index bed7607600..536e290a9f 100644 --- a/spring-integration-core/src/main/java/org/springframework/integration/handler/advice/RequestHandlerRetryAdvice.java +++ b/spring-integration-core/src/main/java/org/springframework/integration/handler/advice/RequestHandlerRetryAdvice.java @@ -93,7 +93,8 @@ public class RequestHandlerRetryAdvice extends AbstractRequestHandlerAdvice throw e; } catch (Exception e) { - throw new MessagingException(message, "Failed to invoke handler", e); + throw new MessagingException(message, "Failed to invoke handler", + unwrapExceptionIfNecessary(e)); } } }, this.recoveryCallback, retryState); diff --git a/spring-integration-core/src/test/java/org/springframework/integration/handler/advice/AdvisedMessageHandlerTests.java b/spring-integration-core/src/test/java/org/springframework/integration/handler/advice/AdvisedMessageHandlerTests.java index dfaf4fd01b..8446828f61 100644 --- a/spring-integration-core/src/test/java/org/springframework/integration/handler/advice/AdvisedMessageHandlerTests.java +++ b/spring-integration-core/src/test/java/org/springframework/integration/handler/advice/AdvisedMessageHandlerTests.java @@ -21,7 +21,11 @@ import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import java.lang.reflect.Method; import java.util.ArrayList; import java.util.List; import java.util.concurrent.atomic.AtomicBoolean; @@ -31,6 +35,9 @@ import org.aopalliance.aop.Advice; import org.aopalliance.intercept.MethodInterceptor; import org.aopalliance.intercept.MethodInvocation; import org.junit.Test; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; +import org.springframework.aop.framework.ProxyFactory; import org.springframework.integration.Message; import org.springframework.integration.MessageHandlingException; import org.springframework.integration.MessagingException; @@ -39,6 +46,7 @@ import org.springframework.integration.core.PollableChannel; import org.springframework.integration.handler.AbstractReplyProducingMessageHandler; import org.springframework.integration.handler.advice.ExpressionEvaluatingRequestHandlerAdvice.MessageHandlingExpressionEvaluatingAdviceException; import org.springframework.integration.message.AdviceMessage; +import org.springframework.integration.message.ErrorMessage; import org.springframework.integration.message.GenericMessage; import org.springframework.retry.RecoveryCallback; import org.springframework.retry.RetryContext; @@ -82,7 +90,7 @@ public class AdvisedMessageHandlerTests { advice.setSuccessChannel(successChannel); advice.setFailureChannel(failureChannel); advice.setOnSuccessExpression("'foo'"); - advice.setOnFailureExpression("'bar:' + #exception.cause.message"); + advice.setOnFailureExpression("'bar:' + #exception.message"); List adviceChain = new ArrayList(); adviceChain.add(advice); @@ -243,7 +251,7 @@ public class AdvisedMessageHandlerTests { assertNotNull(failure); assertEquals("Hello, world!", ((MessagingException) failure.getPayload()).getFailedMessage().getPayload()); assertEquals(MessageHandlingExpressionEvaluatingAdviceException.class, failure.getPayload().getClass()); - assertEquals("qux", ((Exception) failure.getPayload()).getCause().getCause().getMessage()); + assertEquals("qux", ((Exception) failure.getPayload()).getCause().getMessage()); // propagate failing advice with failure; expect original exception advice.setPropagateEvaluationFailures(true); @@ -261,7 +269,7 @@ public class AdvisedMessageHandlerTests { assertNotNull(failure); assertEquals("Hello, world!", ((MessagingException) failure.getPayload()).getFailedMessage().getPayload()); assertEquals(MessageHandlingExpressionEvaluatingAdviceException.class, failure.getPayload().getClass()); - assertEquals("qux", ((Exception) failure.getPayload()).getCause().getCause().getMessage()); + assertEquals("qux", ((Exception) failure.getPayload()).getCause().getMessage()); } @@ -536,7 +544,7 @@ public class AdvisedMessageHandlerTests { handler.handleMessage(message); Message error = errors.receive(1000); assertNotNull(error); - assertEquals("fooException", ((Exception) error.getPayload()).getCause().getCause().getMessage()); + assertEquals("fooException", ((Exception) error.getPayload()).getCause().getMessage()); } @@ -607,8 +615,8 @@ public class AdvisedMessageHandlerTests { } catch (Exception e) { Throwable cause = e.getCause(); - assertEquals("ThrowableHolderException", cause.getClass().getSimpleName()); - assertEquals("intentional", cause.getCause().getMessage()); + assertEquals(RuntimeException.class, cause.getClass()); + assertEquals("intentional", cause.getMessage()); } assertTrue(counter.get() == 0); @@ -631,8 +639,8 @@ public class AdvisedMessageHandlerTests { List adviceChain = new ArrayList(); ExpressionEvaluatingRequestHandlerAdvice expressionAdvice = new ExpressionEvaluatingRequestHandlerAdvice(); -// ThrowableHolderException / MessagingException / ThrowableHolderException / RuntimeException - expressionAdvice.setOnFailureExpression("#exception.cause.cause.cause.message"); +// MessagingException / RuntimeException + expressionAdvice.setOnFailureExpression("#exception.cause.message"); expressionAdvice.setReturnFailureExpressionResult(true); final AtomicInteger outerCounter = new AtomicInteger(); adviceChain.add(new AbstractRequestHandlerAdvice() { @@ -678,7 +686,7 @@ public class AdvisedMessageHandlerTests { List adviceChain = new ArrayList(); ExpressionEvaluatingRequestHandlerAdvice expressionAdvice = new ExpressionEvaluatingRequestHandlerAdvice(); - expressionAdvice.setOnFailureExpression("#exception.cause.message"); + expressionAdvice.setOnFailureExpression("#exception.message"); expressionAdvice.setFailureChannel(errors); adviceChain.add(new RequestHandlerRetryAdvice()); @@ -696,7 +704,7 @@ public class AdvisedMessageHandlerTests { handler.handleMessage(new GenericMessage("test")); } catch (Exception e) { - assertEquals("intentional: 3", e.getCause().getCause().getMessage()); + assertEquals("intentional: 3", e.getCause().getMessage()); } for (int i = 1; i <= 3; i++) { @@ -709,5 +717,90 @@ public class AdvisedMessageHandlerTests { } + /** + * Verify that Errors such as OOM are properly propagated. + */ + @Test + public void throwableProperlyPropagated() throws Exception { + AbstractRequestHandlerAdvice advice = new AbstractRequestHandlerAdvice() { + @Override + protected Object doInvoke(ExecutionCallback callback, Object target, Message message) throws Exception { + Object result; + try { + result = callback.execute(); + } + catch (Exception e) { + // should not be unwrapped because the cause is a Throwable + throw this.unwrapExceptionIfNecessary(e); + } + return result; + } + }; + final Throwable theThrowable = new Throwable("foo"); + MethodInvocation methodInvocation = mock(MethodInvocation.class); + + Method method = AbstractReplyProducingMessageHandler.class.getDeclaredMethod("handleRequestMessage", Message.class); + when(methodInvocation.getMethod()).thenReturn(method); + when(methodInvocation.getArguments()).thenReturn(new Object[] {new GenericMessage("foo")}); + try { + doAnswer(new Answer() { + public Object answer(InvocationOnMock invocation) throws Throwable { + throw theThrowable; + } + }).when(methodInvocation).proceed(); + advice.invoke(methodInvocation); + fail("Expected throwable"); + } + catch (Throwable t) { + assertSame(theThrowable, t); + } + } + + /** + * Verify that Errors such as OOM are properly propagated and we suppress the + * ThrowableHolderException from the output message. + */ + @Test + public void throwableProperlyPropagatedAndReported() throws Exception { + QueueChannel errors = new QueueChannel(); + + ExpressionEvaluatingRequestHandlerAdvice expressionAdvice = new ExpressionEvaluatingRequestHandlerAdvice(); + expressionAdvice.setOnFailureExpression("'foo'"); + expressionAdvice.setFailureChannel(errors); + + Throwable theThrowable = new Throwable("foo"); + ProxyFactory proxyFactory = new ProxyFactory(new Foo(theThrowable)); + proxyFactory.addAdvice(expressionAdvice); + + Bar fooHandler = (Bar) proxyFactory.getProxy(); + + try { + fooHandler.handleRequestMessage(new GenericMessage("foo")); + fail("Expected throwable"); + } + catch (Throwable t) { + assertSame(theThrowable, t); + ErrorMessage error = (ErrorMessage) errors.receive(1000); + assertNotNull(error); + assertSame(theThrowable, error.getPayload().getCause()); + } + } + + private interface Bar { + Object handleRequestMessage(Message message) throws Throwable; + } + + private class Foo implements Bar { + + public final Throwable throwable; + + public Foo(Throwable throwable) { + this.throwable = throwable; + } + + public Object handleRequestMessage(Message message) throws Throwable { + throw this.throwable; + } + } }