HashMap to synchronized Map is not working

[Anand Kumar Mmm]

Hi,
I have converted my HashMap to Synchronized Map and when I try to put some value into Map through Multi Thread its not working correctly. Below are sample code which I'm using

public class CollectionsDemo implements Runnable {
	private Map<String, String> map = null;
	public CollectionsDemo() {
		map = new HashMap<String, String>();
	}
	public static void main(String[] args) {
		CollectionsDemo obj = new CollectionsDemo();
		obj.addValues();
		obj.makeSynchronize();
		for(int i=0;i<5;i++){
			new Thread(obj).start();
		}	
		obj.printValues();
	}
	public void printValues(){
		try {
			Thread.sleep(1000);
		} catch (InterruptedException e) {
			e.printStackTrace();
		}
		System.out.println("VALUE ::: "+map.get("1"));
	}
	public void makeSynchronize(){
		map = Collections.synchronizedMap(map);
	}
	public void addValues(){
		map.put("1", "java-");
	}	
	public void run() {
		String tname = Thread.currentThread().getName();
		map.put("1", (map.get("1")+tname));
		System.out.println(tname + " : "+map.get("1"));
	}
}

OUTPUT:
Thread-1 : java-Thread-1Thread-2
Thread-2 : java-Thread-1Thread-2
Thread-0 : java-Thread-1Thread-2
Thread-3 : java-Thread-1Thread-2Thread-3
Thread-4 : java-Thread-1Thread-2Thread-3Thread-4
VALUE ::: java-Thread-1Thread-2Thread-3Thread-4


Where as program needs to give out by upending all thread names from 0-4. But in above output Thread-0 is missing. When I make Map to Synchronized then it should be thread safe and all thread names must to added in Map value.
So any one let me know what is the issue in my code ?

Thanks
Anand

[Manuel Petermann]

Please edit your post to make use of codetags. e.g select your code and click on the code button.

[Anand Kumar Mmm]

Done

Manuel Petermann wrote:Please edit your post to make use of codetags. e.g select your code and click on the code button.

[Steve Luke]

Read the API for Collections#synchronizedMap(). The synchronized map ensures that a single method call is processed atomically (a get() or a set() call). It does not ensure that consecutive methods calls (a get() followed by a set()) will provide consistent data. And this is your problem. You have a get() (get the current value of "1") followed by a set() (set the value of "1" to the old value + a new string) followed by a get() (get the new value to print). These three steps are not performed atomically, and so are no guaranteed to be consistent. and that it what is happening. Some place in between Thread-0's get-set-get sequence, another thread is setting the value. As a matter of fact, the way it looks for that particular run, Thread-2 gets the value, then Thread-0 gets the value and sets the value, then Thread-2 sets the value and prints the value, then Thread-0 prints the value.

You need to add your own synchronization to ensure that block of code, which is required to be consistent, runs in an atomic fashion.

[Anand Kumar Mmm]

Hi Steve,
Thanks for reply.
Actually my question is at final after processing all 5 Threads then value in the Map must be with all Thread names. Its missing in my output.
You can see in

 VALUE ::: java-Thread-1Thread-2Thread-3Thread-4 

Where I can see Thread-1,2,3,4 and Thread-0 is missing the final value of Map.

Do I need to place synchronized block need to use for this?

Thanks
Anand

[Manuel Petermann]

Same reason as stated by Steve.
You are basically just printing the "last" iteration already printed by your Thread.
I think the Collections.synchronized... should be avoided in most cases because it implicates a kind of security which is simply not there.

[Steve Luke]

Anand Kumar Mmm wrote:Where I can see Thread-1,2,3,4 and Thread-0 is missing the final value of Map.

Re-read my explanation. Use a pencil and paper to follow the execution sequence I described. It is what causes this problem.

Anand Kumar Mmm wrote:Do I need to place synchronized block need to use for this?

Yes, and use the map as the source for the synchronization lock.

Manuel Petermann wrote:I think the Collections.synchronized... should be avoided in most cases because it implicates a kind of security which is simply not there.

I wouldn't go that far. The synchronized collections provide memory consistency when accessing collections across multiple threads. They serve their purpose well and save me from having to maintain my own version of these same classes. People just have to read the documentation and use them correctly.