From a456a1a0e3ba2aa76ce37c6ccc77b7eb71119cf4 Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Mon, 12 Sep 2011 23:05:01 +0000 Subject: [PATCH] Improve annotation processing thread-safety Commit http://bit.ly/nXumTs ensured that component methods and fields marked with 'common annotations' such as @Resource, @PostConstruct and @PreDestroy are invoked/assigned once and only once, even if multiple instances of the CommonAnnotationBeanPostProcessor are processing the same bean factory. The implementation works against the InjectionMetadata API, adding and removing these members from sets that track whether they are already 'externally managed', i.e. that another CABPP has already handled them, thus avoiding redundant processing. Prior to this change, the #remove operations against these sets were not synchronized. In a single-threaded context this is fine thanks to logic in AbstractAutowireCapableBeanFactory#doCreateBean that checks to see whether a given bean definition has already been post processed. However, as reported by SPR-8598, certain cases involving multiple threads and annotated prototype-scoped beans can cause concurrent modification exceptions during the #remove operation (ostensibly because another thread is attempting to do the same removal at the same time, though this has yet to be reproduced in isolation). Now the sets originally introduced by the commit above are decorated with Collections#synchronizedSet and any iterations over those sets are synchronized properly. This change should have low performance impact as such processing happens at container startup time (save for non-singleton lookups at runtime), and there should be little contention in any case. Issue: SPR-8598 --- ...nitDestroyAnnotationBeanPostProcessor.java | 37 +++++++++++-------- .../factory/annotation/InjectionMetadata.java | 19 ++++++---- 2 files changed, 32 insertions(+), 24 deletions(-) diff --git a/org.springframework.beans/src/main/java/org/springframework/beans/factory/annotation/InitDestroyAnnotationBeanPostProcessor.java b/org.springframework.beans/src/main/java/org/springframework/beans/factory/annotation/InitDestroyAnnotationBeanPostProcessor.java index 4c2db7df864..f584458f440 100644 --- a/org.springframework.beans/src/main/java/org/springframework/beans/factory/annotation/InitDestroyAnnotationBeanPostProcessor.java +++ b/org.springframework.beans/src/main/java/org/springframework/beans/factory/annotation/InitDestroyAnnotationBeanPostProcessor.java @@ -24,6 +24,7 @@ import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.lang.reflect.Modifier; import java.util.Collection; +import java.util.Collections; import java.util.Iterator; import java.util.LinkedHashSet; import java.util.LinkedList; @@ -246,7 +247,7 @@ public class InitDestroyAnnotationBeanPostProcessor public LifecycleMetadata(Class targetClass, Collection initMethods, Collection destroyMethods) { - this.initMethods = new LinkedHashSet(); + this.initMethods = Collections.synchronizedSet(new LinkedHashSet()); for (LifecycleElement element : initMethods) { if (logger.isDebugEnabled()) { logger.debug("Found init method on class [" + targetClass.getName() + "]: " + element); @@ -254,7 +255,7 @@ public class InitDestroyAnnotationBeanPostProcessor this.initMethods.add(element); } - this.destroyMethods = new LinkedHashSet(); + this.destroyMethods = Collections.synchronizedSet(new LinkedHashSet()); for (LifecycleElement element : destroyMethods) { if (logger.isDebugEnabled()) { logger.debug("Found destroy method on class [" + targetClass.getName() + "]: " + element); @@ -264,22 +265,26 @@ public class InitDestroyAnnotationBeanPostProcessor } public void checkConfigMembers(RootBeanDefinition beanDefinition) { - for (Iterator it = this.initMethods.iterator(); it.hasNext();) { - String methodIdentifier = it.next().getIdentifier(); - if (!beanDefinition.isExternallyManagedInitMethod(methodIdentifier)) { - beanDefinition.registerExternallyManagedInitMethod(methodIdentifier); - } - else { - it.remove(); + synchronized(this.initMethods) { + for (Iterator it = this.initMethods.iterator(); it.hasNext();) { + String methodIdentifier = it.next().getIdentifier(); + if (!beanDefinition.isExternallyManagedInitMethod(methodIdentifier)) { + beanDefinition.registerExternallyManagedInitMethod(methodIdentifier); + } + else { + it.remove(); + } } } - for (Iterator it = this.destroyMethods.iterator(); it.hasNext();) { - String methodIdentifier = it.next().getIdentifier(); - if (!beanDefinition.isExternallyManagedDestroyMethod(methodIdentifier)) { - beanDefinition.registerExternallyManagedDestroyMethod(methodIdentifier); - } - else { - it.remove(); + synchronized(this.destroyMethods) { + for (Iterator it = this.destroyMethods.iterator(); it.hasNext();) { + String methodIdentifier = it.next().getIdentifier(); + if (!beanDefinition.isExternallyManagedDestroyMethod(methodIdentifier)) { + beanDefinition.registerExternallyManagedDestroyMethod(methodIdentifier); + } + else { + it.remove(); + } } } } diff --git a/org.springframework.beans/src/main/java/org/springframework/beans/factory/annotation/InjectionMetadata.java b/org.springframework.beans/src/main/java/org/springframework/beans/factory/annotation/InjectionMetadata.java index 227605b5a00..4a1733de307 100644 --- a/org.springframework.beans/src/main/java/org/springframework/beans/factory/annotation/InjectionMetadata.java +++ b/org.springframework.beans/src/main/java/org/springframework/beans/factory/annotation/InjectionMetadata.java @@ -22,6 +22,7 @@ import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Member; import java.lang.reflect.Method; import java.util.Collection; +import java.util.Collections; import java.util.Iterator; import java.util.LinkedHashSet; import java.util.Set; @@ -53,7 +54,7 @@ public class InjectionMetadata { public InjectionMetadata(Class targetClass, Collection elements) { - this.injectedElements = new LinkedHashSet(); + this.injectedElements = Collections.synchronizedSet(new LinkedHashSet()); for (InjectedElement element : elements) { if (logger.isDebugEnabled()) { logger.debug("Found injected element on class [" + targetClass.getName() + "]: " + element); @@ -63,13 +64,15 @@ public class InjectionMetadata { } public void checkConfigMembers(RootBeanDefinition beanDefinition) { - for (Iterator it = this.injectedElements.iterator(); it.hasNext();) { - Member member = it.next().getMember(); - if (!beanDefinition.isExternallyManagedConfigMember(member)) { - beanDefinition.registerExternallyManagedConfigMember(member); - } - else { - it.remove(); + synchronized(this.injectedElements) { + for (Iterator it = this.injectedElements.iterator(); it.hasNext();) { + Member member = it.next().getMember(); + if (!beanDefinition.isExternallyManagedConfigMember(member)) { + beanDefinition.registerExternallyManagedConfigMember(member); + } + else { + it.remove(); + } } } }