diff --git a/org.springframework.beans/src/main/java/org/springframework/beans/factory/config/ConfigurableBeanFactory.java b/org.springframework.beans/src/main/java/org/springframework/beans/factory/config/ConfigurableBeanFactory.java index 2be34e583b2..4bbc74ebdc6 100644 --- a/org.springframework.beans/src/main/java/org/springframework/beans/factory/config/ConfigurableBeanFactory.java +++ b/org.springframework.beans/src/main/java/org/springframework/beans/factory/config/ConfigurableBeanFactory.java @@ -320,6 +320,15 @@ public interface ConfigurableBeanFactory extends HierarchicalBeanFactory, Single */ boolean isFactoryBean(String name) throws NoSuchBeanDefinitionException; + /** + * Explicitly control in-creation status of the specified bean. For + * container internal use only. + * @param beanName the name of the bean + * @param inCreation whether the bean is currently in creation + * @since 3.1 + */ + void setCurrentlyInCreation(String beanName, boolean inCreation); + /** * Determine whether the specified bean is currently in creation. * @param beanName the name of the bean diff --git a/org.springframework.beans/src/main/java/org/springframework/beans/factory/support/DefaultSingletonBeanRegistry.java b/org.springframework.beans/src/main/java/org/springframework/beans/factory/support/DefaultSingletonBeanRegistry.java index 31899b6a048..9f7d47bc0ac 100644 --- a/org.springframework.beans/src/main/java/org/springframework/beans/factory/support/DefaultSingletonBeanRegistry.java +++ b/org.springframework.beans/src/main/java/org/springframework/beans/factory/support/DefaultSingletonBeanRegistry.java @@ -97,6 +97,9 @@ public class DefaultSingletonBeanRegistry extends SimpleAliasRegistry implements /** Names of beans that are currently in creation */ private final Set singletonsCurrentlyInCreation = Collections.synchronizedSet(new HashSet()); + /** Names of beans currently excluded from in creation checks */ + private final Set inCreationCheckExclusions = new HashSet(); + /** List of suppressed Exceptions, available for associating related causes */ private Set suppressedExceptions; @@ -293,7 +296,7 @@ public class DefaultSingletonBeanRegistry extends SimpleAliasRegistry implements * @see #isSingletonCurrentlyInCreation */ protected void beforeSingletonCreation(String beanName) { - if (!this.singletonsCurrentlyInCreation.add(beanName)) { + if (!this.inCreationCheckExclusions.contains(beanName) && !this.singletonsCurrentlyInCreation.add(beanName)) { throw new BeanCurrentlyInCreationException(beanName); } } @@ -305,11 +308,19 @@ public class DefaultSingletonBeanRegistry extends SimpleAliasRegistry implements * @see #isSingletonCurrentlyInCreation */ protected void afterSingletonCreation(String beanName) { - if (!this.singletonsCurrentlyInCreation.remove(beanName)) { + if (!this.inCreationCheckExclusions.contains(beanName) && !this.singletonsCurrentlyInCreation.remove(beanName)) { throw new IllegalStateException("Singleton '" + beanName + "' isn't currently in creation"); } } + public final void setCurrentlyInCreation(String beanName, boolean inCreation) { + if (!inCreation) { + this.inCreationCheckExclusions.add(beanName); + } else { + this.inCreationCheckExclusions.remove(beanName); + } + } + /** * Return whether the specified singleton bean is currently in creation * (within the entire factory). diff --git a/org.springframework.beans/src/main/java/org/springframework/beans/factory/support/SimpleInstantiationStrategy.java b/org.springframework.beans/src/main/java/org/springframework/beans/factory/support/SimpleInstantiationStrategy.java index ba0906219fb..4a81e970a80 100644 --- a/org.springframework.beans/src/main/java/org/springframework/beans/factory/support/SimpleInstantiationStrategy.java +++ b/org.springframework.beans/src/main/java/org/springframework/beans/factory/support/SimpleInstantiationStrategy.java @@ -42,6 +42,8 @@ import org.springframework.util.StringUtils; */ public class SimpleInstantiationStrategy implements InstantiationStrategy { + private static final ThreadLocal currentlyInvokedFactoryMethod = new ThreadLocal(); + public Object instantiate(RootBeanDefinition beanDefinition, String beanName, BeanFactory owner) { // Don't override the class with CGLIB if no overrides. if (beanDefinition.getMethodOverrides().isEmpty()) { @@ -140,9 +142,19 @@ public class SimpleInstantiationStrategy implements InstantiationStrategy { else { ReflectionUtils.makeAccessible(factoryMethod); } - - // It's a static method if the target is null. - return factoryMethod.invoke(factoryBean, args); + + Method priorInvokedFactoryMethod = currentlyInvokedFactoryMethod.get(); + try { + currentlyInvokedFactoryMethod.set(factoryMethod); + return factoryMethod.invoke(factoryBean, args); + } finally { + if (priorInvokedFactoryMethod != null) { + currentlyInvokedFactoryMethod.set(priorInvokedFactoryMethod); + } + else { + currentlyInvokedFactoryMethod.remove(); + } + } } catch (IllegalArgumentException ex) { throw new BeanDefinitionStoreException( @@ -159,4 +171,12 @@ public class SimpleInstantiationStrategy implements InstantiationStrategy { } } + /** + * Return the factory method currently being invoked or {@code null} if none. + * Allows factory method implementations to determine whether the current + * caller is the container itself as opposed to user code. + */ + public static Method getCurrentlyInvokedFactoryMethod() { + return currentlyInvokedFactoryMethod.get(); + } } diff --git a/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassEnhancer.java b/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassEnhancer.java index 23a6baff8ee..44969659e3f 100644 --- a/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassEnhancer.java +++ b/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassEnhancer.java @@ -35,6 +35,7 @@ import org.springframework.beans.factory.DisposableBean; import org.springframework.beans.factory.FactoryBean; import org.springframework.beans.factory.config.BeanFactoryPostProcessor; import org.springframework.beans.factory.config.ConfigurableBeanFactory; +import org.springframework.beans.factory.support.SimpleInstantiationStrategy; import org.springframework.core.annotation.AnnotationUtils; import org.springframework.util.Assert; @@ -233,23 +234,42 @@ class ConfigurationClassEnhancer { return enhanceFactoryBean(factoryBean.getClass(), beanName); } } - - // the bean is not a FactoryBean - check to see if it has been cached - if (factoryContainsBean(beanName)) { - // we have an already existing cached instance of this bean -> retrieve it - return this.beanFactory.getBean(beanName); + + boolean factoryIsCaller = beanMethod.equals(SimpleInstantiationStrategy.getCurrentlyInvokedFactoryMethod()); + boolean factoryAlreadyContainsSingleton = this.beanFactory.containsSingleton(beanName); + if (factoryIsCaller && !factoryAlreadyContainsSingleton) { + // the factory is calling the bean method in order to instantiate and register the bean + // (i.e. via a getBean() call) -> invoke the super implementation of the method to actually + // create the bean instance. + if (BeanFactoryPostProcessor.class.isAssignableFrom(beanMethod.getReturnType())) { + logger.warn(String.format("@Bean method %s.%s is non-static and returns an object " + + "assignable to Spring's BeanFactoryPostProcessor interface. This will " + + "result in a failure to process annotations such as @Autowired, " + + "@Resource and @PostConstruct within the method's declaring " + + "@Configuration class. Add the 'static' modifier to this method to avoid " + + "these container lifecycle issues; see @Bean Javadoc for complete details", + beanMethod.getDeclaringClass().getSimpleName(), beanMethod.getName())); + } + return cglibMethodProxy.invokeSuper(enhancedConfigInstance, beanMethodArgs); + } + else { + // the user (i.e. not the factory) is requesting this bean through a + // call to the bean method, direct or indirect. The bean may have already been + // marked as 'in creation' in certain autowiring scenarios; if so, temporarily + // set the in-creation status to false in order to avoid an exception. + boolean alreadyInCreation = this.beanFactory.isCurrentlyInCreation(beanName); + try { + if (alreadyInCreation) { + this.beanFactory.setCurrentlyInCreation(beanName, false); + } + return this.beanFactory.getBean(beanName); + } finally { + if (alreadyInCreation) { + this.beanFactory.setCurrentlyInCreation(beanName, true); + } + } } - if (BeanFactoryPostProcessor.class.isAssignableFrom(beanMethod.getReturnType())) { - logger.warn(String.format("@Bean method %s.%s is non-static and returns an object " + - "assignable to Spring's BeanFactoryPostProcessor interface. This will " + - "result in a failure to process annotations such as @Autowired, " + - "@Resource and @PostConstruct within the method's declaring " + - "@Configuration class. Add the 'static' modifier to this method to avoid" + - "these container lifecycle issues; see @Bean Javadoc for complete details", - beanMethod.getDeclaringClass().getSimpleName(), beanMethod.getName())); - } - return cglibMethodProxy.invokeSuper(enhancedConfigInstance, beanMethodArgs); } /** @@ -266,7 +286,9 @@ class ConfigurationClassEnhancer { * @return whether beanName already exists in the factory */ private boolean factoryContainsBean(String beanName) { - return (this.beanFactory.containsBean(beanName) && !this.beanFactory.isCurrentlyInCreation(beanName)); + boolean containsBean = this.beanFactory.containsBean(beanName); + boolean currentlyInCreation = this.beanFactory.isCurrentlyInCreation(beanName); + return (containsBean && !currentlyInCreation); } /** diff --git a/org.springframework.context/src/test/java/org/springframework/context/annotation/ConfigurationClassPostConstructAndAutowiringTests.java b/org.springframework.context/src/test/java/org/springframework/context/annotation/ConfigurationClassPostConstructAndAutowiringTests.java new file mode 100644 index 00000000000..eb288746fd2 --- /dev/null +++ b/org.springframework.context/src/test/java/org/springframework/context/annotation/ConfigurationClassPostConstructAndAutowiringTests.java @@ -0,0 +1,112 @@ +/* + * Copyright 2002-2011 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.context.annotation; + +import static org.hamcrest.CoreMatchers.is; +import static org.junit.Assert.assertThat; + +import javax.annotation.PostConstruct; + +import org.junit.Test; +import org.springframework.beans.TestBean; +import org.springframework.beans.factory.annotation.Autowired; + +/** + * Tests cornering the issue reported in SPR-8080. If the product of a @Bean method + * was @Autowired into a configuration class while at the same time the declaring + * configuration class for the @Bean method in question has a @PostConstruct + * (or other initializer) method, the container would become confused about the + * 'currently in creation' status of the autowired bean and result in creating multiple + * instances of the given @Bean, violating container scoping / singleton semantics. + * + * This is resolved through no longer relying on 'currently in creation' status, but + * rather on a thread local that informs the enhanced bean method implementation whether + * the factory is the caller or not. + * + * @author Chris Beams + * @since 3.1 + */ +public class ConfigurationClassPostConstructAndAutowiringTests { + + /** + * Prior to the fix for SPR-8080, this method would succeed due to ordering of + * configuration class registration. + */ + @Test + public void control() { + AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(); + ctx.register(Config1.class, Config2.class); + ctx.refresh(); + + assertions(ctx); + + Config2 config2 = ctx.getBean(Config2.class); + assertThat(config2.testBean, is(ctx.getBean(TestBean.class))); + } + + /** + * Prior to the fix for SPR-8080, this method would fail due to ordering of + * configuration class registration. + */ + @Test + public void originalReproCase() { + AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(); + ctx.register(Config2.class, Config1.class); + ctx.refresh(); + + assertions(ctx); + } + + private void assertions(AnnotationConfigApplicationContext ctx) { + Config1 config1 = ctx.getBean(Config1.class); + TestBean testBean = ctx.getBean(TestBean.class); + assertThat(config1.beanMethodCallCount, is(1)); + assertThat(testBean.getAge(), is(2)); + } + + + @Configuration + static class Config1 { + + int beanMethodCallCount = 0; + + @PostConstruct + public void init() { + beanMethod().setAge(beanMethod().getAge() + 1); // age == 2 + } + + @Bean + public TestBean beanMethod() { + beanMethodCallCount++; + TestBean testBean = new TestBean(); + testBean.setAge(1); + return testBean; + } + } + + + @Configuration + static class Config2 { + TestBean testBean; + + @Autowired + void setTestBean(TestBean testBean) { + this.testBean = testBean; + } + } + +}