Avoid Scope.registerDestructionCallback for inner beans in case of scope mismatch

This commit refines getMergedBeanDefinition's scope adaptation algorithm for inner beans, never leaving a custom scope within a containing bean of a different scope. The inner bean's scope will either be aligned with the containing bean's scope (matching the effective state) or be switched to prototype in case of an outer singleton (indicating that no singleton state management is desired).

Issue: SPR-13739

Author: jhoeller

spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractBeanFactory.java

@@ -1062,7 +1062,7 @@ public void destroyScopedBean(String beanName) {
 		String scopeName = mbd.getScope();
 		Scope scope = this.scopes.get(scopeName);
 		if (scope == null) {
-			throw new IllegalStateException("No Scope SPI registered for scope '" + scopeName + "'");
+			throw new IllegalStateException("No Scope SPI registered for scope name '" + scopeName + "'");
 		}
 		Object bean = scope.remove(beanName);
 		if (bean != null) {
@@ -1251,15 +1251,14 @@ protected RootBeanDefinition getMergedBeanDefinition(
 
 				// Set default singleton scope, if not configured before.
 				if (!StringUtils.hasLength(mbd.getScope())) {
-					mbd.setScope(RootBeanDefinition.SCOPE_SINGLETON);
+					mbd.setScope(BeanDefinition.SCOPE_SINGLETON);
 				}
 
-				// A bean contained in a non-singleton bean cannot be a singleton itself.
-				// Let's correct this on the fly here, since this might be the result of
-				// parent-child merging for the outer bean, in which case the original inner bean
-				// definition will not have inherited the merged outer bean's singleton status.
-				if (containingBd != null && !containingBd.isSingleton() && mbd.isSingleton()) {
-					mbd.setScope(containingBd.getScope());
+				// Check for a mismatch between an inner bean's scope and its containing
+				// bean's scope: For example, a bean contained in a non-singleton bean
+				// cannot be a singleton itself. Let's correct this on the fly here.
+				if (containingBd != null && !mbd.isPrototype() && !mbd.getScope().equals(containingBd.getScope())) {
+					mbd.setScope(containingBd.isSingleton() ? BeanDefinition.SCOPE_PROTOTYPE : containingBd.getScope());
 				}
 
 				// Only cache the merged bean definition if we're already about to create an
@@ -1638,7 +1637,7 @@ protected void registerDisposableBeanIfNecessary(String beanName, Object bean, R
 				// A bean with a custom scope...
 				Scope scope = this.scopes.get(mbd.getScope());
 				if (scope == null) {
-					throw new IllegalStateException("No Scope registered for scope '" + mbd.getScope() + "'");
+					throw new IllegalStateException("No Scope registered for scope name '" + mbd.getScope() + "'");
 				}
 				scope.registerDestructionCallback(beanName,
 						new DisposableBeanAdapter(bean, beanName, mbd, getBeanPostProcessors(), acc));

spring-beans/src/test/java/org/springframework/beans/factory/DefaultListableBeanFactoryTests.java

@@ -2511,10 +2511,74 @@ public Object postProcessAfterInitialization(Object bean, String beanName) {
 				return bean;
 			}
 		});
-		BeanWithDestroyMethod.closed = false;
+		BeanWithDestroyMethod.closeCount = 0;
 		lbf.preInstantiateSingletons();
 		lbf.destroySingletons();
-		assertTrue("Destroy method invoked", BeanWithDestroyMethod.closed);
+		assertEquals("Destroy methods invoked", 1, BeanWithDestroyMethod.closeCount);
+	}
+
+	@Test
+	public void testDestroyMethodOnInnerBean() {
+		DefaultListableBeanFactory lbf = new DefaultListableBeanFactory();
+		RootBeanDefinition innerBd = new RootBeanDefinition(BeanWithDestroyMethod.class);
+		innerBd.setDestroyMethodName("close");
+		RootBeanDefinition bd = new RootBeanDefinition(BeanWithDestroyMethod.class);
+		bd.setDestroyMethodName("close");
+		bd.getPropertyValues().add("inner", innerBd);
+		lbf.registerBeanDefinition("test", bd);
+		BeanWithDestroyMethod.closeCount = 0;
+		lbf.preInstantiateSingletons();
+		lbf.destroySingletons();
+		assertEquals("Destroy methods invoked", 2, BeanWithDestroyMethod.closeCount);
+	}
+
+	@Test
+	public void testDestroyMethodOnInnerBeanAsPrototype() {
+		DefaultListableBeanFactory lbf = new DefaultListableBeanFactory();
+		RootBeanDefinition innerBd = new RootBeanDefinition(BeanWithDestroyMethod.class);
+		innerBd.setScope(RootBeanDefinition.SCOPE_PROTOTYPE);
+		innerBd.setDestroyMethodName("close");
+		RootBeanDefinition bd = new RootBeanDefinition(BeanWithDestroyMethod.class);
+		bd.setDestroyMethodName("close");
+		bd.getPropertyValues().add("inner", innerBd);
+		lbf.registerBeanDefinition("test", bd);
+		BeanWithDestroyMethod.closeCount = 0;
+		lbf.preInstantiateSingletons();
+		lbf.destroySingletons();
+		assertEquals("Destroy methods invoked", 1, BeanWithDestroyMethod.closeCount);
+	}
+
+	@Test
+	public void testDestroyMethodOnInnerBeanAsCustomScope() {
+		DefaultListableBeanFactory lbf = new DefaultListableBeanFactory();
+		RootBeanDefinition innerBd = new RootBeanDefinition(BeanWithDestroyMethod.class);
+		innerBd.setScope("custom");
+		innerBd.setDestroyMethodName("close");
+		RootBeanDefinition bd = new RootBeanDefinition(BeanWithDestroyMethod.class);
+		bd.setDestroyMethodName("close");
+		bd.getPropertyValues().add("inner", innerBd);
+		lbf.registerBeanDefinition("test", bd);
+		BeanWithDestroyMethod.closeCount = 0;
+		lbf.preInstantiateSingletons();
+		lbf.destroySingletons();
+		assertEquals("Destroy methods not invoked", 1, BeanWithDestroyMethod.closeCount);
+	}
+
+	@Test
+	public void testDestroyMethodOnInnerBeanAsCustomScopeWithinPrototype() {
+		DefaultListableBeanFactory lbf = new DefaultListableBeanFactory();
+		RootBeanDefinition innerBd = new RootBeanDefinition(BeanWithDestroyMethod.class);
+		innerBd.setScope("custom");
+		innerBd.setDestroyMethodName("close");
+		RootBeanDefinition bd = new RootBeanDefinition(BeanWithDestroyMethod.class);
+		bd.setScope(RootBeanDefinition.SCOPE_PROTOTYPE);
+		bd.setDestroyMethodName("close");
+		bd.getPropertyValues().add("inner", innerBd);
+		lbf.registerBeanDefinition("test", bd);
+		BeanWithDestroyMethod.closeCount = 0;
+		Object prototypeInstance = lbf.getBean("test");
+		lbf.destroyBean("test", prototypeInstance);
+		assertEquals("Destroy methods not invoked", 1, BeanWithDestroyMethod.closeCount);
 	}
 
 	@Test
@@ -2780,7 +2844,7 @@ public void testByTypeLookupIsFastEnough() {
 
 	@Test(timeout = 1000)
 	public void testRegistrationOfManyBeanDefinitionsIsFastEnough() {
-		// Assume.group(TestGroup.PERFORMANCE);
+		Assume.group(TestGroup.PERFORMANCE);
 		DefaultListableBeanFactory bf = new DefaultListableBeanFactory();
 		bf.registerBeanDefinition("b", new RootBeanDefinition(B.class));
 		// bf.getBean("b");
@@ -2792,7 +2856,7 @@ public void testRegistrationOfManyBeanDefinitionsIsFastEnough() {
 
 	@Test(timeout = 1000)
 	public void testRegistrationOfManySingletonsIsFastEnough() {
-		// Assume.group(TestGroup.PERFORMANCE);
+		Assume.group(TestGroup.PERFORMANCE);
 		DefaultListableBeanFactory bf = new DefaultListableBeanFactory();
 		bf.registerBeanDefinition("b", new RootBeanDefinition(B.class));
 		// bf.getBean("b");
@@ -2913,10 +2977,16 @@ public void close() {
 
 	public static class BeanWithDestroyMethod {
 
-		private static boolean closed;
+		private static int closeCount = 0;
+
+		private BeanWithDestroyMethod inner;
+
+		public void setInner(BeanWithDestroyMethod inner) {
+			this.inner = inner;
+		}
 
 		public void close() {
-			closed = true;
+			closeCount++;
 		}
 	}