CODEWITHSUNDEEP
javamultithreadingweb-servicessynchronizedconcurrenthashmap
wallen
wallen

Reputation: 311

Java: is using synchronized(this) an advisable practice when creating a ConcurrentHashMap object?

I just finished developing a java web service server for a distributed programming course I am attending. One of the requirements was to guarantee multi-thread safety to our project hence I decided to use ConcurrentHashMap objects to store my data. At the end of it all I am left with a question regarding this snippet of code:

    public List<THost> getHList() throws ClusterUnavailable_Exception{

    logger.entering(logger.getName(), "getHList");
    if(hMap==null){
        synchronized(this){
            if(hMap==null){
                hMap=createHMap();
            }
        }
    }
    if(hMap==null){
        ClusterUnavailable cu = new ClusterUnavailable();
        cu.setMessage("Data unavailable.");
        ClusterUnavailable_Exception exc = new ClusterUnavailable_Exception("Data unavailable.", new ClusterUnavailable());
        throw exc;
    }
    else{
        List<THost> hList = new ArrayList<THost>(hMap.values());
        logger.info("Returning list of hosts. Number of hosts returned = "+hList.size());
        logger.exiting(logger.getName(), "getHList");
        return hList;
    }
}

do I have to use the synchronized statement when creating the concurrenthashmap object itself in order to guarantee that the service will not have any unpredictable behavior in a multi-threaded environment?

Upvotes: 1

Views: 253

Answers (4)

Stephen C
Stephen C

Reputation: 719229

The simple solution is to avoid the problem by eagerly initializing. And unless you have clear evidence (i.e. profiling) that eager initialization is a performance problem, that is also the best solution.

As to your question, the answer is that the synchronized block is necessary for correctness. Without it you can get the following sequence of events.

  • thread 1 calls getHList()
  • thread 1 sees that hMap is null and starts to create a map.
  • thread 2 calls getHList()
  • thread 2 sees that hMap is null and starts to create a map.
  • thread 1 finishes creating, and assigns the new map to hMap, and returns that map.
  • thread 2 finishes creating, and assigns the second new map to hMap, and returns that map.

In short, thread 1 and thread 2 could get different maps if they simultaneously call getHList() while hMap has its initial null value.

(In the above, I'm assuming that getHList() is a getter for hMap. However, the method as written won't compile, and its declared return type doesn't match the type of hMap ... so it is unclear what it is really intended to do.)

Upvotes: 5

Aurand
Aurand

Reputation: 5537

Double check locking pattern is broken before Java 1.5 (and is inefficient in Java 1.6 and later). See: http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html

Consider using a Initialization-on-demand holder or a single element enum type.

Upvotes: 0

Steven Schlansker
Steven Schlansker

Reputation: 38526

Don't bother. Eagerly initialize the Map, make the field final, and drop the synchronization until you have proven that it is actually necessary. The cost is minuscule and the "obviously safe and correct" solution will almost never be too slow.

You mentioned this is a class project -- focus on getting the code working. Concurrency is hard enough without inventing additional obstacles that you must then hurdle over.

Upvotes: 7

Jayamohan
Jayamohan

Reputation: 12924

The below line has nothing to do with ConcurrentHashMap. Its just creating an instance of ConcurrentHashMap object. Its just like synchronizing any object creation in JAVA.

hMap=new ConcurrentHashMap<BigInteger, THost>();

Upvotes: 1

Related Questions