From 3fb11870d9e9fc47651c08442ac7e85140788579 Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Sat, 19 May 2012 11:04:36 +0300 Subject: [PATCH] Polish async method execution infrastructure In anticipation of substantive changes required to implement @Async executor qualification, the following updates have been made to the components and infrastructure supporting @Async functionality: - Fix trailing whitespace and indentation errors - Fix generics warnings - Add Javadoc where missing, update to use {@code} tags, etc. - Avoid NPE in AopUtils#canApply - Organize imports to follow conventions - Remove System.out.println statements from tests - Correct various punctuation and grammar problems --- .../AsyncExecutionInterceptor.java | 35 +++++----- .../springframework/aop/support/AopUtils.java | 1 + .../aspectj/AbstractAsyncExecutionAspect.aj | 16 ++++- .../aspectj/AnnotationAsyncExecutionAspect.aj | 33 +++++----- .../AnnotationAsyncExecutionAspectTests.java | 65 ++++++++++--------- .../scheduling/annotation/Async.java | 12 ++-- .../annotation/AsyncAnnotationAdvisor.java | 9 +-- .../scheduling/config/spring-task-3.2.xsd | 2 +- .../annotation/AsyncExecutionTests.java | 15 ++--- .../annotation/EnableAsyncTests.java | 14 ++-- 10 files changed, 110 insertions(+), 92 deletions(-) diff --git a/spring-aop/src/main/java/org/springframework/aop/interceptor/AsyncExecutionInterceptor.java b/spring-aop/src/main/java/org/springframework/aop/interceptor/AsyncExecutionInterceptor.java index 1cc6f8e0221a..b8b6f257d9e8 100644 --- a/spring-aop/src/main/java/org/springframework/aop/interceptor/AsyncExecutionInterceptor.java +++ b/spring-aop/src/main/java/org/springframework/aop/interceptor/AsyncExecutionInterceptor.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2009 the original author or authors. + * Copyright 2002-2012 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. @@ -55,14 +55,16 @@ public class AsyncExecutionInterceptor implements MethodInterceptor, Ordered { /** - * Create a new AsyncExecutionInterceptor. - * @param asyncExecutor the Spring AsyncTaskExecutor to delegate to + * Create a new {@code AsyncExecutionInterceptor}. + * @param executor the {@link Executor} (typically a Spring {@link AsyncTaskExecutor} + * or {@link java.util.concurrent.ExecutorService}) to delegate to. */ public AsyncExecutionInterceptor(AsyncTaskExecutor asyncExecutor) { Assert.notNull(asyncExecutor, "TaskExecutor must not be null"); this.asyncExecutor = asyncExecutor; } + /** * Create a new AsyncExecutionInterceptor. * @param asyncExecutor the java.util.concurrent Executor @@ -74,20 +76,21 @@ public AsyncExecutionInterceptor(Executor asyncExecutor) { public Object invoke(final MethodInvocation invocation) throws Throwable { - Future result = this.asyncExecutor.submit(new Callable() { - public Object call() throws Exception { - try { - Object result = invocation.proceed(); - if (result instanceof Future) { - return ((Future) result).get(); + Future result = this.asyncExecutor.submit( + new Callable() { + public Object call() throws Exception { + try { + Object result = invocation.proceed(); + if (result instanceof Future) { + return ((Future) result).get(); + } + } + catch (Throwable ex) { + ReflectionUtils.rethrowException(ex); + } + return null; } - } - catch (Throwable ex) { - ReflectionUtils.rethrowException(ex); - } - return null; - } - }); + }); if (Future.class.isAssignableFrom(invocation.getMethod().getReturnType())) { return result; } diff --git a/spring-aop/src/main/java/org/springframework/aop/support/AopUtils.java b/spring-aop/src/main/java/org/springframework/aop/support/AopUtils.java index f08481efba49..d222d652fc7e 100644 --- a/spring-aop/src/main/java/org/springframework/aop/support/AopUtils.java +++ b/spring-aop/src/main/java/org/springframework/aop/support/AopUtils.java @@ -206,6 +206,7 @@ public static boolean canApply(Pointcut pc, Class targetClass) { * @return whether the pointcut can apply on any method */ public static boolean canApply(Pointcut pc, Class targetClass, boolean hasIntroductions) { + Assert.notNull(pc, "Pointcut must not be null"); if (!pc.getClassFilter().matches(targetClass)) { return false; } diff --git a/spring-aspects/src/main/java/org/springframework/scheduling/aspectj/AbstractAsyncExecutionAspect.aj b/spring-aspects/src/main/java/org/springframework/scheduling/aspectj/AbstractAsyncExecutionAspect.aj index c5402271142e..aaa13647f6a0 100644 --- a/spring-aspects/src/main/java/org/springframework/scheduling/aspectj/AbstractAsyncExecutionAspect.aj +++ b/spring-aspects/src/main/java/org/springframework/scheduling/aspectj/AbstractAsyncExecutionAspect.aj @@ -1,5 +1,5 @@ /* - * Copyright 2002-2010 the original author or authors. + * Copyright 2002-2012 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. @@ -49,10 +49,17 @@ public abstract aspect AbstractAsyncExecutionAspect { } } + /** + * Apply around advice to methods matching the {@link #asyncMethod()} pointcut, + * submit the actual calling of the method to the correct task executor and return + * immediately to the caller. + * @return {@link Future} if the original method returns {@code Future}; {@code null} + * otherwise. + */ Object around() : asyncMethod() { - if (this.asyncExecutor == null) { + if (this.asyncExecutor == null) { return proceed(); - } + } Callable callable = new Callable() { public Object call() throws Exception { Object result = proceed(); @@ -70,6 +77,9 @@ public abstract aspect AbstractAsyncExecutionAspect { } } + /** + * Return the set of joinpoints at which async advice should be applied. + */ public abstract pointcut asyncMethod(); } diff --git a/spring-aspects/src/main/java/org/springframework/scheduling/aspectj/AnnotationAsyncExecutionAspect.aj b/spring-aspects/src/main/java/org/springframework/scheduling/aspectj/AnnotationAsyncExecutionAspect.aj index 328e742670f3..d7144ddedb8a 100644 --- a/spring-aspects/src/main/java/org/springframework/scheduling/aspectj/AnnotationAsyncExecutionAspect.aj +++ b/spring-aspects/src/main/java/org/springframework/scheduling/aspectj/AnnotationAsyncExecutionAspect.aj @@ -1,5 +1,5 @@ /* - * Copyright 2002-2010 the original author or authors. + * Copyright 2002-2012 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. @@ -24,31 +24,32 @@ import org.springframework.scheduling.annotation.Async; * *

