Improve failure mode when depends-on cycles exist [SPR-7966]

[spring-projects-issues]

Archie Cobbs opened SPR-7966 and commented

I had an application context containing a bogus cyclic dependency like this:

<bean id="bean1" depends-on="bean2" .../>
<bean id="bean2" depends-on="bean1" .../>

Obviously this is misconfigured. But the error message you get from Spring is a StackOverflowError:

	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.initializeBean(AbstractAutowireCapableBeanFactory.java:1420)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:519)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:456)
	at org.springframework.beans.factory.support.AbstractBeanFactory$1.getObject(AbstractBeanFactory.java:291)
	at org.springframework.beans.factory.support.DefaultSingletonBeanRegistry.getSingleton(DefaultSingletonBeanRegistry.java:222)
	at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:288)
	at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:190)
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.preInstantiateSingletons(DefaultListableBeanFactory.java:580)
	at org.springframework.context.support.AbstractApplicationContext.finishBeanFactoryInitialization(AbstractApplicationContext.java:895)
	at org.springframework.context.support.AbstractApplicationContext.refresh(AbstractApplicationContext.java:425)
	at org.springframework.context.support.ClassPathXmlApplicationContext.<init>(ClassPathXmlApplicationContext.java:197)
	at org.springframework.context.support.ClassPathXmlApplicationContext.<init>(ClassPathXmlApplicationContext.java:172)
	at org.springframework.context.support.ClassPathXmlApplicationContext.<init>(ClassPathXmlApplicationContext.java:158)
        ... application-specifc stack frames ...
Caused by: java.lang.StackOverflowError
	at java.util.HashMap.put(HashMap.java:389)
	at java.util.HashSet.add(HashSet.java:217)
	at java.util.Collections$SynchronizedCollection.add(Collections.java:1593)
	at org.springframework.beans.factory.support.AbstractBeanFactory.markBeanAsCreated(AbstractBeanFactory.java:1363)
	at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:271)
	at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:190)
	at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:281)
	at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:190)
	at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:281)
	at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:190)
	at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:281)
	at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:190)
        ...

This failure mode could be improved to better indicate the source of the problem.

Doing so could be implemented easily e.g. by keeping track as the dependency graph is explored of the members in the current dependency path in a ThreadLocal<HashMap>.

---

Affects: 3.0.5

Issue Links:

• Unnecessarily conservative locking in registerDependentBean is a bottleneck [SPR-8814] #13456 Unnecessarily conservative locking in registerDependentBean is a bottleneck

Referenced from: commits e48c315, bd84699

[spring-projects-issues]

Soungmin Joo commented

DefaultSingletonBeanRegistry.class seems already have member variable 'dependenciesForBeanMap'
to keep 'depends-on' relations of beans. And AbstractBeanFactory seems verifiy 'depends-on' definition
of a bean through following code.

// Guarantee initialization of beans that the current bean depends on.			
String[] dependsOn = mbd.getDependsOn();			
if (dependsOn != null) {				
    for (String dependsOnBean : dependsOn) {					
        getBean(dependsOnBean);					
        registerDependentBean(dependsOnBean, beanName);				
    }			
}

StackOverflowError occurs from inside of getBean() method because above code invokes
infinitely changeing value of 'dependsOnBean' variable to 'bean1' and 'bean2'.

The next method 'registerDependentBean' saves dependency relation into ConcurrentHashMap.

Because 'getBean' method is invoked prior to 'registerDependentBean' method,
depends-on relations cannot be accessed inside of 'getBean' method.

So I have tried to change above code into following code which creates BeanCreationException
in case of circular 'depends-on'. And I'm preparing for pull-request according to this issue.

Is this approach OK?

String[] dependsOn = mbd.getDependsOn();			
if (dependsOn != null) {				
    for (String dependsOnBean : dependsOn) {					
        registerDependentBean(dependsOnBean, beanName);        
        checkCircularDependsOn(dependsOnBean, beanName);
        registerDependenciesForBean(dependsOnBean, beanName);
        getBean(dependsOnBean);								
    }			
}

private void checkCircularDependsOn(String dependsOnBean, String beanName) {
    String[] dependenciesForBean = this.getDependenciesForBean(dependsOnBean);
    for(int i = 0; i < dependenciesForBean.length; i++) {
        if(dependenciesForBean[i].equals(beanName)) {
            throw new BeanCreationException(beanName,
                    "Beans with name '" + dependsOnBean + "' and '" + beanName + 
                    "' are dependent on each other; " + 
                    "consider removing 'depends-on' attribute from one bean definition.");
        }
    }
}