Consistent ClassLoader propagation and ConcurrentHashMap setup for AspectJ pointcuts

Issue: SPR-15040

spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJExpressionPointcut.java

@@ -187,13 +187,24 @@ public class AspectJExpressionPointcut extends AbstractExpressionPointcut
 			throw new IllegalStateException("Must set property 'expression' before attempting to match");
 		}
 		if (this.pointcutExpression == null) {
-			this.pointcutClassLoader = (this.beanFactory instanceof ConfigurableBeanFactory ?
-					((ConfigurableBeanFactory) this.beanFactory).getBeanClassLoader() :
-					ClassUtils.getDefaultClassLoader());
+			this.pointcutClassLoader = determinePointcutClassLoader();
 			this.pointcutExpression = buildPointcutExpression(this.pointcutClassLoader);
 		}
 	}
 
+	/**
+	 * Determine the ClassLoader to use for pointcut evaluation.
+	 */
+	private ClassLoader determinePointcutClassLoader() {
+		if (this.beanFactory instanceof ConfigurableBeanFactory) {
+			return ((ConfigurableBeanFactory) this.beanFactory).getBeanClassLoader();
+		}
+		if (this.pointcutDeclarationScope != null) {
+			return this.pointcutDeclarationScope.getClassLoader();
+		}
+		return ClassUtils.getDefaultClassLoader();
+	}
+
 	/**
 	 * Build the underlying AspectJ pointcut expression.
 	 */
@@ -211,10 +222,10 @@ public class AspectJExpressionPointcut extends AbstractExpressionPointcut
 	/**
 	 * Initialize the underlying AspectJ pointcut parser.
 	 */
-	private PointcutParser initializePointcutParser(ClassLoader cl) {
+	private PointcutParser initializePointcutParser(ClassLoader classLoader) {
 		PointcutParser parser = PointcutParser
 				.getPointcutParserSupportingSpecifiedPrimitivesAndUsingSpecifiedClassLoaderForResolution(
-						SUPPORTED_PRIMITIVES, cl);
+						SUPPORTED_PRIMITIVES, classLoader);
 		parser.registerPointcutDesignatorHandler(new BeanPointcutDesignatorHandler());
 		return parser;
 	}
@@ -608,7 +619,8 @@ public class AspectJExpressionPointcut extends AbstractExpressionPointcut
 		}
 
 		private boolean matchesBean(String advisedBeanName) {
-			return BeanFactoryAnnotationUtils.isQualifierMatch(this.expressionPattern::matches, advisedBeanName, beanFactory);
+			return BeanFactoryAnnotationUtils.isQualifierMatch(
+					this.expressionPattern::matches, advisedBeanName, beanFactory);
 		}
 	}
 

spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJExpressionPointcutAdvisor.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2014 the original author or authors.
+ * Copyright 2002-2016 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,6 +18,8 @@ package org.springframework.aop.aspectj;
 
 import org.springframework.aop.Pointcut;
 import org.springframework.aop.support.AbstractGenericPointcutAdvisor;
+import org.springframework.beans.factory.BeanFactory;
+import org.springframework.beans.factory.BeanFactoryAware;
 
 /**
  * Spring AOP Advisor that can be used for any AspectJ pointcut expression.
@@ -26,16 +28,11 @@ import org.springframework.aop.support.AbstractGenericPointcutAdvisor;
  * @since 2.0
  */
 @SuppressWarnings("serial")
-public class AspectJExpressionPointcutAdvisor extends AbstractGenericPointcutAdvisor {
+public class AspectJExpressionPointcutAdvisor extends AbstractGenericPointcutAdvisor implements BeanFactoryAware {
 
 	private final AspectJExpressionPointcut pointcut = new AspectJExpressionPointcut();
 
 
-	@Override
-	public Pointcut getPointcut() {
-		return this.pointcut;
-	}
-
 	public void setExpression(String expression) {
 		this.pointcut.setExpression(expression);
 	}
@@ -52,12 +49,22 @@ public class AspectJExpressionPointcutAdvisor extends AbstractGenericPointcutAdv
 		return this.pointcut.getLocation();
 	}
 
-	public void setParameterTypes(Class<?>[] types) {
+	public void setParameterNames(String... names) {
+		this.pointcut.setParameterNames(names);
+	}
+
+	public void setParameterTypes(Class<?>... types) {
 		this.pointcut.setParameterTypes(types);
 	}
 
-	public void setParameterNames(String... names) {
-		this.pointcut.setParameterNames(names);
+	@Override
+	public void setBeanFactory(BeanFactory beanFactory) {
+		this.pointcut.setBeanFactory(beanFactory);
+	}
+
+	@Override
+	public Pointcut getPointcut() {
+		return this.pointcut;
 	}
 
 }

spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/AbstractAspectJAdvisorFactory.java

@@ -38,7 +38,6 @@ import org.aspectj.lang.reflect.AjType;
 import org.aspectj.lang.reflect.AjTypeSystem;
 import org.aspectj.lang.reflect.PerClauseKind;
 
-import org.springframework.aop.aspectj.AspectJExpressionPointcut;
 import org.springframework.aop.framework.AopConfigException;
 import org.springframework.core.ParameterNameDiscoverer;
 import org.springframework.core.annotation.AnnotationUtils;
@@ -121,49 +120,6 @@ public abstract class AbstractAspectJAdvisorFactory implements AspectJAdvisorFac
 		}
 	}
 
