ConcurrentModificationException

[Jose Alvarez de Lara]

Hi,

I am having troubles in the following method,

private void put(ConcurrentMap<HTMLMessage, List<DataFile>> hashMap, HTMLMessage htmlMessage, List<DataFile> dfList) {
    	Set<HTMLMessage> keys = hashMap.keySet();
    	HTMLMessage htmlMsg = null;
		boolean exists = false;
    	Iterator<HTMLMessage> it = keys.iterator();
    	while(it.hasNext()){
    		htmlMsg = it.next();
    		if(htmlMsg.compareEquals(htmlMessage)){ // true
    			exists = true;
    			break;
    		}
    	}
    	if(exists){
		List<DataFile> list = hashMap.get(htmlMsg);
		for(DataFile df: list){
			if(!dfList.contains(df))
				dfList.add(df);
		}
		hashMap.put(htmlMsg, dfList);
	} else {
		hashMap.put(htmlMessage, dfList);
	}
}

The reasson is that throws a java.util.ConcurrentModificationException when I call it
from inside another method in a Servlet.

The argument List<DataFile> dfList is declared as
List<DataFile> dfList = Collections.synchronizedList(new ArrayList<DataFile>())

The execution rules ok until a random point that the exception is throwed.

Any suggestion should be appreciated.

Kind regards,
Jose

[Martin Vajsar]

I see three issues in your code, however I might be wrong, because I don't know the context and meaning of your method, most importantly where the dfList variable comes from.

1) You're iterating over synchronized list on line 15. Even though the list is synchronized, the iteration as a whole isn't. Put the whole for loop into a synchronized(list) block.

2) You're replacing a list that was already in the map with the dfList given as a parameter. I think this might lead to race conditions in your code. I'd say you should iterate over dfList and put all items into the list you got from the map. If the ownership structure of your lists is more complicated that I expect, I may be wrong in this regard. The race condition is there in any case, though.

3) The lines 2 to 13 of your code seem to just look for the presence of htmlMessage in the map. Simple hashMap.get(htmlMessage) should do the same and at the same time your intent would be clear, this way it is not clear at all. Besides, the performance is horrible and the operation is utterly thread-unsafe. (If the reason is you didn't implement hashcode() in your HTMLMessage class, go forth and implement it. It is the only way to go.)

Moreover, it's possible that if two threads try to insert identical HTMLMessage into the map, both will succeed (and whoever executes last the statement on line 21, list inserted by the other threads will be lost). You should use putIfAbsent() - putIfAbsent(htmlMessage, dfList) would probably do it.

I may have missed some of your intents, so don't just blindly rewrite the code according to my advice, but I'm pretty sure your code is not thread-safe in these three aspects. Maybe someone else will prove me wrong?

[Jose Alvarez de Lara]

Hi Martin,

Yes, you are right. This is my code after debugging,

    private void put(ConcurrentMap<HTMLMessage, List<DataFile>> hashMap, HTMLMessage htmlMessage, List<DataFile> dfList) {
    	Set<HTMLMessage> keys = hashMap.keySet();
    	HTMLMessage htmlMsg = null;
    	Iterator<HTMLMessage> it = keys.iterator();
    	synchronized(it){
	    	while(it.hasNext()){
	    		htmlMsg = it.next();
	    		if(htmlMsg.compareEquals(htmlMessage)){ // true
	    			List<DataFile> list = hashMap.get(htmlMsg);
	    			for(DataFile df: list){
	    				if(!dfList.contains(df))
	    					dfList.add(df);
	    			}
	    			hashMap.put(htmlMsg, dfList);
	    		} else {
	    			hashMap.put(htmlMessage, dfList);
	    		}
	    	}
    	}
}

This works fine.

Kind regards,
Jose

[Martin Vajsar]

Hi Jose,

unfortunately, the new version of your code doesn't address any of the issues I've written about and still is not thread-safe: you synchronize on the iterator you got by calling keys.iterator(). Every call to the iterator() method creates a new instance (it is sort of inevitable, isn't it?). The synchronized (it) statement therefore does not prevent different threads from entering the synchronized section, because any two different threads will synchronize on different instances of the iterator. That the code now seems to work fine might be caused by a slight changes in timing between the two versions.

If you intent to write thread-safe applications, you really need to get better understanding of multithreaded programming. You can start here, but keep in mind that it will take a lot of practice to get good understanding of this topic.

Meanwhile try to rework your code to address all of the issues I've mentioned. I can help you a bit if you get stuck.

[Jose Alvarez de Lara]

Hi Martin,

I have saved the Concurrency tutorial in my favourite’s folder of the browser.

It is clear I have to study about this stuff.

Regarding the method I have changed my code that means
I do not going to need it any longer.

Kind regards,
Jose