Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consistent thread-safe iteration in DefaultSingletonBeanRegistry [SPR-16620] #21161

Closed
spring-projects-issues opened this issue Mar 20, 2018 · 3 comments
Assignees
Labels
in: core status: backported type: bug
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Mar 20, 2018

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 #1745

Backported to: 4.3.15

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 23, 2018

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.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 24, 2018

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.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 25, 2018

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.

@spring-projects-issues spring-projects-issues added type: bug status: backported in: core labels Jan 11, 2019
@spring-projects-issues spring-projects-issues added this to the 5.0.5 milestone Jan 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core status: backported type: bug
Projects
None yet
Development

No branches or pull requests

2 participants