-	/**
-	 * The pointcut and advice annotations both have an "argNames" member which contains a
-	 * comma-separated list of the argument names. We use this (if non-empty) to build the
-	 * formal parameters for the pointcut.
-	 */
-	protected AspectJExpressionPointcut createPointcutExpression(
-			Method annotatedMethod, Class<?> declarationScope, String[] pointcutParameterNames) {
-
-		Class<?> [] pointcutParameterTypes = new Class<?>[0];
-		if (pointcutParameterNames != null) {
-			pointcutParameterTypes = extractPointcutParameterTypes(pointcutParameterNames,annotatedMethod);
-		}
-
-		AspectJExpressionPointcut ajexp =
-				new AspectJExpressionPointcut(declarationScope,pointcutParameterNames,pointcutParameterTypes);
-		ajexp.setLocation(annotatedMethod.toString());
-		return ajexp;
-	}
-
-	/**
-	 * Create the pointcut parameters needed by aspectj based on the given argument names
-	 * and the argument types that are available from the adviceMethod. Needs to take into
-	 * account (ignore) any JoinPoint based arguments as these are not pointcut context but
-	 * rather part of the advice execution context (thisJoinPoint, thisJoinPointStaticPart)
-	 */
-	private Class<?>[] extractPointcutParameterTypes(String[] argNames, Method adviceMethod) {
-		Class<?>[] ret = new Class<?>[argNames.length];
-		Class<?>[] paramTypes = adviceMethod.getParameterTypes();
-		if (argNames.length > paramTypes.length) {
-			throw new IllegalStateException("Expecting at least " + argNames.length +
-					" arguments in the advice declaration, but only found " + paramTypes.length);
-		}
-
-		// Make the simplifying assumption for now that all of the JoinPoint based arguments
-		// come first in the advice declaration.
-		int typeOffset = paramTypes.length - argNames.length;
-		for (int i = 0; i < ret.length; i++) {
-			ret[i] = paramTypes[i + typeOffset];
-		}
-		return ret;
-	}
-
-
 	/**
 	 * Find and return the first AspectJ annotation on the given method
 	 * (there <i>should</i> only be one anyway...)

spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/AnnotationAwareAspectJAutoProxyCreator.java

@@ -50,7 +50,7 @@ public class AnnotationAwareAspectJAutoProxyCreator extends AspectJAwareAdvisorA
 
 	private List<Pattern> includePatterns;
 
-	private AspectJAdvisorFactory aspectJAdvisorFactory = new ReflectiveAspectJAdvisorFactory();
+	private AspectJAdvisorFactory aspectJAdvisorFactory;
 
 	private BeanFactoryAspectJAdvisorsBuilder aspectJAdvisorsBuilder;
 
@@ -74,6 +74,9 @@ public class AnnotationAwareAspectJAutoProxyCreator extends AspectJAwareAdvisorA
 	@Override
 	protected void initBeanFactory(ConfigurableListableBeanFactory beanFactory) {
 		super.initBeanFactory(beanFactory);
+		if (this.aspectJAdvisorFactory == null) {
+			this.aspectJAdvisorFactory = new ReflectiveAspectJAdvisorFactory(beanFactory);
+		}
 		this.aspectJAdvisorsBuilder =
 				new BeanFactoryAspectJAdvisorsBuilderAdapter(beanFactory, this.aspectJAdvisorFactory);
 	}
@@ -130,6 +133,7 @@ public class AnnotationAwareAspectJAutoProxyCreator extends AspectJAwareAdvisorA
 
 		public BeanFactoryAspectJAdvisorsBuilderAdapter(
 				ListableBeanFactory beanFactory, AspectJAdvisorFactory advisorFactory) {
+
 			super(beanFactory, advisorFactory);
 		}
 

spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/AspectJProxyFactory.java

@@ -16,9 +16,9 @@
 
 package org.springframework.aop.aspectj.annotation;
 
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
 
 import org.aspectj.lang.reflect.PerClauseKind;
 
@@ -50,7 +50,7 @@ import org.springframework.util.ClassUtils;
 public class AspectJProxyFactory extends ProxyCreatorSupport {
 
 	/** Cache for singleton aspect instances */
