Reputation: 9829
I came across the following piece of code ,and noted a few inconsistencies - for a multi-thread safe code.
Map<String,Map<String,Set<String>> clusters = new HashMap<.........>;
Map<String,Set<String>> servers = clusters.get(clusterkey);
if(servers==null){
synchronized(clusterkey){
servers = clusters.get(clusterkey);
if(servers==null){....initialize new hashmap and put...}
}
}
Set<String> users=servers.get(serverkey);
if(users==null){
synchronized(serverkey){
users=servers.get(serverkey);
if(users==null){ ... initialize new hashset and put...}
}
}
users.add(userid);
Upvotes: 1
Views: 97
Reputation: 2051
The code looks like a non-thought through version of the Double checked locking idiom that sometimes is used for lazy initialisation. Read the provided link for why this is a really bad implementation of it.
The problem with the given code is that it fails intermittently. There is a race condition when there are several threads trying to work on the map using the same key (or keys with the same hashcode) which means that the map created first might be replaced by the second hashmap.
Upvotes: 2
Reputation: 19910
Here just some observations:
String
is a very bad idea -> sync on clusterKey
and serverKey
will probably not work the way intended.ConcurrentHashMap
s and ConcurrentHashSet
s.Though without more context it is not really possible to answer this question. It seems the code-author wanted to safely create just 1 mapping per clusterKey
and serverKey
so the user can be added just once.
A (probably better) way would be to just synchronize
on the clusters
map itself and then you're safe as only one thread can read and/or write to said map.
Another way would be to use custom Lock
s, maybe one for reading, and another one for writing, though this may lead again to inconsistencies if one thread is writing to the Map
while another is reading that exact value from it.
Upvotes: 4
Reputation: 11900
1 -The synch is trying to avoid that two threads, at the same time, create a new Entry in that Map. The second one must wait so his (servers==null)
doesn't also return true
.
2 - That users
list seems to be out of scope, but seems like it doesn't need a synch. Maybe the programmer knows there is no duplicated userIds, or maybe he doesn't care about resetting the same user again and again.
3- ConcurrentHashMap maybe?
Upvotes: 1