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

SimpleAliasRegistry registerAlias not atomic [SPR-16577] #21119

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

Comments

@spring-projects-issues
Copy link
Collaborator

spring-projects-issues commented Mar 10, 2018

László Magyar opened SPR-16577 and commented

org.springframework.core.SimpleAliasRegistry class registerAlias method get - compute - put is not atomic.
It should be synchronized with this.aliasMap or java.util.concurrent.ConcurrentMap interface compute method should be used.


Affects: 4.3.14, 5.0.4

Issue Links:

Referenced from: commits 3dff1b3, 1b1a69a

Backported to: 4.3.15

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Mar 10, 2018

László Magyar commented

Also I'm not sure the synchronization of the entire aliasMap is valid at all since inside the ConcurrentHashMap other lock obejects used.
I did not track the history back of this class, but it seems to me at the be beginning this aliasMap field was a simple HashMap or Collections.synchronizedMap() where the synchronization of the entire aliasMap was valid.
So Collections.synchronizedMap should used again, or use the appropriate methods of ConcurrentHashMap without outer synchronization attempt.

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Mar 10, 2018

Juergen Hoeller commented

Synchronization of the entire Map is valid as long as it is done consistently for certain kinds of access to the Map, not expecting direct Map operations to participate in that lock. We're doing that in quite a few places for a ConcurrentHashMap with such selective external synchronization.

Since SimpleAliasRegistry doesn't leak its Map to the outside, this is also a feasible compromise here: I've guarded registerAlias and removeAlias with synchronization, allowing the existing synchronized resolveAliases method and others to reliably see consistent state. Individual resolution of aliases still operates on the underlying Map directly, so it still makes sense to keep it as a ConcurrentHashMap (if all access to it was guarded, we could make it a regular HashMap).

That said, registerAlias is usually a startup-time operation in a BeanFactory, usually with no further modifications once factory initialization has finished. This is the main reason why it is worth keeping individual alias resolution via direct ConcurrentHashMap access at runtime.

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Mar 11, 2018

László Magyar commented

So if I get it right, synchronization on the entire map needed to get a consistent view of the map when iterate over it's elements.
But if it is the case, all map modifications methods (put, remove etc.) should guarded by the same lock as the iteration (which is the entire map).

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Mar 11, 2018

Juergen Hoeller commented

Indeed, iteration needs to be guarded the same way (e.g. the existing synchronization in resolveAliases. It's only individual lookups - the typical case for bean retrieval - that go straight to the underlying ConcurrentHashMap, providing consistent visibility of individual entries without a full lock. So overall, adding explicit synchronization to registerAlias and removeAlias should do the trick here, in particular applicable because of the check for existing entries and the handling of same-named aliases which perform several operations on the underlying Map. I'll push the corresponding commit tomorrow morning.

We usually fine-tune synchronization according to our common framework use cases. We could even enforce that registration and removal may only happen during single-threaded startup, then we wouldn't need any synchronization for aliases at all. While startup-time registration is the 99.9% case, we nevertheless allow for later registration and also for later overriding which is why we need some synchronization to begin with. We don't want to compromise performance for the common case though, hence the efficient direct lookup of individual entries on an underlying ConcurrentHashMap. Aliases are one affected case here but the key area is bean instance retrieval for singleton beans where most such beans are created on startup... but some bean instances may get created lazily on demand, and in very rare scenarios there may also be runtime registration of new singleton beans or runtime overriding of existing singleton beans.

@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