-	private static final Map<Class<?>, Object> aspectCache = new HashMap<>();
+	private static final Map<Class<?>, Object> aspectCache = new ConcurrentHashMap<>();
 
 	private final AspectJAdvisorFactory aspectFactory = new ReflectiveAspectJAdvisorFactory();
 
@@ -144,7 +144,7 @@ public class AspectJProxyFactory extends ProxyCreatorSupport {
 	private MetadataAwareAspectInstanceFactory createAspectInstanceFactory(
 			AspectMetadata am, Class<?> aspectClass, String aspectName) {
 
-		MetadataAwareAspectInstanceFactory instanceFactory = null;
+		MetadataAwareAspectInstanceFactory instanceFactory;
 		if (am.getAjType().getPerClause().getKind() == PerClauseKind.SINGLETON) {
 			// Create a shared aspect instance.
 			Object instance = getSingletonAspectInstance(aspectClass);
@@ -162,15 +162,20 @@ public class AspectJProxyFactory extends ProxyCreatorSupport {
 	 * is created if one cannot be found in the instance cache.
 	 */
 	private Object getSingletonAspectInstance(Class<?> aspectClass) {
-		synchronized (aspectCache) {
-			Object instance = aspectCache.get(aspectClass);
-			if (instance != null) {
-				return instance;
+		// Quick check without a lock...
+		Object instance = aspectCache.get(aspectClass);
+		if (instance == null) {
+			synchronized (aspectCache) {
+				// To be safe, check within full lock now...
+				instance = aspectCache.get(aspectClass);
+				if (instance != null) {
+					return instance;
+				}
+				instance = new SimpleAspectInstanceFactory(aspectClass).getAspectInstance();
+				aspectCache.put(aspectClass, instance);
 			}
-			instance = new SimpleAspectInstanceFactory(aspectClass).getAspectInstance();
-			aspectCache.put(aspectClass, instance);
-			return instance;
 		}
+		return instance;
 	}
 
 

spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/AspectMetadata.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2015 the original author or authors.
+ * Copyright 2002-2016 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,9 +35,8 @@ import org.springframework.aop.support.ComposablePointcut;
  * Metadata for an AspectJ aspect class, with an additional Spring AOP pointcut
  * for the per clause.
  *
- * <p>Uses AspectJ 5 AJType reflection API, so is only supported on Java 5.
- * Enables us to work with different AspectJ instantiation models such as
- * "singleton", "pertarget" and "perthis".
+ * <p>Uses AspectJ 5 AJType reflection API, enabling us to work with different
+ * AspectJ instantiation models such as "singleton", "pertarget" and "perthis".
  *
  * @author Rod Johnson
  * @author Juergen Hoeller
@@ -102,20 +101,22 @@ public class AspectMetadata implements Serializable {
 		this.ajType = ajType;
 
 		switch (this.ajType.getPerClause().getKind()) {
-			case SINGLETON :
+			case SINGLETON:
 				this.perClausePointcut = Pointcut.TRUE;
 				return;
-			case PERTARGET : case PERTHIS :
+			case PERTARGET:
+			case PERTHIS:
 				AspectJExpressionPointcut ajexp = new AspectJExpressionPointcut();
-				ajexp.setLocation("@Aspect annotation on " + aspectClass.getName());
+				ajexp.setLocation(aspectClass.getName());
 				ajexp.setExpression(findPerClause(aspectClass));
+				ajexp.setPointcutDeclarationScope(aspectClass);
 				this.perClausePointcut = ajexp;
 				return;
-			case PERTYPEWITHIN :
+			case PERTYPEWITHIN:
 				// Works with a type pattern
 				this.perClausePointcut = new ComposablePointcut(new TypePatternClassFilter(findPerClause(aspectClass)));
 				return;
-			default :
+			default:
 				throw new AopConfigException(
 						"PerClause " + ajType.getPerClause().getKind() + " not supported by Spring AOP for " + aspectClass);
 		}
@@ -125,8 +126,6 @@ public class AspectMetadata implements Serializable {
 	 * Extract contents from String of form {@code pertarget(contents)}.
 	 */
 	private String findPerClause(Class<?> aspectClass) {
-		// TODO when AspectJ provides this, we can remove this hack. Hence we don't
-		// bother to make it elegant. Or efficient. Or robust :-)
 		String str = aspectClass.getAnnotation(Aspect.class).value();
 		str = str.substring(str.indexOf("(") + 1);
 		str = str.substring(0, str.length() - 1);

spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/BeanFactoryAspectJAdvisorsBuilder.java

@@ -17,10 +17,10 @@
 package org.springframework.aop.aspectj.annotation;
 
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
 
 import org.aspectj.lang.reflect.PerClauseKind;
 
@@ -43,12 +43,11 @@ public class BeanFactoryAspectJAdvisorsBuilder {
 
 	private final AspectJAdvisorFactory advisorFactory;
 
-	private List<String> aspectBeanNames;
+	private volatile List<String> aspectBeanNames;
 
-	private final Map<String, List<Advisor>> advisorsCache = new HashMap<>();
+	private final Map<String, List<Advisor>> advisorsCache = new ConcurrentHashMap<>();
 
-	private final Map<String, MetadataAwareAspectInstanceFactory> aspectFactoryCache =
-			new HashMap<>();
+	private final Map<String, MetadataAwareAspectInstanceFactory> aspectFactoryCache = new ConcurrentHashMap<>();
 
 
 	/**
@@ -56,7 +55,7 @@ public class BeanFactoryAspectJAdvisorsBuilder {
 	 * @param beanFactory the ListableBeanFactory to scan
 	 */
 	public BeanFactoryAspectJAdvisorsBuilder(ListableBeanFactory beanFactory) {
-		this(beanFactory, new ReflectiveAspectJAdvisorFactory());
+		this(beanFactory, new ReflectiveAspectJAdvisorFactory(beanFactory));
 	}
 
 	/**
@@ -80,56 +79,57 @@ public class BeanFactoryAspectJAdvisorsBuilder {
 	 * @see #isEligibleBean
 	 */
 	public List<Advisor> buildAspectJAdvisors() {
-		List<String> aspectNames = null;
-
-		synchronized (this) {
-			aspectNames = this.aspectBeanNames;
-			if (aspectNames == null) {
-				List<Advisor> advisors = new LinkedList<>();
-				aspectNames = new LinkedList<>();
-				String[] beanNames =
-						BeanFactoryUtils.beanNamesForTypeIncludingAncestors(this.beanFactory, Object.class, true, false);
-				for (String beanName : beanNames) {
-					if (!isEligibleBean(beanName)) {
-						continue;
-					}
-					// We must be careful not to instantiate beans eagerly as in this
-					// case they would be cached by the Spring container but would not
-					// have been weaved
-					Class<?> beanType = this.beanFactory.getType(beanName);
-					if (beanType == null) {
-						continue;
-					}
-					if (this.advisorFactory.isAspect(beanType)) {
-						aspectNames.add(beanName);
-						AspectMetadata amd = new AspectMetadata(beanType, beanName);
-						if (amd.getAjType().getPerClause().getKind() == PerClauseKind.SINGLETON) {
-							MetadataAwareAspectInstanceFactory factory =
-									new BeanFactoryAspectInstanceFactory(this.beanFactory, beanName);
-							List<Advisor> classAdvisors = this.advisorFactory.getAdvisors(factory);
-							if (this.beanFactory.isSingleton(beanName)) {
-								this.advisorsCache.put(beanName, classAdvisors);
+		List<String> aspectNames = this.aspectBeanNames;
+
+		if (aspectNames == null) {
+			synchronized (this) {
+				aspectNames = this.aspectBeanNames;
+				if (aspectNames == null) {
+					List<Advisor> advisors = new LinkedList<>();
+					aspectNames = new LinkedList<>();
+					String[] beanNames = BeanFactoryUtils.beanNamesForTypeIncludingAncestors(
+							this.beanFactory, Object.class, true, false);
+					for (String beanName : beanNames) {
+						if (!isEligibleBean(beanName)) {
+							continue;
+						}
+						// We must be careful not to instantiate beans eagerly as in this case they
+						// would be cached by the Spring container but would not have been weaved.
+						Class<?> beanType = this.beanFactory.getType(beanName);
+						if (beanType == null) {
+							continue;
+						}
+						if (this.advisorFactory.isAspect(beanType)) {
+							aspectNames.add(beanName);
+							AspectMetadata amd = new AspectMetadata(beanType, beanName);
+							if (amd.getAjType().getPerClause().getKind() == PerClauseKind.SINGLETON) {
+								MetadataAwareAspectInstanceFactory factory =
+										new BeanFactoryAspectInstanceFactory(this.beanFactory, beanName);
+								List<Advisor> classAdvisors = this.advisorFactory.getAdvisors(factory);
+								if (this.beanFactory.isSingleton(beanName)) {
+									this.advisorsCache.put(beanName, classAdvisors);
+								}
+								else {
+									this.aspectFactoryCache.put(beanName, factory);
+								}
+								advisors.addAll(classAdvisors);
 							}
 							else {
+								// Per target or per this.
+								if (this.beanFactory.isSingleton(beanName)) {
+									throw new IllegalArgumentException("Bean with name '" + beanName +
+											"' is a singleton, but aspect instantiation model is not singleton");
+								}
+								MetadataAwareAspectInstanceFactory factory =
+										new PrototypeAspectInstanceFactory(this.beanFactory, beanName);
 								this.aspectFactoryCache.put(beanName, factory);
+								advisors.addAll(this.advisorFactory.getAdvisors(factory));
 							}
-							advisors.addAll(classAdvisors);
-						}
-						else {
-							// Per target or per this.
-							if (this.beanFactory.isSingleton(beanName)) {
-								throw new IllegalArgumentException("Bean with name '" + beanName +
-										"' is a singleton, but aspect instantiation model is not singleton");
-							}
-							MetadataAwareAspectInstanceFactory factory =
-									new PrototypeAspectInstanceFactory(this.beanFactory, beanName);
-							this.aspectFactoryCache.put(beanName, factory);
-							advisors.addAll(this.advisorFactory.getAdvisors(factory));
 						}
 					}
+					this.aspectBeanNames = aspectNames;
+					return advisors;
 				}
-				this.aspectBeanNames = aspectNames;
-				return advisors;
 			}
 		}
 

spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/ReflectiveAspectJAdvisorFactory.java

@@ -46,6 +46,7 @@ import org.springframework.aop.aspectj.AspectJMethodBeforeAdvice;
 import org.springframework.aop.aspectj.DeclareParentsAdvisor;
 import org.springframework.aop.framework.AopConfigException;
 import org.springframework.aop.support.DefaultPointcutAdvisor;
+import org.springframework.beans.factory.BeanFactory;
 import org.springframework.core.annotation.AnnotationUtils;
 import org.springframework.core.convert.converter.Converter;
 import org.springframework.core.convert.converter.ConvertingComparator;
@@ -95,6 +96,30 @@ public class ReflectiveAspectJAdvisorFactory extends AbstractAspectJAdvisorFacto
 	}
 
 
+	private final BeanFactory beanFactory;
+
+
+	/**
+	 * Create a new {@code ReflectiveAspectJAdvisorFactory}.
+	 */
+	public ReflectiveAspectJAdvisorFactory() {
+		this(null);
+	}
+
+	/**
+	 * Create a new {@code ReflectiveAspectJAdvisorFactory}, propagating the given
+	 * {@link BeanFactory} to the created {@link AspectJExpressionPointcut} instances,
+	 * for bean pointcut handling as well as consistent {@link ClassLoader} resolution.
+	 * @param beanFactory the BeanFactory to propagate (may be {@code null}}
+	 * @since 4.3.6
+	 * @see AspectJExpressionPointcut#setBeanFactory
+	 * @see org.springframework.beans.factory.config.ConfigurableBeanFactory#getBeanClassLoader()
+	 */
+	public ReflectiveAspectJAdvisorFactory(BeanFactory beanFactory) {
+		this.beanFactory = beanFactory;
+	}
+
+
 	@Override
 	public List<Advisor> getAdvisors(MetadataAwareAspectInstanceFactory aspectInstanceFactory) {
 		Class<?> aspectClass = aspectInstanceFactory.getAspectMetadata().getAspectClass();
@@ -161,9 +186,7 @@ public class ReflectiveAspectJAdvisorFactory extends AbstractAspectJAdvisorFacto
 		}
 
 		if (DeclareParents.class == declareParents.defaultImpl()) {
-			// This is what comes back if it wasn't set. This seems bizarre...
-			// TODO this restriction possibly should be relaxed
-			throw new IllegalStateException("defaultImpl must be set on DeclareParents");
+			throw new IllegalStateException("'defaultImpl' attribute must be set on DeclareParents");
 		}
 
 		return new DeclareParentsAdvisor(
@@ -197,6 +220,7 @@ public class ReflectiveAspectJAdvisorFactory extends AbstractAspectJAdvisorFacto
 		AspectJExpressionPointcut ajexp =
 				new AspectJExpressionPointcut(candidateAspectClass, new String[0], new Class<?>[0]);
 		ajexp.setExpression(aspectJAnnotation.getPointcutExpression());
+		ajexp.setBeanFactory(this.beanFactory);
 		return ajexp;
 	}