Consistent ClassLoader propagation and ConcurrentHashMap setup for AspectJ pointcuts

Issue: SPR-15040

Author: jhoeller

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

@@ -187,13 +187,24 @@ private void checkReadyToMatch() {
 			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 @@ private PointcutExpression buildPointcutExpression(ClassLoader classLoader) {
 	/**
 	 * 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 @@ private FuzzyBoolean contextMatch(Class<?> targetType) {
 		}
 
 		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 @@
 
 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 @@
  * @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 String getLocation() {
 		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.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 void validate(Class<?> aspectClass) throws AopConfigException {
 		}
 	}
 
-	/**
-	 * 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 void setAspectJAdvisorFactory(AspectJAdvisorFactory aspectJAdvisorFactory
 	@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 @@ private class BeanFactoryAspectJAdvisorsBuilderAdapter extends BeanFactoryAspect
 
 		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 @@
 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 @@ private AspectMetadata createAspectMetadata(Class<?> aspectClass, String aspectN
 	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 @@ private MetadataAwareAspectInstanceFactory createAspectInstanceFactory(
 	 * 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 @@
  * 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 AspectMetadata(Class<?> aspectClass, String aspectName) {
 		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 AspectMetadata(Class<?> aspectClass, String aspectName) {
 	 * 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);