This aspect routes methods marked with the {@link Async} annotation * as well as methods in classes marked with the same. Any method expected - * to be routed asynchronously must return either void, {@link Future}, - * or a subtype of {@link Future}. This aspect, therefore, will produce - * a compile-time error for methods that violate this constraint on the return type. - * If, however, a class marked with @Async contains a method that - * violates this constraint, it produces only a warning. - * + * to be routed asynchronously must return either {@code void}, {@link Future}, + * or a subtype of {@link Future}. This aspect, therefore, will produce + * a compile-time error for methods that violate this constraint on the return type. + * If, however, a class marked with {@code @Async} contains a method that violates this + * constraint, it produces only a warning. + * * @author Ramnivas Laddad * @since 3.0.5 */ public aspect AnnotationAsyncExecutionAspect extends AbstractAsyncExecutionAspect { - private pointcut asyncMarkedMethod() + private pointcut asyncMarkedMethod() : execution(@Async (void || Future+) *(..)); - private pointcut asyncTypeMarkedMethod() + private pointcut asyncTypeMarkedMethod() : execution((void || Future+) (@Async *).*(..)); - + public pointcut asyncMethod() : asyncMarkedMethod() || asyncTypeMarkedMethod(); - - declare error: - execution(@Async !(void||Future) *(..)): + + declare error: + execution(@Async !(void||Future) *(..)): "Only methods that return void or Future may have an @Async annotation"; - declare warning: - execution(!(void||Future) (@Async *).*(..)): - "Methods in a class marked with @Async that do not return void or Future will be routed synchronously"; + declare warning: + execution(!(void||Future) (@Async *).*(..)): + "Methods in a class marked with @Async that do not return void or Future will " + + "be routed synchronously"; } diff --git a/spring-aspects/src/test/java/org/springframework/scheduling/aspectj/AnnotationAsyncExecutionAspectTests.java b/spring-aspects/src/test/java/org/springframework/scheduling/aspectj/AnnotationAsyncExecutionAspectTests.java index e2194cfe5cab..43fde1fcaade 100644 --- a/spring-aspects/src/test/java/org/springframework/scheduling/aspectj/AnnotationAsyncExecutionAspectTests.java +++ b/spring-aspects/src/test/java/org/springframework/scheduling/aspectj/AnnotationAsyncExecutionAspectTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2010 the original author or authors. + * Copyright 2002-2012 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. @@ -20,31 +20,31 @@ import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; -import junit.framework.Assert; - -import static junit.framework.Assert.*; - import org.junit.Before; import org.junit.Test; import org.springframework.core.task.SimpleAsyncTaskExecutor; import org.springframework.scheduling.annotation.Async; import org.springframework.scheduling.annotation.AsyncResult; +import static org.junit.Assert.*; + /** + * Unit tests for {@link AnnotationAsyncExecutionAspect}. + * * @author Ramnivas Laddad */ public class AnnotationAsyncExecutionAspectTests { - private static final long WAIT_TIME = 1000; //milli seconds + private static final long WAIT_TIME = 1000; //milliseconds private CountingExecutor executor; - + @Before public void setUp() { executor = new CountingExecutor(); AnnotationAsyncExecutionAspect.aspectOf().setExecutor(executor); } - + @Test public void asyncMethodGetsRoutedAsynchronously() { ClassWithoutAsyncAnnotation obj = new ClassWithoutAsyncAnnotation(); @@ -54,7 +54,7 @@ public void asyncMethodGetsRoutedAsynchronously() { assertEquals(1, executor.submitStartCounter); assertEquals(1, executor.submitCompleteCounter); } - + @Test public void asyncMethodReturningFutureGetsRoutedAsynchronouslyAndReturnsAFuture() throws InterruptedException, ExecutionException { ClassWithoutAsyncAnnotation obj = new ClassWithoutAsyncAnnotation(); @@ -73,8 +73,8 @@ public void syncMethodGetsRoutedSynchronously() { assertEquals(1, obj.counter); assertEquals(0, executor.submitStartCounter); assertEquals(0, executor.submitCompleteCounter); - } - + } + @Test public void voidMethodInAsyncClassGetsRoutedAsynchronously() { ClassWithAsyncAnnotation obj = new ClassWithAsyncAnnotation(); @@ -102,13 +102,14 @@ public void methodReturningNonVoidNonFutureInAsyncClassGetsRoutedSynchronously() assertEquals(5, returnValue); assertEquals(0, executor.submitStartCounter); assertEquals(0, executor.submitCompleteCounter); - } + } + @SuppressWarnings("serial") private static class CountingExecutor extends SimpleAsyncTaskExecutor { int submitStartCounter; int submitCompleteCounter; - + @Override public Future submit(Callable task) { submitStartCounter++; @@ -119,52 +120,56 @@ public Future submit(Callable task) { } return future; } - + public synchronized void waitForCompletion() { try { wait(WAIT_TIME); } catch (InterruptedException e) { - Assert.fail("Didn't finish the async job in " + WAIT_TIME + " milliseconds"); + fail("Didn't finish the async job in " + WAIT_TIME + " milliseconds"); } } } - + + static class ClassWithoutAsyncAnnotation { int counter; - + @Async public void incrementAsync() { counter++; } - + public void increment() { counter++; } - + @Async public Future incrementReturningAFuture() { counter++; return new AsyncResult(5); } - - // It should be an error to attach @Async to a method that returns a non-void - // or non-Future. - // We need to keep this commented out, otherwise there will be a compile-time error. - // Please uncomment and re-comment this periodically to check that the compiler - // produces an error message due to the 'declare error' statement - // in AnnotationAsyncExecutionAspect + + /** + * It should raise an error to attach @Async to a method that returns a non-void + * or non-Future. This method must remain commented-out, otherwise there will be a + * compile-time error. Uncomment to manually verify that the compiler produces an + * error message due to the 'declare error' statement in + * {@link AnnotationAsyncExecutionAspect}. + */ // @Async public int getInt() { // return 0; // } } - + + @Async static class ClassWithAsyncAnnotation { int counter; - + public void increment() { counter++; } - - // Manually check that there is a warning from the 'declare warning' statement in AnnotationAsynchExecutionAspect + + // Manually check that there is a warning from the 'declare warning' statement in + // AnnotationAsyncExecutionAspect public int return5() { return 5; } diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/Async.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/Async.java index c79f9ecd8325..7236747d0803 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/annotation/Async.java +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/Async.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2009 the original author or authors. + * Copyright 2002-2012 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. @@ -28,13 +28,13 @@ * considered as asynchronous. * *

In terms of target method signatures, any parameter types are supported. - * However, the return type is constrained to either void or - * java.util.concurrent.Future. In the latter case, the Future handle - * returned from the proxy will be an actual asynchronous Future that can be used + * However, the return type is constrained to either {@code void} or + * {@link java.util.concurrent.Future}. In the latter case, the {@code Future} handle + * returned from the proxy will be an actual asynchronous {@code Future} that can be used * to track the result of the asynchronous method execution. However, since the * target method needs to implement the same signature, it will have to return - * a temporary Future handle that just passes the return value through: e.g. - * Spring's {@link AsyncResult} or EJB 3.1's javax.ejb.AsyncResult. + * a temporary {@code Future} handle that just passes the return value through: e.g. + * Spring's {@link AsyncResult} or EJB 3.1's {@link javax.ejb.AsyncResult}. * * @author Juergen Hoeller * @since 3.0 diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/AsyncAnnotationAdvisor.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/AsyncAnnotationAdvisor.java index 245ff645b9ca..2e20b2cf6001 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/annotation/AsyncAnnotationAdvisor.java +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/AsyncAnnotationAdvisor.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2009 the original author or authors. + * Copyright 2002-2012 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. @@ -50,6 +50,7 @@ * @see org.springframework.dao.DataAccessException * @see org.springframework.dao.support.PersistenceExceptionTranslator */ +@SuppressWarnings("serial") public class AsyncAnnotationAdvisor extends AbstractPointcutAdvisor { private Advice advice; @@ -58,14 +59,14 @@ public class AsyncAnnotationAdvisor extends AbstractPointcutAdvisor { /** - * Create a new ConcurrencyAnnotationBeanPostProcessor for bean-style configuration. + * Create a new {@code AsyncAnnotationAdvisor} for bean-style configuration. */ public AsyncAnnotationAdvisor() { this(new SimpleAsyncTaskExecutor()); } /** - * Create a new ConcurrencyAnnotationBeanPostProcessor for the given task executor. + * Create a new {@code AsyncAnnotationAdvisor} for the given task executor. * @param executor the task executor to use for asynchronous methods */ @SuppressWarnings("unchecked") @@ -74,7 +75,7 @@ public AsyncAnnotationAdvisor(Executor executor) { asyncAnnotationTypes.add(Async.class); ClassLoader cl = AsyncAnnotationAdvisor.class.getClassLoader(); try { - asyncAnnotationTypes.add((Class) cl.loadClass("javax.ejb.Asynchronous")); + asyncAnnotationTypes.add((Class) cl.loadClass("javax.ejb.Asynchronous")); } catch (ClassNotFoundException ex) { // If EJB 3.1 API not present, simply ignore. diff --git a/spring-context/src/main/resources/org/springframework/scheduling/config/spring-task-3.2.xsd b/spring-context/src/main/resources/org/springframework/scheduling/config/spring-task-3.2.xsd index 8d17fffb8404..86ebacfd3e2e 100644 --- a/spring-context/src/main/resources/org/springframework/scheduling/config/spring-task-3.2.xsd +++ b/spring-context/src/main/resources/org/springframework/scheduling/config/spring-task-3.2.xsd @@ -35,7 +35,7 @@ diff --git a/spring-context/src/test/java/org/springframework/scheduling/annotation/AsyncExecutionTests.java b/spring-context/src/test/java/org/springframework/scheduling/annotation/AsyncExecutionTests.java index da4436b35e1d..209ba73117b7 100644 --- a/spring-context/src/test/java/org/springframework/scheduling/annotation/AsyncExecutionTests.java +++ b/spring-context/src/test/java/org/springframework/scheduling/annotation/AsyncExecutionTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2009 the original author or authors. + * Copyright 2002-2012 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. @@ -18,8 +18,6 @@ import java.util.concurrent.Future; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; import org.junit.Test; import org.springframework.aop.framework.autoproxy.DefaultAdvisorAutoProxyCreator; @@ -27,7 +25,8 @@ import org.springframework.context.ApplicationEvent; import org.springframework.context.ApplicationListener; import org.springframework.context.support.GenericApplicationContext; -import org.springframework.scheduling.annotation.AsyncResult; + +import static org.junit.Assert.*; /** * @author Juergen Hoeller @@ -155,7 +154,6 @@ public void doNothing(int i) { @Async public void doSomething(int i) { - System.out.println(Thread.currentThread().getName() + ": " + i); assertTrue(!Thread.currentThread().getName().equals(originalThreadName)); } @@ -171,7 +169,6 @@ public Future returnSomething(int i) { public static class AsyncClassBean { public void doSomething(int i) { - System.out.println(Thread.currentThread().getName() + ": " + i); assertTrue(!Thread.currentThread().getName().equals(originalThreadName)); } @@ -194,7 +191,6 @@ public interface AsyncInterface { public static class AsyncInterfaceBean implements AsyncInterface { public void doSomething(int i) { - System.out.println(Thread.currentThread().getName() + ": " + i); assertTrue(!Thread.currentThread().getName().equals(originalThreadName)); } @@ -224,7 +220,6 @@ public void doNothing(int i) { } public void doSomething(int i) { - System.out.println(Thread.currentThread().getName() + ": " + i); assertTrue(!Thread.currentThread().getName().equals(originalThreadName)); } @@ -235,7 +230,7 @@ public Future returnSomething(int i) { } - public static class AsyncMethodListener implements ApplicationListener { + public static class AsyncMethodListener implements ApplicationListener { @Async public void onApplicationEvent(ApplicationEvent event) { @@ -246,7 +241,7 @@ public void onApplicationEvent(ApplicationEvent event) { @Async - public static class AsyncClassListener implements ApplicationListener { + public static class AsyncClassListener implements ApplicationListener { public AsyncClassListener() { listenerConstructed++; diff --git a/spring-context/src/test/java/org/springframework/scheduling/annotation/EnableAsyncTests.java b/spring-context/src/test/java/org/springframework/scheduling/annotation/EnableAsyncTests.java index 224da8f4febf..eb403b9a42f4 100644 --- a/spring-context/src/test/java/org/springframework/scheduling/annotation/EnableAsyncTests.java +++ b/spring-context/src/test/java/org/springframework/scheduling/annotation/EnableAsyncTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2011 the original author or authors. + * Copyright 2002-2012 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,18 +16,15 @@ package org.springframework.scheduling.annotation; -import static org.hamcrest.CoreMatchers.is; -import static org.hamcrest.Matchers.startsWith; -import static org.junit.Assert.assertThat; -import static org.junit.Assert.assertTrue; - import java.lang.annotation.ElementType; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; + import java.util.concurrent.Executor; import org.junit.Test; + import org.springframework.aop.Advisor; import org.springframework.aop.framework.Advised; import org.springframework.aop.support.AopUtils; @@ -39,6 +36,11 @@ import org.springframework.core.Ordered; import org.springframework.scheduling.concurrent.ThreadPoolTaskExecutor; +import static org.hamcrest.CoreMatchers.*; +import static org.hamcrest.Matchers.startsWith; + +import static org.junit.Assert.*; + /** * Tests use of @EnableAsync on @Configuration classes. *