CODEWITHSUNDEEP
javahashmapsynchronized
IUnknown
IUnknown

Reputation: 9839

thread safe map operation

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);
  1. Why would map be synchronized on clusterkey- shouldnt it be on the map as monitor itself?
  2. Shouldnt the last users.add... be synchronized too?
  3. This seems to be a lot of code to add a single user in a thread-safe manner.What would be a smarter implementation?

Upvotes: 1

Views: 97

Answers (3)

Erik
Erik

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

Lino
Lino

Reputation: 19910

Here just some observations:

  1. Synchronizing on a String is a very bad idea -> sync on clusterKey and serverKey will probably not work the way intended.
  2. Better would be to use ConcurrentHashMaps and ConcurrentHashSets.

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 Locks, 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

aran
aran

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

Related Questions