Fix @Autowired+@PostConstruct+@Configuration issue

A subtle issue existed with the way we relied on isCurrentlyInCreation
to determine whether a @Bean method is being called by the container
or by user code.  This worked in most cases, but in the particular
scenario laid out by SPR-8080, this approach was no longer sufficient.

This change introduces a ThreadLocal that contains the factory method
currently being invoked by the container, such that enhanced @Bean
methods can check against it to see if they are being called by the
container or not.  If so, that is the cue that the user-defined @Bean
method implementation should be invoked in order to actually create
the bean for the first time.  If not, then the cached instance of
the already-created bean should be looked up and returned.

See ConfigurationClassPostConstructAndAutowiringTests for
reproduction cases and more detail.

Issue: SPR-8080

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

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<String> singletonsCurrentlyInCreation = Collections.synchronizedSet(new HashSet<String>());
 
+	/** Names of beans currently excluded from in creation checks */
+	private final Set<String> inCreationCheckExclusions = new HashSet<String>();
+
 	/** List of suppressed Exceptions, available for associating related causes */
 	private Set<Exception> 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).

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<Method> currentlyInvokedFactoryMethod = new ThreadLocal<Method>();
+
 	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.
+			Method priorInvokedFactoryMethod = currentlyInvokedFactoryMethod.get();
-			return factoryMethod.invoke(factoryBean, args);
+			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();
+	}
 }

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
+			boolean factoryIsCaller = beanMethod.equals(SimpleInstantiationStrategy.getCurrentlyInvokedFactoryMethod());
-			if (factoryContainsBean(beanName)) {
+			boolean factoryAlreadyContainsSingleton = this.beanFactory.containsSingleton(beanName);
-				// we have an already existing cached instance of this bean -> retrieve it
+			if (factoryIsCaller && !factoryAlreadyContainsSingleton) {
-				return this.beanFactory.getBean(beanName);
+				// 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 <var>beanName</var> 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);
 		}
 
 		/**

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;
+		}
+	}
+
+}