Apply AsyncUncaughtExceptionHandler to AspectJ

Prior to this commit, only @Async annotated methods with proxy style
had their custom uncaught exception handler applied. This commit
harmonizes the configuration so that AspectJ applies that behaviour as
well.

Issue: SPR-12090

spring-aop/src/main/java/org/springframework/aop/interceptor/AsyncExecutionAspectSupport.java

@@ -20,6 +20,10 @@ import java.lang.reflect.Method;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.Executor;
+import java.util.concurrent.Future;
+
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
 
 import org.springframework.beans.factory.BeanFactory;
 import org.springframework.beans.factory.BeanFactoryAware;
@@ -28,11 +32,12 @@ import org.springframework.core.task.AsyncListenableTaskExecutor;
 import org.springframework.core.task.AsyncTaskExecutor;
 import org.springframework.core.task.support.TaskExecutorAdapter;
 import org.springframework.util.Assert;
+import org.springframework.util.ReflectionUtils;
 import org.springframework.util.StringUtils;
 
 /**
  * Base class for asynchronous method execution aspects, such as
- * {@link org.springframework.scheduling.annotation.AnnotationAsyncExecutionInterceptor}
+ * {@code org.springframework.scheduling.annotation.AnnotationAsyncExecutionInterceptor}
  * or {@code org.springframework.scheduling.aspectj.AnnotationAsyncExecutionAspect}.
  *
  * <p>Provides support for <i>executor qualification</i> on a method-by-method basis.
@@ -42,14 +47,19 @@ import org.springframework.util.StringUtils;
  *
  * @author Chris Beams
  * @author Juergen Hoeller
+ * @author Stephane Nicoll
  * @since 3.1.2
  */
 public abstract class AsyncExecutionAspectSupport implements BeanFactoryAware {
 
+	protected final Log logger = LogFactory.getLog(getClass());
+
 	private final Map<Method, AsyncTaskExecutor> executors = new ConcurrentHashMap<Method, AsyncTaskExecutor>(16);
 
 	private Executor defaultExecutor;
 
+	private AsyncUncaughtExceptionHandler exceptionHandler;
+
 	private BeanFactory beanFactory;
 
 
@@ -58,9 +68,18 @@ public abstract class AsyncExecutionAspectSupport implements BeanFactoryAware {
 	 * executor unless individual async methods indicate via qualifier that a more
 	 * specific executor should be used.
 	 * @param defaultExecutor the executor to use when executing asynchronous methods
+	 * @param exceptionHandler the {@link AsyncUncaughtExceptionHandler} to use
+	 */
+	public AsyncExecutionAspectSupport(Executor defaultExecutor, AsyncUncaughtExceptionHandler exceptionHandler) {
+		this.defaultExecutor = defaultExecutor;
+		this.exceptionHandler = exceptionHandler;
+	}
+
+	/**
+	 * Create a new instance with a default {@link AsyncUncaughtExceptionHandler}.
 	 */
 	public AsyncExecutionAspectSupport(Executor defaultExecutor) {
-		this.defaultExecutor = defaultExecutor;
+		this(defaultExecutor, new SimpleAsyncUncaughtExceptionHandler());
 	}
 
 
@@ -78,6 +97,14 @@ public abstract class AsyncExecutionAspectSupport implements BeanFactoryAware {
 		this.defaultExecutor = defaultExecutor;
 	}
 
+	/**
+	 * Supply the {@link AsyncUncaughtExceptionHandler} to use to handle exceptions
+	 * thrown by invoking asynchronous methods with a {@code void} return type.
+	 */
+	public void setExceptionHandler(AsyncUncaughtExceptionHandler exceptionHandler) {
+		this.exceptionHandler = exceptionHandler;
+	}
+
 	/**
 	 * Set the {@link BeanFactory} to be used when looking up executors by qualifier.
 	 */
@@ -125,4 +152,32 @@ public abstract class AsyncExecutionAspectSupport implements BeanFactoryAware {
 	 */
 	protected abstract String getExecutorQualifier(Method method);
 
+	/**
+	 * Handles a fatal error thrown while asynchronously invoking the specified
+	 * {@link Method}.
+	 * <p>If the return type of the method is a {@link java.util.concurrent.Future} object, the original
+	 * exception can be propagated by just throwing it at the higher level. However,
+	 * for all other cases, the exception will not be transmitted back to the client.
+	 * In that later case, the current {@link AsyncUncaughtExceptionHandler} will be
+	 * used to manage such exception.
+	 * @param ex the exception to handle
+	 * @param method the method that was invoked
+	 * @param params the parameters used to invoke the method
+	 */
+	protected void handleError(Throwable ex, Method method, Object... params) throws Exception {
+		if (method.getReturnType().isAssignableFrom(Future.class)) {
+			ReflectionUtils.rethrowException(ex);
+		}
+		else {
+			// Could not transmit the exception to the caller with default executor
+			try {
+				this.exceptionHandler.handleUncaughtException(ex, method, params);
+			}
+			catch (Throwable ex2) {
+				logger.error("Exception handler for async method '" + method.toGenericString() +
+						"' threw unexpected exception itself", ex2);
+			}
+		}
+	}
+
 }

spring-aop/src/main/java/org/springframework/aop/interceptor/AsyncExecutionInterceptor.java

@@ -23,8 +23,6 @@ import java.util.concurrent.Future;
 
 import org.aopalliance.intercept.MethodInterceptor;
 import org.aopalliance.intercept.MethodInvocation;
-import org.apache.commons.logging.Log;
-import org.apache.commons.logging.LogFactory;
 
 import org.springframework.aop.support.AopUtils;
 import org.springframework.core.BridgeMethodResolver;
@@ -32,7 +30,6 @@ import org.springframework.core.Ordered;
 import org.springframework.core.task.AsyncListenableTaskExecutor;
 import org.springframework.core.task.AsyncTaskExecutor;
 import org.springframework.util.ClassUtils;
-import org.springframework.util.ReflectionUtils;
 import org.springframework.util.concurrent.ListenableFuture;
 
 /**
@@ -70,11 +67,6 @@ import org.springframework.util.concurrent.ListenableFuture;
 public class AsyncExecutionInterceptor extends AsyncExecutionAspectSupport
 		implements MethodInterceptor, Ordered {
 
-	private final Log logger = LogFactory.getLog(getClass());
-
-	private AsyncUncaughtExceptionHandler exceptionHandler;
-
-
 	/**
 	 * Create a new {@code AsyncExecutionInterceptor}.
 	 * @param defaultExecutor the {@link Executor} (typically a Spring {@link AsyncTaskExecutor}
@@ -82,27 +74,16 @@ public class AsyncExecutionInterceptor extends AsyncExecutionAspectSupport
 	 * @param exceptionHandler the {@link AsyncUncaughtExceptionHandler} to use
 	 */
 	public AsyncExecutionInterceptor(Executor defaultExecutor, AsyncUncaughtExceptionHandler exceptionHandler) {
-		super(defaultExecutor);
+		super(defaultExecutor, exceptionHandler);
-		this.exceptionHandler = exceptionHandler;
 	}
 
 	/**
 	 * Create a new instance with a default {@link AsyncUncaughtExceptionHandler}.
 	 */
 	public AsyncExecutionInterceptor(Executor defaultExecutor) {
-		this(defaultExecutor, new SimpleAsyncUncaughtExceptionHandler());
+		super(defaultExecutor);
 	}
 
-
-	/**
-	 * Supply the {@link AsyncUncaughtExceptionHandler} to use to handle exceptions
-	 * thrown by invoking asynchronous methods with a {@code void} return type.
-	 */
-	public void setExceptionHandler(AsyncUncaughtExceptionHandler exceptionHandler) {
-		this.exceptionHandler = exceptionHandler;
-	}
-
-
 	/**
 	 * Intercept the given method invocation, submit the actual calling of the method to
 	 * the correct task executor and return immediately to the caller.
@@ -151,34 +132,6 @@ public class AsyncExecutionInterceptor extends AsyncExecutionAspectSupport
 		}
 	}
 
-	/**
-	 * Handles a fatal error thrown while asynchronously invoking the specified
-	 * {@link Method}.
-	 * <p>If the return type of the method is a {@link Future} object, the original
-	 * exception can be propagated by just throwing it at the higher level. However,
-	 * for all other cases, the exception will not be transmitted back to the client.
-	 * In that later case, the current {@link AsyncUncaughtExceptionHandler} will be
-	 * used to manage such exception.
-	 * @param ex the exception to handle
-	 * @param method the method that was invoked
-	 * @param params the parameters used to invoke the method
-	 */
-	protected void handleError(Throwable ex, Method method, Object... params) throws Exception {
-		if (method.getReturnType().isAssignableFrom(Future.class)) {
-			ReflectionUtils.rethrowException(ex);
-		}
-		else {
-			// Could not transmit the exception to the caller with default executor
-			try {
-				this.exceptionHandler.handleUncaughtException(ex, method, params);
-			}
-			catch (Throwable ex2) {
-				logger.error("Exception handler for async method '" + method.toGenericString() +
-						"' threw unexpected exception itself", ex2);
-			}
-		}
-	}
-
 	/**
 	 * This implementation is a no-op for compatibility in Spring 3.1.2.
 	 * Subclasses may override to provide support for extracting qualifier information,

spring-aspects/src/main/java/org/springframework/scheduling/aspectj/AbstractAsyncExecutionAspect.aj

@@ -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.
@@ -35,6 +35,7 @@ import org.springframework.core.task.AsyncTaskExecutor;
  * @author Ramnivas Laddad
  * @author Juergen Hoeller
  * @author Chris Beams
+ * @author Stephane Nicoll
  * @since 3.0.5
  */
 public abstract aspect AbstractAsyncExecutionAspect extends AsyncExecutionAspectSupport {
@@ -42,7 +43,7 @@ public abstract aspect AbstractAsyncExecutionAspect extends AsyncExecutionAspect
 	/**
 	 * Create an {@code AnnotationAsyncExecutionAspect} with a {@code null} default
 	 * executor, which should instead be set via {@code #aspectOf} and
-	 * {@link #setExecutor(Executor)}.
+	 * {@link #setExecutor(Executor)}. The same applies for {@link #setExceptionHandler}
 	 */
 	public AbstractAsyncExecutionAspect() {
 		super(null);
@@ -56,16 +57,21 @@ public abstract aspect AbstractAsyncExecutionAspect extends AsyncExecutionAspect
 	 * otherwise.
 	 */
 	Object around() : asyncMethod() {
-		MethodSignature methodSignature = (MethodSignature) thisJoinPointStaticPart.getSignature();
+		final MethodSignature methodSignature = (MethodSignature) thisJoinPointStaticPart.getSignature();
 		AsyncTaskExecutor executor = determineAsyncExecutor(methodSignature.getMethod());
 		if (executor == null) {
 			return proceed();
 		}
 		Callable<Object> callable = new Callable<Object>() {
 			public Object call() throws Exception {
-				Object result = proceed();
+				try {
-				if (result instanceof Future) {
+					Object result = proceed();
-					return ((Future<?>) result).get();
+					if (result instanceof Future) {
+						return ((Future<?>) result).get();
+					}
+				}
+				catch (Throwable ex) {
+					handleError(ex, methodSignature.getMethod(), thisJoinPoint.getArgs());
 				}
 				return null;
 			}};

spring-aspects/src/main/java/org/springframework/scheduling/aspectj/AspectJAsyncConfiguration.java

@@ -29,6 +29,7 @@ import org.springframework.scheduling.config.TaskManagementConfigUtils;
  * to enable AspectJ-based asynchronous method execution.
  *
  * @author Chris Beams
+ * @author Stephane Nicoll
  * @since 3.1
  * @see EnableAsync
  * @see org.springframework.scheduling.annotation.AsyncConfigurationSelector
@@ -44,6 +45,9 @@ public class AspectJAsyncConfiguration extends AbstractAsyncConfiguration {
 		if (this.executor != null) {
 			asyncAspect.setExecutor(this.executor);
 		}
+		if (this.exceptionHandler != null) {
+			asyncAspect.setExceptionHandler(this.exceptionHandler);
+		}
 		return asyncAspect;
 	}
 

spring-aspects/src/test/java/org/springframework/scheduling/aspectj/AnnotationAsyncExecutionAspectTests.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.
@@ -16,6 +16,7 @@
 
 package org.springframework.scheduling.aspectj;
 
+import java.lang.reflect.Method;
 import java.util.concurrent.Callable;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Future;
@@ -23,6 +24,8 @@ import java.util.concurrent.Future;
 import org.junit.Before;
 import org.junit.Test;
 
+import org.springframework.aop.interceptor.AsyncUncaughtExceptionHandler;
+import org.springframework.aop.interceptor.SimpleAsyncUncaughtExceptionHandler;
 import org.springframework.beans.factory.support.DefaultListableBeanFactory;
 import org.springframework.beans.factory.support.RootBeanDefinition;
 import org.springframework.core.task.SimpleAsyncTaskExecutor;
@@ -31,6 +34,7 @@ import org.springframework.scheduling.annotation.AsyncResult;
 import org.springframework.scheduling.concurrent.ThreadPoolTaskExecutor;
 import org.springframework.tests.Assume;
 import org.springframework.tests.TestGroup;
+import org.springframework.util.ReflectionUtils;
 
 import static org.hamcrest.CoreMatchers.*;
 import static org.hamcrest.Matchers.not;
@@ -40,6 +44,7 @@ import static org.junit.Assert.*;
  * Unit tests for {@link AnnotationAsyncExecutionAspect}.
  *
  * @author Ramnivas Laddad
+ * @author Stephane Nicoll
  */
 public class AnnotationAsyncExecutionAspectTests {
 
@@ -47,6 +52,8 @@ public class AnnotationAsyncExecutionAspectTests {
 
 	private CountingExecutor executor;
 
+	private AsyncUncaughtExceptionHandler defaultExceptionHandler = new SimpleAsyncUncaughtExceptionHandler();
+
 	@Before
 	public void setUp() {
 		Assume.group(TestGroup.PERFORMANCE);
@@ -134,6 +141,46 @@ public class AnnotationAsyncExecutionAspectTests {
 		assertThat(e1Thread.get().getName(), startsWith("e1-"));
 	}
 
+	@Test
+	public void exceptionHandlerCalled() {
+		Method m = ReflectionUtils.findMethod(ClassWithException.class, "failWithVoid");
+		TestableAsyncUncaughtExceptionHandler exceptionHandler = new TestableAsyncUncaughtExceptionHandler();
+		AnnotationAsyncExecutionAspect.aspectOf().setExceptionHandler(exceptionHandler);
+		try {
+			assertFalse("Handler should not have been called", exceptionHandler.isCalled());
+			ClassWithException obj = new ClassWithException();
+			obj.failWithVoid();
+			exceptionHandler.await(3000);
+			exceptionHandler.assertCalledWith(m, UnsupportedOperationException.class);
+		}
+		finally {
+			AnnotationAsyncExecutionAspect.aspectOf().setExceptionHandler(defaultExceptionHandler);
+		}
+	}
+
+	@Test
+	public void exceptionHandlerNeverThrowsUnexpectedException() {
+		Method m = ReflectionUtils.findMethod(ClassWithException.class, "failWithVoid");
+		TestableAsyncUncaughtExceptionHandler exceptionHandler = new TestableAsyncUncaughtExceptionHandler(true);
+		AnnotationAsyncExecutionAspect.aspectOf().setExceptionHandler(exceptionHandler);
+		try {
+			assertFalse("Handler should not have been called", exceptionHandler.isCalled());
+			ClassWithException obj = new ClassWithException();
+			try {
+				obj.failWithVoid();
+				exceptionHandler.await(3000);
+				exceptionHandler.assertCalledWith(m, UnsupportedOperationException.class);
+			}
+			catch (Exception ex) {
+				fail("No unexpected exception should have been received but got " + ex.getMessage());
+			}
+		}
+		finally {
+			AnnotationAsyncExecutionAspect.aspectOf().setExceptionHandler(defaultExceptionHandler);
+
+		}
+	}
+
 
 	@SuppressWarnings("serial")
 	private static class CountingExecutor extends SimpleAsyncTaskExecutor {
@@ -224,4 +271,12 @@ public class AnnotationAsyncExecutionAspectTests {
 			return new AsyncResult<Thread>(Thread.currentThread());
 		}
 	}
+
+	static class ClassWithException {
+
+		@Async
+		public void failWithVoid() {
+			 throw new UnsupportedOperationException("failWithVoid");
+		}
+	}
 }

spring-aspects/src/test/java/org/springframework/scheduling/aspectj/TestableAsyncUncaughtExceptionHandler.java

@@ -0,0 +1,87 @@
+/*
+ * 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.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.springframework.scheduling.aspectj;
+
+import java.lang.reflect.Method;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+
+import org.springframework.aop.interceptor.AsyncUncaughtExceptionHandler;
+
+import static org.junit.Assert.*;
+
+/**
+ * A {@link AsyncUncaughtExceptionHandler} implementation used for testing purposes.
+ *
+ * @author Stephane Nicoll
+ */
+class TestableAsyncUncaughtExceptionHandler
+		implements AsyncUncaughtExceptionHandler {
+
+	private final CountDownLatch latch = new CountDownLatch(1);
+
+	private UncaughtExceptionDescriptor descriptor;
+
+	private final boolean throwUnexpectedException;
+
+	TestableAsyncUncaughtExceptionHandler() {
+		this(false);
+	}
+
+	TestableAsyncUncaughtExceptionHandler(boolean throwUnexpectedException) {
+		this.throwUnexpectedException = throwUnexpectedException;
+	}
+
+	@Override
+	public void handleUncaughtException(Throwable ex, Method method, Object... params) {
+		descriptor = new UncaughtExceptionDescriptor(ex, method);
+		this.latch.countDown();
+		if (throwUnexpectedException) {
+			throw new IllegalStateException("Test exception");
+		}
+	}
+
+	public boolean isCalled() {
+		return descriptor != null;
+	}
+
+	public void assertCalledWith(Method expectedMethod, Class<? extends Throwable> expectedExceptionType) {
+		assertNotNull("Handler not called", descriptor);
+		assertEquals("Wrong exception type", expectedExceptionType, descriptor.ex.getClass());
+		assertEquals("Wrong method", expectedMethod, descriptor.method);
+	}
+
+	public void await(long timeout) {
+		try {
+			this.latch.await(timeout, TimeUnit.MILLISECONDS);
+		}
+		catch (Exception e) {
+			Thread.currentThread().interrupt();
+		}
+	}
+
+	private static class UncaughtExceptionDescriptor {
+		private final Throwable ex;
+
+		private final Method method;
+
+		private UncaughtExceptionDescriptor(Throwable ex, Method method) {
+			this.ex = ex;
+			this.method = method;
+		}
+	}
+}

spring-context/src/main/java/org/springframework/scheduling/annotation/AnnotationAsyncExecutionInterceptor.java

@@ -46,7 +46,7 @@ public class AnnotationAsyncExecutionInterceptor extends AsyncExecutionIntercept
 	 * executor has been qualified at the method level using {@link Async#value()}
 	 */
 	public AnnotationAsyncExecutionInterceptor(Executor defaultExecutor) {
-		this(defaultExecutor, new SimpleAsyncUncaughtExceptionHandler());
+		super(defaultExecutor);
 	}
 
 	/**