thread safe map operation

Question

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);

Why would map be synchronized on clusterkey- shouldnt it be on the map as monitor itself?
Shouldnt the last users.add... be synchronized too?
This seems to be a lot of code to add a single user in a thread-safe manner.What would be a smarter implementation?

There is no right and wrong until we know the exact context. The code snippet you provided is not enough to tell us what the original implementer is thinking. — Jai, Feb 14 '19 at 07:59

Lino · Accepted Answer · 2019-02-14T08:20:22.013

Here just some observations:

Synchronizing on a String is a very bad idea -> sync on clusterKey and serverKey will probably not work the way intended.
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.

Erik · Answer 2 · 2019-02-14T09:24:30.383

2

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.

edited Feb 14 '19 at 09:24

answered Feb 14 '19 at 08:22

Erik

2,013
11
17

This doens't quite qualify as a link only answer, but still could just be a comment. Maybe add a citation and/or code from the wikipedia page you provided, maybe suggest a solution? – Lino Feb 14 '19 at 08:25
The OP wanted comments on what the code does, not a solution to get it to work consistently. But I can provide some more detail. – Erik Feb 14 '19 at 09:20

score 1 · Answer 3 · answered Feb 14 '19 at 07:57

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?

thread safe map operation

3 Answers3