From 097bcfb997bd215c7b39635770e34812744f0d9e Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Fri, 25 Sep 2015 15:20:40 +0200 Subject: [PATCH] DefaultListableBeanFactory switches to thread-safe copying for names collections if necessary Issue: SPR-13493 Issue: SPR-13123 Issue: SPR-12503 --- .../factory/support/AbstractBeanFactory.java | 10 +++ .../support/DefaultListableBeanFactory.java | 73 +++++++++++++++---- .../DefaultListableBeanFactoryTests.java | 42 +++++++++-- 3 files changed, 104 insertions(+), 21 deletions(-) diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractBeanFactory.java b/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractBeanFactory.java index 66f5d75e03..fab12fd594 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractBeanFactory.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractBeanFactory.java @@ -1535,6 +1535,16 @@ public abstract class AbstractBeanFactory extends FactoryBeanRegistrySupport imp } } + /** + * Check whether this factory's bean creation phase already started, + * i.e. whether any bean has been marked as created in the meantime. + * @since 4.2.2 + * @see #markBeanAsCreated + */ + protected boolean hasBeanCreationStarted() { + return !this.alreadyCreated.isEmpty(); + } + /** * Get the object for the given bean instance, either the bean * instance itself or its created object in case of a FactoryBean. diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/support/DefaultListableBeanFactory.java b/spring-beans/src/main/java/org/springframework/beans/factory/support/DefaultListableBeanFactory.java index a79be895e4..f7a50cb003 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/support/DefaultListableBeanFactory.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/support/DefaultListableBeanFactory.java @@ -155,7 +155,7 @@ public class DefaultListableBeanFactory extends AbstractAutowireCapableBeanFacto private AutowireCandidateResolver autowireCandidateResolver = new SimpleAutowireCandidateResolver(); /** Map from dependency type to corresponding autowired value */ - private final Map, Object> resolvableDependencies = new HashMap, Object>(16); + private final Map, Object> resolvableDependencies = new ConcurrentHashMap, Object>(16); /** Map of bean definition objects, keyed by bean name */ private final Map beanDefinitionMap = new ConcurrentHashMap(64); @@ -167,16 +167,16 @@ public class DefaultListableBeanFactory extends AbstractAutowireCapableBeanFacto private final Map, String[]> singletonBeanNamesByType = new ConcurrentHashMap, String[]>(64); /** List of bean definition names, in registration order */ - private final List beanDefinitionNames = new ArrayList(64); + private volatile List beanDefinitionNames = new ArrayList(64); /** List of names of manually registered singletons, in registration order */ - private final Set manualSingletonNames = new LinkedHashSet(16); - - /** Whether bean definition metadata may be cached for all beans */ - private boolean configurationFrozen = false; + private volatile Set manualSingletonNames = new LinkedHashSet(16); /** Cached array of bean definition names in case of frozen configuration */ - private String[] frozenBeanDefinitionNames; + private volatile String[] frozenBeanDefinitionNames; + + /** Whether bean definition metadata may be cached for all beans */ + private volatile boolean configurationFrozen = false; /** @@ -848,13 +848,32 @@ public class DefaultListableBeanFactory extends AbstractAutowireCapableBeanFacto "] with [" + beanDefinition + "]"); } } + this.beanDefinitionMap.put(beanName, beanDefinition); } else { - this.beanDefinitionNames.add(beanName); - this.manualSingletonNames.remove(beanName); + if (hasBeanCreationStarted()) { + // Cannot modify startup-time collection elements anymore (for stable iteration) + synchronized (this.beanDefinitionMap) { + this.beanDefinitionMap.put(beanName, beanDefinition); + List updatedDefinitions = new ArrayList(this.beanDefinitionNames.size() + 1); + updatedDefinitions.addAll(this.beanDefinitionNames); + updatedDefinitions.add(beanName); + this.beanDefinitionNames = updatedDefinitions; + if (this.manualSingletonNames.contains(beanName)) { + Set updatedSingletons = new LinkedHashSet(this.manualSingletonNames); + updatedSingletons.remove(beanName); + this.manualSingletonNames = updatedSingletons; + } + } + } + else { + // Still in startup registration phase + this.beanDefinitionMap.put(beanName, beanDefinition); + this.beanDefinitionNames.add(beanName); + this.manualSingletonNames.remove(beanName); + } this.frozenBeanDefinitionNames = null; } - this.beanDefinitionMap.put(beanName, beanDefinition); if (oldBeanDefinition != null || containsSingleton(beanName)) { resetBeanDefinition(beanName); @@ -872,7 +891,19 @@ public class DefaultListableBeanFactory extends AbstractAutowireCapableBeanFacto } throw new NoSuchBeanDefinitionException(beanName); } - this.beanDefinitionNames.remove(beanName); + + if (hasBeanCreationStarted()) { + // Cannot modify startup-time collection elements anymore (for stable iteration) + synchronized (this.beanDefinitionMap) { + List updatedDefinitions = new ArrayList(this.beanDefinitionNames); + updatedDefinitions.remove(beanName); + this.beanDefinitionNames = updatedDefinitions; + } + } + else { + // Still in startup registration phase + this.beanDefinitionNames.remove(beanName); + } this.frozenBeanDefinitionNames = null; resetBeanDefinition(beanName); @@ -914,9 +945,25 @@ public class DefaultListableBeanFactory extends AbstractAutowireCapableBeanFacto @Override public void registerSingleton(String beanName, Object singletonObject) throws IllegalStateException { super.registerSingleton(beanName, singletonObject); - if (!this.beanDefinitionMap.containsKey(beanName)) { - this.manualSingletonNames.add(beanName); + + if (hasBeanCreationStarted()) { + // Cannot modify startup-time collection elements anymore (for stable iteration) + synchronized (this.beanDefinitionMap) { + if (!this.beanDefinitionMap.containsKey(beanName)) { + Set updatedSingletons = new LinkedHashSet(this.manualSingletonNames.size() + 1); + updatedSingletons.addAll(this.manualSingletonNames); + updatedSingletons.add(beanName); + this.manualSingletonNames = updatedSingletons; + } + } } + else { + // Still in startup registration phase + if (!this.beanDefinitionMap.containsKey(beanName)) { + this.manualSingletonNames.add(beanName); + } + } + clearByTypeCache(); } diff --git a/spring-beans/src/test/java/org/springframework/beans/factory/DefaultListableBeanFactoryTests.java b/spring-beans/src/test/java/org/springframework/beans/factory/DefaultListableBeanFactoryTests.java index 4f8f7f26b7..9312130bb5 100644 --- a/spring-beans/src/test/java/org/springframework/beans/factory/DefaultListableBeanFactoryTests.java +++ b/spring-beans/src/test/java/org/springframework/beans/factory/DefaultListableBeanFactoryTests.java @@ -1224,7 +1224,7 @@ public class DefaultListableBeanFactoryTests { RootBeanDefinition rbd = new RootBeanDefinition(PropertiesFactoryBean.class); MutablePropertyValues pvs = new MutablePropertyValues(); - pvs.add("locations", new String[] {"#{foo}"}); + pvs.add("locations", new String[]{"#{foo}"}); rbd.setPropertyValues(pvs); bf.registerBeanDefinition("myProperties", rbd); Properties properties = (Properties) bf.getBean("myProperties"); @@ -2462,6 +2462,7 @@ public class DefaultListableBeanFactoryTests { public Object postProcessBeforeInitialization(Object bean, String beanName) { return new TestBean(); } + @Override public Object postProcessAfterInitialization(Object bean, String beanName) { return bean; @@ -2750,10 +2751,6 @@ public class DefaultListableBeanFactoryTests { verify(r3, never()).resolveStringValue(isNull(String.class)); } - - static class A { } - static class B { } - /** * Test that by-type bean lookup caching is working effectively by searching for a * bean of type B 10K times within a container having 1K additional beans of type A. @@ -2764,23 +2761,52 @@ public class DefaultListableBeanFactoryTests { * under the 1000 ms timeout, usually ~= 300ms. With caching removed and on the same * hardware the method will take ~13000 ms. See SPR-6870. */ - @Test(timeout=1000) + @Test(timeout = 1000) public void testByTypeLookupIsFastEnough() { Assume.group(TestGroup.PERFORMANCE); DefaultListableBeanFactory bf = new DefaultListableBeanFactory(); for (int i = 0; i < 1000; i++) { - bf.registerBeanDefinition("a"+i, new RootBeanDefinition(A.class)); + bf.registerBeanDefinition("a" + i, new RootBeanDefinition(A.class)); } bf.registerBeanDefinition("b", new RootBeanDefinition(B.class)); bf.freezeConfiguration(); - for (int i=0; i<10000; i++) { + for (int i = 0; i < 10000; i++) { bf.getBean(B.class); } } + @Test(timeout = 1000) + public void testRegistrationOfManyBeanDefinitionsIsFastEnough() { + // Assume.group(TestGroup.PERFORMANCE); + DefaultListableBeanFactory bf = new DefaultListableBeanFactory(); + bf.registerBeanDefinition("b", new RootBeanDefinition(B.class)); + // bf.getBean("b"); + + for (int i = 0; i < 100000; i++) { + bf.registerBeanDefinition("a" + i, new RootBeanDefinition(A.class)); + } + } + + @Test(timeout = 1000) + public void testRegistrationOfManySingletonsIsFastEnough() { + // Assume.group(TestGroup.PERFORMANCE); + DefaultListableBeanFactory bf = new DefaultListableBeanFactory(); + bf.registerBeanDefinition("b", new RootBeanDefinition(B.class)); + // bf.getBean("b"); + + for (int i = 0; i < 100000; i++) { + bf.registerSingleton("a" + i, new A()); + } + } + + + static class A { } + + static class B { } + public static class NoDependencies {