Synchronized collection

Question

Since c is already synchronzied collection, and it's thus thread safe. But why do we have to use synchronized(c) again for the iteration? Really confused. Thanks.

" It is imperative that the user manually synchronize on the returned collection when iterating over it:

Collection c = Collections.synchronizedCollection(myCollection);
 ...
 synchronized(c) {
     Iterator i = c.iterator(); // Must be in the synchronized block
     while (i.hasNext()) {
         foo(i.next());
     }
}

Failure to follow this advice may result in non-deterministic behavior. " http://docs.oracle.com/javase/6/docs/api/java/util/Collections.

Facelift

Answer 1

The most any synchronized collection implementation could possibly do is to guarantee that each individual method call is synchronized. But iteration necessarily involves multiple separate method calls, so you, the user of the synchronized collection, have to synchronize on the whole iteration yourself.

For example, if you didn't synchronize on c, the contents of the collection could change between i.hasNext() and i.next() -- it could even go from having elements to having no more elements, in which case i.next() would fail.

Palinode

What do you mean by "necessarily involves multiple separate method calls"? – Facelift

@user697911: i.hasNext() and i.next() are two separate method calls, and there's no possible way for the implementation of Collections.synchronizedCollection to ensure that nothing changes in between i.hasNext() and i.next(). – Palinode

So, concurrentHashmap would be the same? Do we also have to synchronize the concurrenthashamp in iteration? – Facelift

No, you never need to synchronize ConcurrentHashMap. The docs for CHM state: "The view's iterator is a "weakly consistent" iterator that will never throw ConcurrentModificationException, and guarantees to traverse elements as they existed upon construction of the iterator, and may (but is not guaranteed to) reflect any modifications subsequent to construction." In other words, ConcurrentHashMap uses a specialized implementation that makes sure its iterators don't blow up when you modify the map during iteration, and those iterators may or may not take into account further modifications. – Palinode

ConcurrentHashMap can do things like that because concurrency is built in from the ground up; Collections.synchronizedCollection can only provide a simple wrapper around a not-normally-concurrent collection. – Palinode

Answer 2

Making all the methods on a class individually synchronized doesn't make an aggregation ( calling them in a group ) of those methods thread safe. By wrapping the Iterator in a synchronized block you are protecting that particular instance of the iterator from having its individual method called interspersed with other calls by multiple threads.

If I call .add() once it is safe, if I need to call .add() multiple times to complete a logical statement, there is no guarantee that someone else hasn't added something else or removed something else between my .add() calls unless I block everything else from calling .add() ( or any other method ) by synchronizing on the variable that represents the collection.

The Iterator makes mutiple calls to the individual methods on the collection, they all have to be wrapped in a single synchronized block to make them execute as a single transaction of sorts. Examine the source code of the implemenation of Iterator you will see what I mean. Here is the source code for List it makes multiple individual calls to the underlying implementation, so they all need to be executed in uninterrupted order by the same thread to be deterministic.

  @Override
    public Iterator<A> iterator() {
        if (tail == null)
            return emptyIterator();
    return new Iterator<A>() {
        List<A> elems = List.this;
        public boolean hasNext() {
        return elems.tail != null;
        }
        public A next() {
                if (elems.tail == null)
                    throw new NoSuchElementException();
        A result = elems.head;
        elems = elems.tail;
        return result;
        }
        public void remove() {
        throw new UnsupportedOperationException();
        }
    };
    }

The source for AbstractList.iterator() shows even more complicated logic that makes multiple calls.

A better wrapper is wrapping them in Immutable collections, then you guarantee that nothing else can alter the underlying collection between calls.

Enforcement

best explanation I have come across! – Mcculley