Java Concurrency In Practice. Listing 5.6

Question

In Java Concurrency in Practice author gives the following example of a not-thread safe class, that behind the scenes invokes iterator on a set object and if multiple threads are involved, this may cause a ConcurrentModificationException. This is understood, one thread is modifying the collection, the other is iterating over it and, - boom!

What I do not understand, - the author is saying that this code can be fixed by wrapping a HashSet with Collections.synchronizedSet(). How would this fix a problem? Even though access to all the methods will be synchronized and guarded by the same intrinsic lock, once the iterator object is obtained, there is no guarantee that the other thread won't modify the collection once an iteration is being made.

Quote from the book:

If HiddenIterator wrapped the HashSet with a synchronizedSet, encapsulating the synchronization, this sort of error would not occur.

public class HiddenIterator {

    //Solution : 
    //If HiddenIterator wrapped the HashSet with a synchronizedSet, encapsulating the synchronization, 
    //this sort of error would not occur. 
    //@GuardedBy("this") 
    private final Set<Integer> set = new HashSet<Integer>();

    public synchronized void add(Integer i) {
        set.add(i);
    }

    public synchronized void remove(Integer i) {
        set.remove(i);
    }

    public void addTenThings() {
        Random r = new Random();
        for (int i = 0; i < 10; i++)
            add(r.nextInt());
        /*The string concatenation gets turned by the compiler into a call to StringBuilder.append(Object), 
         * which in turn invokes the collection's toString method - and the implementation of toString in 
         * the standard collections iterates the collection and calls toString on each element to
         * produce a nicely formatted representation of the collection's contents. */
        System.out.println("DEBUG: added ten elements to " + set);
    }
}

If someone could help me understand that, I'd be grateful.

Here is how I think it could've been fixed:

public class HiddenIterator {

    private final Set<Integer> set = Collections.synchronizedSet(new HashSet<Integer>());

    public void add(Integer i) {
        set.add(i);
    }

    public void remove(Integer i) {
        set.remove(i);
    }

    public void addTenThings() {
        Random r = new Random();
        for (int i = 0; i < 10; i++)
            add(r.nextInt());
        // synchronizing in set's intrinsic lock
        synchronized(set) {
            System.out.println("DEBUG: added ten elements to " + set);
        }
    }
}

Or, as an alternative, one could keep synchronized keyword for add() and remove() methods. We'd be synchronizing on this in this case. Also, we'd have to add a synchronized block (again sync'ed on this) into addTenThings(), which would contain a single operation - logging with implicit iteration:

public class HiddenIterator {

    private final Set<Integer> set = new HashSet<Integer>();

    public synchronized void add(Integer i) {
        set.add(i);
    }

    public synchronized void remove(Integer i) {
        set.remove(i);
    }

    public void addTenThings() {
        Random r = new Random();
        for (int i = 0; i < 10; i++)
            add(r.nextInt());
        synchronized(this) {
            System.out.println("DEBUG: added ten elements to " + set);
        }
    }
}

Answer 1

Edited

synchronizedSet()::toString()

As Sergei Petunin pointed out rightly, the toString() method of Collections.synchronizedSet() internally takes care about synchronisation, so no manual synchronistion is necessary in this case.

external iteration on synchronizedSet()

once the iterator object is obtained, there is no guarantee that the other thread won't modify the collection once an iteration is being made.

In cases of external iteration, like using for-each or an Iterator, the approach with encapsulating that iteration in an synchronize(set) block is required/sufficient.

That's why the JavaDoc of Collections.synchronizedSet() states, that

It is imperative that the user manually synchronize on the returned sorted set when iterating over it or any of its subSet, headSet, or tailSet views.

  SortedSet s = Collections.synchronizedSortedSet(new TreeSet());
      ...   
  synchronized (s) {
      Iterator i = s.iterator(); // Must be in the synchronized block
      while (i.hasNext())
          foo(i.next());   
  }

manual synchronization

Your second version with the synchronized add/remove methods of the class HiddenIterator and synchronize(this) would work too, but it introduces unneccesarry overhead as adding/removing would be synchronized twice (by HiddenIterator and Collections.synchronizedSet(..).

However, in this case you could omit the Collections.synchronizedSet(..) as HiddenIterator takes care of all the synchronization required when accessing the private Set field.

Answer 2

Collections.synchronizedSet() wraps the collection in an instance of an internal class called SynchronizedSet, extending SynchronizedCollection. Now let's look how's the SynchronizedCollection.toString() is implemented:

public String toString() {
    synchronized (mutex) {return c.toString();}
}

Basically the iteration is still there, hidden in the c.toString() call, but it's already synchronized with all other methods of this wrapper collection. So you don't need to repeat the synchronization in your code.