László Magyar opened SPR-16620 and commented
Refactor concurrency within org.springframework.beans.factory.support.DefaultSingletonBeanRegistry
Affects: 4.3.14, 5.0.4
Issue Links: - #21166 FactoryBeanRegistrySupport atomicity issues
Referenced from: pull request https://github.com/spring-projects/spring-framework/pull/1745
Backported to: 4.3.15
Comment From: spring-projects-issues
Juergen Hoeller commented
I see what you're going for here but I'm not sure extra synchronization is needed to quite this degree. BTW I've introduced use of computeIfAbsent for nested collection structures already but your PR does way more than that. I'll revisit the code over the weekend, analyzing the concurrency scenarios once more.
Comment From: spring-projects-issues
Juergen Hoeller commented
I've revised all of our iteration attempts over nested sets to synchronize on their contained map. As long as additions and removals happen within the same lock, there is no need for making those nested sets themselves synchronized. Also, on removal from their contained map, it is safe to iterate them without a lock since they are guaranteed to be disconnected from the map and fully visible to the receiving thread at that point (as long as additions and removals to the set consistently happen within the lock of the containing map).
In practice, none of this will really matter since those data structures are effectively getting populated on startup and introspected on shutdown... usually clearly distinct phases, and usually happening in the same thread. That said, there may be startup failure scenarios involving parallel bean initialization where those registration/removal attempts may interleave, so it is indeed necessary to apply this revision here.
Comment From: spring-projects-issues
László Magyar commented
Yes, I've just checked this. On my code base I've used the synchronized wrapper to get smaller synchronization block.
BTW great work on the whole spring project, I've learned a lot from this.