No external locking for singleton advice/aspect beans

Issue: SPR-14324
(cherry picked from commit e18e7ec)

Author: jhoeller

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

@@ -93,8 +93,19 @@ public AspectMetadata getAspectMetadata() {
 	}
 
 	public Object getAspectCreationMutex() {
-		return (this.beanFactory instanceof ConfigurableBeanFactory ?
-				((ConfigurableBeanFactory) this.beanFactory).getSingletonMutex() : this);
+		if (this.beanFactory != null) {
+			if (this.beanFactory.isSingleton(name)) {
+				// Rely on singleton semantics provided by the factory -> no local lock.
+				return null;
+			}
+			else if (this.beanFactory instanceof ConfigurableBeanFactory) {
+				// No singleton guarantees from the factory -> let's lock locally but
+				// reuse the factory's singleton lock, just in case a lazy dependency
+				// of our advice bean happens to trigger the singleton lock implicitly...
+				return ((ConfigurableBeanFactory) this.beanFactory).getSingletonMutex();
+			}
+		}
+		return this;
 	}
 
 	/**

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

@@ -49,9 +49,14 @@ public Object getAspectInstance() {
 			if (this.maaif instanceof BeanFactoryAspectInstanceFactory) {
 				mutex = ((BeanFactoryAspectInstanceFactory) this.maaif).getAspectCreationMutex();
 			}
-			synchronized (mutex) {
-				if (this.materialized == null) {
-					this.materialized = this.maaif.getAspectInstance();
+			if (mutex == null) {
+				this.materialized = this.maaif.getAspectInstance();
+			}
+			else {
+				synchronized (mutex) {
+					if (this.materialized == null) {
+						this.materialized = this.maaif.getAspectInstance();
+					}
 				}
 			}
 		}

spring-aop/src/main/java/org/springframework/aop/support/AbstractBeanFactoryPointcutAdvisor.java

@@ -46,7 +46,7 @@ public abstract class AbstractBeanFactoryPointcutAdvisor extends AbstractPointcu
 
 	private BeanFactory beanFactory;
 
-	private transient Advice advice;
+	private transient volatile Advice advice;
 
 	private transient volatile Object adviceMonitor = new Object();
 
@@ -98,12 +98,28 @@ public void setAdvice(Advice advice) {
 
 	@Override
 	public Advice getAdvice() {
-		synchronized (this.adviceMonitor) {
-			if (this.advice == null && this.adviceBeanName != null) {
-				Assert.state(this.beanFactory != null, "BeanFactory must be set to resolve 'adviceBeanName'");
-				this.advice = this.beanFactory.getBean(this.adviceBeanName, Advice.class);
+		Advice advice = this.advice;
+		if (advice != null || this.adviceBeanName == null) {
+			return advice;
+		}
+
+		Assert.state(this.beanFactory != null, "BeanFactory must be set to resolve 'adviceBeanName'");
+		if (this.beanFactory.isSingleton(this.adviceBeanName)) {
+			// Rely on singleton semantics provided by the factory.
+			advice = this.beanFactory.getBean(this.adviceBeanName, Advice.class);
+			this.advice = advice;
+			return advice;
+		}
+		else {
+			// No singleton guarantees from the factory -> let's lock locally but
+			// reuse the factory's singleton lock, just in case a lazy dependency
+			// of our advice bean happens to trigger the singleton lock implicitly...
+			synchronized (this.adviceMonitor) {
+				if (this.advice == null) {
+					this.advice = this.beanFactory.getBean(this.adviceBeanName, Advice.class);
+				}
+				return this.advice;
 			}
-			return this.advice;
 		}
 	}