diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/support/DefaultSingletonBeanRegistry.java b/spring-beans/src/main/java/org/springframework/beans/factory/support/DefaultSingletonBeanRegistry.java index eea12e5ab0..00867c7d41 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/support/DefaultSingletonBeanRegistry.java +++ b/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 singletonsInLenientCreation = new HashSet<>(); - /** Map from bean name to actual creation thread for leniently created beans. */ - private final Map lenientCreationThreads = new ConcurrentHashMap<>(); + /** Map from one creation thread waiting on a lenient creation thread. */ + private final Map lenientWaitingThreads = new HashMap<>(); + + /** Map from bean name to actual creation thread for currently created beans. */ + private final Map 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(); + } } } } diff --git a/spring-context/src/test/java/org/springframework/context/annotation/BackgroundBootstrapTests.java b/spring-context/src/test/java/org/springframework/context/annotation/BackgroundBootstrapTests.java index bd2071f960..3d7662ec44 100644 --- a/spring-context/src/test/java/org/springframework/context/annotation/BackgroundBootstrapTests.java +++ b/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 testBean2) { @@ -229,6 +248,46 @@ class BackgroundBootstrapTests { } + @Configuration(proxyBeanMethods = false) + static class CircularReferenceWithBlockingMainThreadBeanConfig { + + @Bean + public TestBean testBean1(ObjectProvider 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 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 testBean2, ObjectProvider 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 testBean3) { + try { + Thread.sleep(1000); + } + catch (InterruptedException ex) { + throw new RuntimeException(ex); + } + return new TestBean(testBean3.getObject()); + } + + @Bean + public TestBean testBean3(ObjectProvider testBean2) { + try { + Thread.sleep(1000); + } + catch (InterruptedException ex) { + throw new RuntimeException(ex); + } + return new TestBean(testBean2.getObject()); + } + } + + @Configuration(proxyBeanMethods = false) static class CustomExecutorBeanConfig {