FactoryBeanRegistrySupport atomicity issues [SPR-16625]

[spring-projects-issues]

László Magyar opened SPR-16625 and commented

Within org.springframework.beans.factory.support.FactoryBeanRegistrySupport found some issues with atomic operation.
Please check PR.

---

Affects: 4.3.14, 5.0.4

Issue Links:

• SimpleAliasRegistry registerAlias not atomic [SPR-16577] #21119 SimpleAliasRegistry registerAlias not atomic
• Consistent thread-safe iteration in DefaultSingletonBeanRegistry [SPR-16620] #21161 Consistent thread-safe iteration in DefaultSingletonBeanRegistry
• Address race condition within spring that causes about-to-be-created-bean exceptions [SPR-16627] #21168 Address race condition within spring that causes about-to-be-created-bean exceptions

Referenced from: pull request #1749

Backported to: 4.3.15

[spring-projects-issues]

Juergen Hoeller commented

Good catch, this may indeed lead to race conditions in on-the-fly removal during live operations. I've revised the code accordingly, inspired by your pull request but somewhat more extensively: including factoryBeanInstanceCache in AbstractAutowireCapableBeanFactory, and with a new clearSingletonCache() template method that reduces the scope of the singleton lock. Also, the extra containsSingleton check isn't actually necessary once all removal attempts happen consistently within the singleton lock since the factoryBeanObjectCache will always be in sync.

[spring-projects-issues]

László Magyar commented

Juergen, please check my other created ticket and it's PR, it is related to this maybe.
It's number #21161.
Please check the synchronized wrapper usage when adding elements to containedBeanMap and dependentBeanMap, also the synchronization when iterating over these collections.
In my opinion it is required.

[spring-projects-issues]

László Magyar commented

I've checked the code.
The extra containsSingleton check was because maybe between the returned true, and the synchronization some thread removed this bean from singletons.

[spring-projects-issues]

Juergen Hoeller commented

I see, your concern there is effectively the singleton nature of the given FactoryBean instance: If it gets replaced by a non-singleton FactoryBean definition at runtime, we might accidentally decide to cache the instance in case of such a race condition... Even if the instance won't get reused later on when no singleton is found when reentering this code, we won't clean it up either since removeSingleton assumes it has cleaned it up already on singleton removal.

Initially I only looked at it from the perspective of receiving an outdated instance there which is guaranteed to not happen with the synchronized removal from both data structures in removeSingleton now. I'll revisit it from the perspective of a hot replacement with a non-singleton, as outlined above. However, this is a very unusual scenario since replacements are expected to be of the same kind in quite a few spots across the core container.

[spring-projects-issues]

Juergen Hoeller commented

I've added another containsSingleton guard but only right before putting into the factoryBeanObjectCache where we need to make sure we're not adding to it when no singleton exists anymore at that point. All other conditions are being enforced through consistent synchronization in the meantime, and we keep the "happy path" free from an extra check here.