1

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?

Improve this question
4
  • 3
    Why would you wait to construct hMap until this point? Why not just initialize it at construction time? Commented Jan 30, 2013 at 1:40
  • The question applies to creating any object, not just ConcurrentHashMaps. Commented Jan 30, 2013 at 1:43
  • It is pattern I use when synchronizing the construction of array lists. In this particular case however I want hMap to be created once a client requests one of the operations my web service is offering. In fact I actually call hMap=createHMap(); in the inner if and do some unmarshalling inside. Commented Jan 30, 2013 at 1:44
  • 1
    While there are cases where you might want to delay construction, this isn't one of them. Creating an empty HashMap uses virtually no CPU time or memory. I strongly agree with Louis, just do it at construction time and make it final. If you were constructing a seldom used file directory tree of some remote server, then delay... Commented Jan 30, 2013 at 1:49

4 Answers 4

Reset to default
7

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.

Improve this answer
Sign up to request clarification or add additional context in comments.

1 Comment

I am convinced, I will change the code so that the map is initialized at construction time.
5

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

Improve this answer

1 Comment

That's exactly the scenario I had in mind when I started thinking that the synchronized block was necessary. The method calls a function which reads data from XML and stores it into said concurrent hash map, then constructs a list out of the values contained in the concurrent hash map and returns said list. I am sorry for truncating the method, it was probably pretty unclear.
1

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

Improve this answer

3 Comments

So I assume from your answer that using synchronized(this) is indeed suggested.
That depends. If you want to make your object creation synchronized, the answer is yes.
@wallen - a better idea it to avoid the problem entirely - see Steven Schlansker's answer.
0

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.

Improve this answer

1 Comment

According to that article, it's broken in Java 1.4 and earlier not 1.5 and earlier. Starting with 1.5 you can make it work using volatile (what the poster used).

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.