Avoid getting objects that have not completed initialization

[lijinxiong]

in org.springframework.beans.factory.support.DefaultSingletonBeanRegistry

@Nullable
protected Object getSingleton(String beanName, boolean allowEarlyReference) {
// Quick check for existing instance without full singleton lock
Object singletonObject = this.singletonObjects.get(beanName);
if (singletonObject == null && isSingletonCurrentlyInCreation(beanName)) {
	singletonObject = this.earlySingletonObjects.get(beanName);
	if (singletonObject == null && allowEarlyReference) {
		synchronized (this.singletonObjects) {
			// Consistent creation of early reference within full singleton lock
			singletonObject = this.singletonObjects.get(beanName);
			if (singletonObject == null) {
				singletonObject = this.earlySingletonObjects.get(beanName);
				if (singletonObject == null) {
					ObjectFactory<?> singletonFactory = this.singletonFactories.get(beanName);
					if (singletonFactory != null) {
						singletonObject = singletonFactory.getObject();
						this.earlySingletonObjects.put(beanName, singletonObject);
						this.singletonFactories.remove(beanName);
					}
				}
			}
		}
	}
}
return singletonObject;
}

Unit test

public class Father {

	private Son son;

	public Son getSon() {
		return son;
	}

	public void setSon(Son son) {
		this.son = son;
	}
}

public class Son {

	private Father father;

	private Mother mother;

	public Father getFather() {
		return father;
	}

	public void setFather(Father father) {
		this.father = father;
	}

	public Mother getMother() {
		return mother;
	}

	public void setMother(Mother mother) {
		this.mother = mother;
	}
}

public class Mother {

	public Mother() {

		// Simulation takes a long time、just for testing
		try {
			Thread.sleep(4 * 1000);
		} catch (InterruptedException e) {
			e.printStackTrace();
		}

	}
}

	@Test
	public void testGetLazySingleton() throws InterruptedException {
		testGetSingleton("father");
	}

	@Test
	public void testGetNonLazySingleton() throws InterruptedException {
		testGetSingleton("father-non-lazy");
	}

	private void testGetSingleton(String fatherBeanName) throws InterruptedException {

		CountDownLatch countDownLatch = new CountDownLatch(2);
		AtomicInteger occurExceptionCount = new AtomicInteger(0);
		DefaultListableBeanFactory beanFactory = new DefaultListableBeanFactory();
		XmlBeanDefinitionReader reader = new XmlBeanDefinitionReader(beanFactory);
		reader.loadBeanDefinitions(new ClassPathResource("DefaultSingletonBeanRegistryTests.xml", getClass()));

		// Just for testing, you should use a thread pool
		new Thread(() -> {
			Father father = beanFactory.getBean(fatherBeanName, Father.class);
			try {
				Assert.notNull(father, "father Shouldn't be null");
				Assert.notNull(father.getSon(), "son Shouldn't be null");
			} catch (Exception e) {
				occurExceptionCount.incrementAndGet();
				System.out.println(Thread.currentThread().getName());
				e.printStackTrace();
			}
			countDownLatch.countDown();
		}, "first-thread").start();

		Thread.sleep(1000);

		// Just for testing, you should use a thread pool
		new Thread(() -> {
			Father father = beanFactory.getBean(fatherBeanName, Father.class);
			try {
				Assert.notNull(father, "father Shouldn't be null");
				Assert.notNull(father.getSon(), "son Shouldn't be null");
			} catch (Exception e) {
				occurExceptionCount.incrementAndGet();
				System.out.println(Thread.currentThread().getName());
				e.printStackTrace();
			}
			countDownLatch.countDown();
		}, "second-thread").start();

		while (countDownLatch.getCount() != 0) {

		}
		Assert.isTrue(occurExceptionCount.get() == 0, "Throwing an exception in the sub-thread that gets the bean");
	}

<?xml version="1.0" encoding="UTF-8"?>
<beans xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
	   xmlns="http://www.springframework.org/schema/beans"
	   xsi:schemaLocation="http://www.springframework.org/schema/beans https://www.springframework.org/schema/beans/spring-beans-4.1.xsd">
	<bean id="mother" class="org.springframework.beans.testfixture.beans.Mother" lazy-init="true">
	</bean>
	<bean id="father" class="org.springframework.beans.testfixture.beans.Father" lazy-init="true">
		<property name="son" ref="son"/>
	</bean>
	<bean id="son" class="org.springframework.beans.testfixture.beans.Son" lazy-init="true">
		<property name="father" ref="father"/>
		<property name="mother" ref="mother"/>
	</bean>
  
	<bean id="mother-non-lazy" class="org.springframework.beans.testfixture.beans.Mother" lazy-init="false">
	</bean>
	<bean id="father-non-lazy" class="org.springframework.beans.testfixture.beans.Father" lazy-init="false">
		<property name="son" ref="son-non-lazy"/>
	</bean>
	<bean id="son-non-lazy" class="org.springframework.beans.testfixture.beans.Son" lazy-init="false">
		<property name="father" ref="father-non-lazy"/>
		<property name="mother" ref="mother-non-lazy"/>
	</bean>
