diff --git a/spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJExpressionPointcut.java b/spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJExpressionPointcut.java index 6d3629b10f..5a805052d6 100644 --- a/spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJExpressionPointcut.java +++ b/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); } } diff --git a/spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJExpressionPointcutAdvisor.java b/spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJExpressionPointcutAdvisor.java index 9de941958c..1c89754195 100644 --- a/spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJExpressionPointcutAdvisor.java +++ b/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) { - this.pointcut.setParameterTypes(types); - } - public void setParameterNames(String... names) { this.pointcut.setParameterNames(names); } + public void setParameterTypes(Class... types) { + this.pointcut.setParameterTypes(types); + } + + @Override + public void setBeanFactory(BeanFactory beanFactory) { + this.pointcut.setBeanFactory(beanFactory); + } + + @Override + public Pointcut getPointcut() { + return this.pointcut; + } + } diff --git a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/AbstractAspectJAdvisorFactory.java b/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/AbstractAspectJAdvisorFactory.java index 40d2e28036..0a14954819 100644 --- a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/AbstractAspectJAdvisorFactory.java +++ b/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 should only be one anyway...) diff --git a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/AnnotationAwareAspectJAutoProxyCreator.java b/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/AnnotationAwareAspectJAutoProxyCreator.java index 8d0fd175cc..25369cb1fc 100644 --- a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/AnnotationAwareAspectJAutoProxyCreator.java +++ b/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/AnnotationAwareAspectJAutoProxyCreator.java @@ -50,7 +50,7 @@ public class AnnotationAwareAspectJAutoProxyCreator extends AspectJAwareAdvisorA private List 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); } diff --git a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/AspectJProxyFactory.java b/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/AspectJProxyFactory.java index 5f87a852dd..a6049ba271 100644 --- a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/AspectJProxyFactory.java +++ b/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, Object> aspectCache = new HashMap<>(); + private static final Map, 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; } diff --git a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/AspectMetadata.java b/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/AspectMetadata.java index dd8823ec1c..eefecd1592 100644 --- a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/AspectMetadata.java +++ b/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. * - *

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". + *

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); diff --git a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/BeanFactoryAspectJAdvisorsBuilder.java b/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/BeanFactoryAspectJAdvisorsBuilder.java index 665eb648fe..e481eb9322 100644 --- a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/BeanFactoryAspectJAdvisorsBuilder.java +++ b/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 aspectBeanNames; + private volatile List aspectBeanNames; - private final Map> advisorsCache = new HashMap<>(); + private final Map> advisorsCache = new ConcurrentHashMap<>(); - private final Map aspectFactoryCache = - new HashMap<>(); + private final Map 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 buildAspectJAdvisors() { - List aspectNames = null; + List aspectNames = this.aspectBeanNames; - synchronized (this) { - aspectNames = this.aspectBeanNames; - if (aspectNames == null) { - List 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 classAdvisors = this.advisorFactory.getAdvisors(factory); - if (this.beanFactory.isSingleton(beanName)) { - this.advisorsCache.put(beanName, classAdvisors); + if (aspectNames == null) { + synchronized (this) { + aspectNames = this.aspectBeanNames; + if (aspectNames == null) { + List 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 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; } } diff --git a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/ReflectiveAspectJAdvisorFactory.java b/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/ReflectiveAspectJAdvisorFactory.java index 49aaaababd..2cd3497d1b 100644 --- a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/ReflectiveAspectJAdvisorFactory.java +++ b/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 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; }