Description
Rich Head opened SPR-4932 and commented
I have come across a threading bug in the Spring libraries when using the AutowiredAnnotationBeanPostProcessor.
The symptoms are random NullPointerExceptions being thrown due to the autowiring of beans not being performed.
The problem is due to the RootBeanDefinition of prototypes being initialized by more then one thread.
The problem exists in 2.5.1 and also 2.5.4. It may exist in other releases but these are the two I looked at.
The problem starts when two threads try to create a prototype bean of the same type.
For example:
MyWebController uses the following code to get a referecne to the prototype bean MyService.
MyService emailService = (MyService)getApplicationContext().getBean( "MyService");
MyService has an autowired property which is a singleton DAO named MyDAO.
@Service("MyService")
@Scope("prototype")
public class MyService_Impl implements MyService
{
@Autowired
@Qualifier("MyDAO")
private MyDAO myDAO
. . .
}
The problem occurs when two threads try to get an instance for the first time to the MyService bean. The myDAO is NOT autowired.
In the spring libraries, the problem starts when two threads enter the following method during a request to create the prototype bean MyService.
org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean()
The method looks like this...
protected Object doCreateBean(final String beanName, final RootBeanDefinition mbd, final Object[] args)
{
. . .
// Allow post-processors to modify the merged bean definition.
if(!mbd.postProcessed)
{
applyMergedBeanDefinitionPostProcessors(mbd, beanType, beanName);
mbd.postProcessed = true;
}
. . .
}
If more then two threads enter the applyMergedBeanDefinitionPostProcessors() for the same RootBeanDefinition at the same time, the properties (fields) to be autowired (injected) are lost.
The best way to see this is set a break point inside of doCreateBean() and allow two threads to enter the applyMergedBeanDefinitionPostProcessors() one at a time. Note that the check "if(!mbd.postProcessed)" is NOT thread safe if two threads have already entered the if() statement.
The applyMergedBeanDefinitionPostProcessors() method is used to allow Bean Post Processers to have their postProcessMergedBeanDefinition() called.
When using the autowire annotation, the org.springframework.beans.factory.annotation.AutowiredAnnotationBeanPostProcessor is registered.
For the MyService bean, the AutowiredAnnotationBeanPostProcessor bean has its postProcessMergedBeanDefinition() called.
In the AutowiredAnnotationBeanPostProcessor.postProcessMergedBeanDefinition() method the findAutowiringMetadata() method is called.
The findAutowiringMetadata() is used to get the InjectionMetadata for the bean and store this object into a local cache in the AutowiredAnnotationBeanPostProcessor.injectionMetadataCache map field.
The InjectionMetadata contains members, both fields and methods that need to be autowired (injected) for this bean.
When the first thread enters this code, this works fine and returns the InjectionMetadata.
NOTE THAT THIS INSTANCE of InjectionMetadata IS CACHED IN THE "AutowiredAnnotationBeanPostProcessor.injectionMetadataCache map field". As you will see below, the InjectionMetadata object in the map will be modified later.
Back in the postProcessMergedBeanDefinition() after the InjectionMetadata has been returned the following is called:
metadata.checkConfigMembers(beanDefinition);
Inside of checkConfigMembers() it calls:
doRegisterConfigMembers(beanDefinition, this.injectedFields);
Here the fields to be autowired are merged into the beanDefinition if it does not exist (see code below).
If it does exist, IT IS REMOVED from the input set "this.injectedFields" (The cause of the problem).
private void doRegisterConfigMembers(RootBeanDefinition beanDefinition, Set<InjectedElement> members) {
for (Iterator<InjectedElement> it = members.iterator(); it.hasNext();) {
Member member = it.next().getMember();
if (!beanDefinition.isExternallyManagedConfigMember(member)) {
beanDefinition.registerExternallyManagedConfigMember(member);
}
else {
it.remove();
}
}
}
For a single thread initializing this beanDefinition this is okay.
The problem is when another thread is also trying to initialize the same beanDefinition. Looking at the logic above, if the "Member" is already in the bean definition it is removed from the input Set of InjectedElements (see "it.remove()" from above). This Set of InjectedElements is the same set cached in the AutowiredAnnotationBeanPostProcessor.injectionMetadataCache map.
So, the beanDefinition (RootBeanDefinition.externallyManagedConfigMembers) is CORRECT but the AutowiredAnnotationBeanPostProcessor.injectionMetadataCache map field no longer has this member as a field to be autowired.
Future calls to findAutowiringMetadata() will return an InjectionMetadata from its cache but the fields (member) to be autowired will not exist.
So when it comes time to inject other beans into this bean (like the myDAO bean), and the findAutowiringMetadata() is called, the information about what fields of a bean should be autowired is lost and therefore, those properties will not be injected and will be null.
The fix I have is to only allow a single thread to ever perform a "merge bean definition post process".
This can be accomplished by modifying the following method:
org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean()
The change will replace the none-thread safe call with...
protected Object doCreateBean(final String beanName, final RootBeanDefinition mbd, final Object[] args)
{
. . .
// Allow post-processors to modify the merged bean definition.
if (!mbd.postProcessed)
{
synchronized(mbd)
{
if (!mbd.postProcessed)
{
applyMergedBeanDefinitionPostProcessors(mbd, beanType, beanName);
mbd.postProcessed = true;
}
}
}
. . .
}
I have tested this and it seems to work. My concern is the synchronization on the mbd itself. This seems like a good candidate to synch on but I am not sure what the impact will be if it is being synch on in other parts of the code. My concern would be a deadlock.
Affects: 2.5.4