Reputation: 3148
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);
}
}
}
Upvotes: 4
Views: 163
Reputation: 1610
Edited
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.
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
, ortailSet
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()); }
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.
Upvotes: 0
Reputation: 7810
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.
Upvotes: 4