</beans>

console log

second-thread
java.lang.IllegalArgumentException: son Shouldn't be null
...........
...........

The code after my changes

@Nullable
protected Object getSingleton(String beanName, boolean allowEarlyReference) {
  // Quick check for existing instance without full singleton lock
  Object singletonObject = this.singletonObjects.get(beanName);
  if (singletonObject == null && isSingletonCurrentlyInCreation(beanName)) {
	  synchronized (this.singletonObjects) {
		  singletonObject = this.singletonObjects.get(beanName);
		  if (singletonObject != null || !isSingletonCurrentlyInCreation(beanName)) {
			  return singletonObject;
		  }
		  singletonObject = this.earlySingletonObjects.get(beanName);
		  if (singletonObject == null && allowEarlyReference) {
			  ObjectFactory<?> singletonFactory = this.singletonFactories.get(beanName);
			  if (singletonFactory != null) {
				  singletonObject = singletonFactory.getObject();
				  this.earlySingletonObjects.put(beanName, singletonObject);
				  this.singletonFactories.remove(beanName);
			  }
		  }
	  }
  }
  return singletonObject;
}

[pivotal-issuemaster]

@lijinxiong Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-issuemaster]

@lijinxiong Thank you for signing the Contributor License Agreement!

[quaff]

Unit test is required.

[lijinxiong]

Unit test is required.

The code is in the comments above

[sbrannen]

Unit test is required.

The code is in the comments above

@quaff is correct: when providing a PR to address an issue, you need to introduce a JUnit Jupiter based unit test that fails before the fix and passes after the fix.

A Java class with a main() method that demonstrates the issue does not count as a test since a class with a main() method cannot be included in the Spring Framework test suite.

[lijinxiong]

Unit test is required.

The code is in the comments above

@quaff is correct: when providing a PR to address an issue, you need to introduce a JUnit Jupiter based unit test that fails before the fix and passes after the fix.

A Java class with a main() method that demonstrates the issue does not count as a test since a class with a main() method cannot be included in the Spring Framework test suite.

@sbrannen Do I need to submit code for unit tests?
Or just give it in the comments
Thanks！

[sbrannen]

@sbrannen Do I need to submit code for unit tests?

Tests need to be submitted as part of the PR.

You will find existing tests under src/test/java in each module.

For example, for the class you have modified, you will find org.springframework.beans.factory.support.DefaultSingletonBeanRegistryTests in the spring-beans module.

Note, however, that a pure unit test does not always suffice. In other words, you may need to create a dedicated integration test. For examples, feel free to browse the existing test suite. It often helps to search in the same package for related test classes.

[lijinxiong]

@sbrannen Do I need to submit code for unit tests?

Tests need to be submitted as part of the PR.

You will find existing tests under src/test/java in each module.

For example, for the class you have modified, you will find org.springframework.beans.factory.support.DefaultSingletonBeanRegistryTests in the spring-beans module.

Note, however, that a pure unit test does not always suffice. In other words, you may need to create a dedicated integration test. For examples, feel free to browse the existing test suite. It often helps to search in the same package for related test classes.

Thank you very much

[lijinxiong]

@sbrannen Do I need to submit code for unit tests?

Tests need to be submitted as part of the PR.

You will find existing tests under src/test/java in each module.

For example, for the class you have modified, you will find org.springframework.beans.factory.support.DefaultSingletonBeanRegistryTests in the spring-beans module.

Note, however, that a pure unit test does not always suffice. In other words, you may need to create a dedicated integration test. For examples, feel free to browse the existing test suite. It often helps to search in the same package for related test classes.

Unit tests already in pr
Thanks

[JackieGGu]

@lijinxiong I also found this problem recently, but from the previous issues #25667, It can be seen that Jhoeller is trying to solve #25667 thread deadlock, but the code in your PR will make Spring return to the thread deadlock problem again, Can you offer a better way to solve this problem?

[zhangdefu159]

@lijinxiong
if (singletonObject == null && allowEarlyReference)
-->
if(!((singletonObject != null && this.singletonFactories.get(beanName) == null) && (singletonObject == null && this.singletonFactories.get(beanName) != null)) && allowEarlyReference)

You can try changing the top line of code to the third line of code. Will this problem still occur

[jhoeller]

Completely removing the early singleton check like in this PR is not really a proper way out. If you'd like to avoid the circular reference problem completely, set your factory to setAllowCircularReferences(false) which will avoid any exposure to earlySingletonObjects to begin with, therefore reaching the same effect as dropping the early singleton check in the PR.

Note that we are considering to disallow circular references by default in a future Spring Framework generation. However, as long as we need to be able to resolve circular references, the current code still seems to be the best we can do there.

Also, please note that we generally recommend against concurrent bean initialization scenarios. In particular with respect to circular reference resolution, a single-threaded preInstantiateSingletons phase is the most reliable arrangement. If you need to initialize with multiple threads, at least apply setAllowCircularReferences(false) so that it becomes as reliable as possible.