From 5ac5ec10465948d6ce2864c5d94cfcca5af2e574 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 --- .../BeanFactoryAspectInstanceFactory.java | 15 ++++++++-- ...ngletonAspectInstanceFactoryDecorator.java | 12 ++++++-- .../MetadataAwareAspectInstanceFactory.java | 2 +- .../AbstractBeanFactoryPointcutAdvisor.java | 28 +++++++++++++++---- 4 files changed, 45 insertions(+), 12 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 f68fea5627..afb8ceb332 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 @@ -97,8 +97,19 @@ public class BeanFactoryAspectInstanceFactory implements MetadataAwareAspectInst @Override 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 3e76fb47a1..18bf9f327b 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 @@ -48,9 +48,15 @@ public class LazySingletonAspectInstanceFactoryDecorator implements MetadataAwar @Override public Object getAspectInstance() { if (this.materialized == null) { - synchronized (this.maaif.getAspectCreationMutex()) { - if (this.materialized == null) { - this.materialized = this.maaif.getAspectInstance(); + Object mutex = this.maaif.getAspectCreationMutex(); + 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/aspectj/annotation/MetadataAwareAspectInstanceFactory.java b/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/MetadataAwareAspectInstanceFactory.java index a001efbc33..5f89f007c1 100644 --- a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/MetadataAwareAspectInstanceFactory.java +++ b/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/MetadataAwareAspectInstanceFactory.java @@ -41,7 +41,7 @@ public interface MetadataAwareAspectInstanceFactory extends AspectInstanceFactor /** * Return the best possible creation mutex for this factory. - * @return the mutex object (never {@code null}) + * @return the mutex object (may be {@code null} for no mutex to use) * @since 4.3 */ Object getAspectCreationMutex(); 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 2c2eff5feb..5401bb96a7 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; } }