Enforce circular reference exception between all thread variations

Closes gh-34672

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

@@ -17,6 +17,7 @@
 package org.springframework.beans.factory.support;
 
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
@@ -110,8 +111,11 @@ public class DefaultSingletonBeanRegistry extends SimpleAliasRegistry implements
 	/** Names of beans that are currently in lenient creation. */
 	private final Set<String> singletonsInLenientCreation = new HashSet<>();
 
-	/** Map from bean name to actual creation thread for leniently created beans. */
-	private final Map<String, Thread> lenientCreationThreads = new ConcurrentHashMap<>();
+	/** Map from one creation thread waiting on a lenient creation thread. */
+	private final Map<Thread, Thread> lenientWaitingThreads = new HashMap<>();
+
+	/** Map from bean name to actual creation thread for currently created beans. */
+	private final Map<String, Thread> currentCreationThreads = new ConcurrentHashMap<>();
 
 	/** Flag that indicates whether we're currently within destroySingletons. */
 	private volatile boolean singletonsCurrentlyInDestruction = false;
@@ -253,9 +257,11 @@ public class DefaultSingletonBeanRegistry extends SimpleAliasRegistry implements
 	public Object getSingleton(String beanName, ObjectFactory<?> singletonFactory) {
 		Assert.notNull(beanName, "Bean name must not be null");
 
+		Thread currentThread = Thread.currentThread();
 		Boolean lockFlag = isCurrentThreadAllowedToHoldSingletonLock();
 		boolean acquireLock = !Boolean.FALSE.equals(lockFlag);
 		boolean locked = (acquireLock && this.singletonLock.tryLock());
+
 		try {
 			Object singletonObject = this.singletonObjects.get(beanName);
 			if (singletonObject == null) {
@@ -307,17 +313,27 @@ public class DefaultSingletonBeanRegistry extends SimpleAliasRegistry implements
 					this.lenientCreationLock.lock();
 					try {
 						while ((singletonObject = this.singletonObjects.get(beanName)) == null) {
+							Thread otherThread = this.currentCreationThreads.get(beanName);
+							if (otherThread != null && (otherThread == currentThread ||
+									this.lenientWaitingThreads.get(otherThread) == currentThread)) {
+								throw ex;
+							}
 							if (!this.singletonsInLenientCreation.contains(beanName)) {
 								break;
 							}
-							if (this.lenientCreationThreads.get(beanName) == Thread.currentThread()) {
-								throw ex;
+							if (otherThread != null) {
+								this.lenientWaitingThreads.put(currentThread, otherThread);
 							}
 							try {
 								this.lenientCreationFinished.await();
 							}
 							catch (InterruptedException ie) {
-								Thread.currentThread().interrupt();
+								currentThread.interrupt();
+							}
+							finally {
+								if (otherThread != null) {
+									this.lenientWaitingThreads.remove(currentThread);
+								}
 							}
 						}
 					}
@@ -350,17 +366,12 @@ public class DefaultSingletonBeanRegistry extends SimpleAliasRegistry implements
 					// Leniently created singleton object could have appeared in the meantime.
 					singletonObject = this.singletonObjects.get(beanName);
 					if (singletonObject == null) {
-						if (locked) {
+						this.currentCreationThreads.put(beanName, currentThread);
+						try {
 							singletonObject = singletonFactory.getObject();
 						}
-						else {
-							this.lenientCreationThreads.put(beanName, Thread.currentThread());
-							try {
-								singletonObject = singletonFactory.getObject();
-							}
-							finally {
-								this.lenientCreationThreads.remove(beanName);
-							}
+						finally {
+							this.currentCreationThreads.remove(beanName);
 						}
 						newSingleton = true;
 					}
@@ -410,6 +421,8 @@ public class DefaultSingletonBeanRegistry extends SimpleAliasRegistry implements
 			this.lenientCreationLock.lock();
 			try {
 				this.singletonsInLenientCreation.remove(beanName);
+				this.lenientWaitingThreads.entrySet().removeIf(
+						entry -> entry.getValue() == currentThread);
 				this.lenientCreationFinished.signalAll();
 			}
 			finally {
@@ -724,12 +737,19 @@ public class DefaultSingletonBeanRegistry extends SimpleAliasRegistry implements
 			// For an individual destruction, remove the registered instance now.
 			// As of 6.2, this happens after the current bean's destruction step,
 			// allowing for late bean retrieval by on-demand suppliers etc.
-			this.singletonLock.lock();
-			try {
+			if (this.currentCreationThreads.get(beanName) == Thread.currentThread()) {
+				// Local remove after failed creation step -> without singleton lock
+				// since bean creation may have happened leniently without any lock.
 				removeSingleton(beanName);
 			}
-			finally {
-				this.singletonLock.unlock();
+			else {
+				this.singletonLock.lock();
+				try {
+					removeSingleton(beanName);
+				}
+				finally {
+					this.singletonLock.unlock();
+				}
 			}
 		}
 	}

spring-context/src/test/java/org/springframework/context/annotation/BackgroundBootstrapTests.java

@@ -19,6 +19,7 @@ package org.springframework.context.annotation;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.Timeout;
 
+import org.springframework.beans.factory.BeanCreationException;
 import org.springframework.beans.factory.BeanCurrentlyInCreationException;
 import org.springframework.beans.factory.ObjectProvider;
 import org.springframework.beans.factory.UnsatisfiedDependencyException;
@@ -42,7 +43,7 @@ import static org.springframework.core.testfixture.TestGroup.LONG_RUNNING;
 class BackgroundBootstrapTests {
 
 	@Test
-	@Timeout(5)
+	@Timeout(10)
 	@EnabledForTestGroups(LONG_RUNNING)
 	void bootstrapWithUnmanagedThread() {
 		ConfigurableApplicationContext ctx = new AnnotationConfigApplicationContext(UnmanagedThreadBeanConfig.class);
@@ -52,7 +53,7 @@ class BackgroundBootstrapTests {
 	}
 
 	@Test
-	@Timeout(5)
+	@Timeout(10)
 	@EnabledForTestGroups(LONG_RUNNING)
 	void bootstrapWithUnmanagedThreads() {
 		ConfigurableApplicationContext ctx = new AnnotationConfigApplicationContext(UnmanagedThreadsBeanConfig.class);
@@ -64,7 +65,7 @@ class BackgroundBootstrapTests {
 	}
 
 	@Test
-	@Timeout(5)
+	@Timeout(10)
 	@EnabledForTestGroups(LONG_RUNNING)
 	void bootstrapWithStrictLockingThread() {
 		SpringProperties.setFlag(DefaultListableBeanFactory.STRICT_LOCKING_PROPERTY_NAME);
@@ -79,17 +80,26 @@ class BackgroundBootstrapTests {
 	}
 
 	@Test
-	@Timeout(5)
+	@Timeout(10)
 	@EnabledForTestGroups(LONG_RUNNING)
-	void bootstrapWithCircularReference() {
-		ConfigurableApplicationContext ctx = new AnnotationConfigApplicationContext(CircularReferenceBeanConfig.class);
+	void bootstrapWithCircularReferenceAgainstMainThread() {
+		ConfigurableApplicationContext ctx = new AnnotationConfigApplicationContext(CircularReferenceAgainstMainThreadBeanConfig.class);
 		ctx.getBean("testBean1", TestBean.class);
 		ctx.getBean("testBean2", TestBean.class);
 		ctx.close();
 	}
 
 	@Test
-	@Timeout(5)
+	@Timeout(10)
+	@EnabledForTestGroups(LONG_RUNNING)
+	void bootstrapWithCircularReferenceWithBlockingMainThread() {
+		assertThatExceptionOfType(BeanCreationException.class)
+				.isThrownBy(() -> new AnnotationConfigApplicationContext(CircularReferenceWithBlockingMainThreadBeanConfig.class))
+				.withRootCauseInstanceOf(BeanCurrentlyInCreationException.class);
+	}
+
+	@Test
+	@Timeout(10)
 	@EnabledForTestGroups(LONG_RUNNING)
 	void bootstrapWithCircularReferenceInSameThread() {
 		assertThatExceptionOfType(UnsatisfiedDependencyException.class)
@@ -98,7 +108,16 @@ class BackgroundBootstrapTests {
 	}
 
 	@Test
-	@Timeout(5)
+	@Timeout(10)
+	@EnabledForTestGroups(LONG_RUNNING)
+	void bootstrapWithCircularReferenceInMultipleThreads() {
+		assertThatExceptionOfType(BeanCreationException.class)
+				.isThrownBy(() -> new AnnotationConfigApplicationContext(CircularReferenceInMultipleThreadsBeanConfig.class))
+				.withRootCauseInstanceOf(BeanCurrentlyInCreationException.class);
+	}
+
+	@Test
+	@Timeout(10)
 	@EnabledForTestGroups(LONG_RUNNING)
 	void bootstrapWithCustomExecutor() {
 		ConfigurableApplicationContext ctx = new AnnotationConfigApplicationContext(CustomExecutorBeanConfig.class);
@@ -202,7 +221,7 @@ class BackgroundBootstrapTests {
 
 
 	@Configuration(proxyBeanMethods = false)
-	static class CircularReferenceBeanConfig {
+	static class CircularReferenceAgainstMainThreadBeanConfig {
 
 		@Bean
 		public TestBean testBean1(ObjectProvider<TestBean> testBean2) {
@@ -229,6 +248,46 @@ class BackgroundBootstrapTests {
 	}
 
 
+	@Configuration(proxyBeanMethods = false)
+	static class CircularReferenceWithBlockingMainThreadBeanConfig {
+
+		@Bean
+		public TestBean testBean1(ObjectProvider<TestBean> testBean2) {
+			Thread thread = new Thread(testBean2::getObject);
+			thread.setUncaughtExceptionHandler((t, ex) -> System.out.println(System.currentTimeMillis() + " " + ex + " " + t));
+			thread.start();
+			try {
+				Thread.sleep(1000);
+			}
+			catch (InterruptedException ex) {
+				throw new RuntimeException(ex);
+			}
+			return new TestBean(testBean2.getObject());
+		}
+
+		@Bean
+		public TestBean testBean2(ObjectProvider<TestBean> testBean1) {
+			System.out.println(System.currentTimeMillis() + " testBean2 begin " + Thread.currentThread());
+			try {
+				Thread.sleep(2000);
+			}
+			catch (InterruptedException ex) {
+				throw new RuntimeException(ex);
+			}
+			try {
+				return new TestBean(testBean1.getObject());
+			}
+			catch (RuntimeException ex) {
+				System.out.println(System.currentTimeMillis() + " testBean2 exception " + Thread.currentThread());
+				throw ex;
+			}
+			finally {
+				System.out.println(System.currentTimeMillis() + " testBean2 end " + Thread.currentThread());
+			}
+		}
+	}
+
+
 	@Configuration(proxyBeanMethods = false)
 	static class CircularReferenceInSameThreadBeanConfig {
 
@@ -262,6 +321,46 @@ class BackgroundBootstrapTests {
 	}
 
 
+	@Configuration(proxyBeanMethods = false)
+	static class CircularReferenceInMultipleThreadsBeanConfig {
+
+		@Bean
+		public TestBean testBean1(ObjectProvider<TestBean> testBean2, ObjectProvider<TestBean> testBean3) {
+			new Thread(testBean2::getObject).start();
+			new Thread(testBean3::getObject).start();
+			try {
+				Thread.sleep(2000);
+			}
+			catch (InterruptedException ex) {
+				throw new RuntimeException(ex);
+			}
+			return new TestBean();
+		}
+
+		@Bean
+		public TestBean testBean2(ObjectProvider<TestBean> testBean3) {
+			try {
+				Thread.sleep(1000);
+			}
+			catch (InterruptedException ex) {
+				throw new RuntimeException(ex);
+			}
+			return new TestBean(testBean3.getObject());
+		}
+
+		@Bean
+		public TestBean testBean3(ObjectProvider<TestBean> testBean2) {
+			try {
+				Thread.sleep(1000);
+			}
+			catch (InterruptedException ex) {
+				throw new RuntimeException(ex);
+			}
+			return new TestBean(testBean2.getObject());
+		}
+	}
+
+
 	@Configuration(proxyBeanMethods = false)
 	static class CustomExecutorBeanConfig {