From 6b7f3fd94247bb3bc17351021b09c096ffaeec03 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Fri, 28 Oct 2016 23:35:01 +0200 Subject: [PATCH] No external locking for singleton advice/aspect beans Issue: SPR-14324 (cherry picked from commit e18e7ec) --- .../BeanFactoryAspectInstanceFactory.java | 15 ++++++++-- ...ngletonAspectInstanceFactoryDecorator.java | 11 ++++++-- .../AbstractBeanFactoryPointcutAdvisor.java | 28 +++++++++++++++---- 3 files changed, 43 insertions(+), 11 deletions(-) diff --git a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/BeanFactoryAspectInstanceFactory.java b/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/BeanFactoryAspectInstanceFactory.java index 713fb44fce0..eb78d0bfbfe 100644 --- a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/BeanFactoryAspectInstanceFactory.java +++ b/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/BeanFactoryAspectInstanceFactory.java @@ -93,8 +93,19 @@ public class BeanFactoryAspectInstanceFactory implements MetadataAwareAspectInst } 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; } /** diff --git a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/LazySingletonAspectInstanceFactoryDecorator.java b/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/LazySingletonAspectInstanceFactoryDecorator.java index 78df54658c4..0f793cafef8 100644 --- a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/LazySingletonAspectInstanceFactoryDecorator.java +++ b/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/LazySingletonAspectInstanceFactoryDecorator.java @@ -49,9 +49,14 @@ public class LazySingletonAspectInstanceFactoryDecorator implements MetadataAwar 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(); + } } } } diff --git a/spring-aop/src/main/java/org/springframework/aop/support/AbstractBeanFactoryPointcutAdvisor.java b/spring-aop/src/main/java/org/springframework/aop/support/AbstractBeanFactoryPointcutAdvisor.java index 2c2eff5feb5..5401bb96a79 100644 --- a/spring-aop/src/main/java/org/springframework/aop/support/AbstractBeanFactoryPointcutAdvisor.java +++ b/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 abstract class AbstractBeanFactoryPointcutAdvisor extends AbstractPointcu @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; } }