FindBug says this concurrent map doesnt need synchronized

Question

So I've recently discovered FindBug, but it's making me think I don't know what I'm doing in a couple of places. This is one of them

private Map<String, Object> map = new ConcurrentHashMap<String, Object>();

public void method1(){
    synchronized(map){ // FindBug says this is unnecessary
        for (String keys: map.keySet()){
            ...
        }
    }        
}

I thought that I needed to synchronize the iteration, or is this a case of FindBug not being smart enough to realize it's necessary? I just want to make sure!

http://stackoverflow.com/questions/3768554/is-iterating-concurrenthashmap-values-thread-safe — Thihara, May 15 '13 at 03:47

score 3 · Accepted Answer · answered May 15 '13 at 03:36

3

The iterators for ConcurrentHashMap are "weakly consistent," meaning that they reflect the state of the map at the time that the iterator was created but may not reflect modifications made to the map after the iterator was created; in other words, the iterator isn't going to throw a ConcurrentModificationException, so you probably don't need to lock the underlying map.

answered May 15 '13 at 03:36

Zim-Zam O'Pootertoot

17,888
4
41
69

But doesn't the locking prevent elements from being added or removed during my iteration? – Jay Smith May 15 '13 at 03:38
You'll need to synchronize any writes to the map to block them during iteration which defeats part of the benefit of SHM. – David Harkness May 15 '13 at 15:46
1

"But doesn't the locking prevent elements from being added or removed during my iteration?" _No, it doesn't._ It does _nothing._ – Louis Wasserman May 15 '13 at 16:23
Could you explain more fully as an answer Louis? What you are saying seems to be the root of FindBugs warning, but I don't understand how if I synchronize the datastructure that it wouldn't prevent adding/deleting – Jay Smith May 15 '13 at 20:31
1

@Jay Smith Synchronizing the data structure would only prevent adding/deleting if all adds/deletes were done within synchronized blocks/methods, which would defeat the purpose of using a `ConcurrentHashMap`. – Zim-Zam O'Pootertoot May 15 '13 at 20:39
*sigh, feel like such a noob. Alright, so basically i need to either synchronize everything, or i just need to not care if the iterator has a null or nonvalid object due to modification – Jay Smith May 15 '13 at 20:46
@Jay Smith Depends what you mean by "invalid object;" for example, if an object is invalid if it was added to the map after the iterator was initialized, then associate a timestamp with all map values (something like `map.put(key, new ValueTimeStamp(object, System.currentTimeMillis())`) and ignore values whose timestamps are greater than the time at which you initialized the iterator – Zim-Zam O'Pootertoot May 15 '13 at 20:55
I just meant that I need to cope with objects that could have been deleted during the iteration. I'll figure out some solution, many thanks! – Jay Smith May 15 '13 at 21:04

FindBug says this concurrent map doesnt need synchronized

1 Answers1