Principle
- To avoid liveness and safety failures, never cede control to the client within a synchronized method or block.
- Do as little work as possible inside synchronized regions.
- You should make a mutable class thread-safe (Item 70) if it is intended for concurrent use and you can achieve significantly higher concurrency by synchronizing internally than you could by locking the entire object externally. Otherwise, don't synchronize internally. Let the client synchronize externally where it is appropriate. (eg. StringBuffer vs. StringBuilder)
-
If a method modifies a static field, you must synchronize access to this field, even if the method is typically used only by a single thread.
The failure by invoking alien method
/**
* Demo for "67 Avoid excessive synchronization".
*/
package com.effectivejava.concurrency;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import com.effectivejava.classinterface.ForwardingSet;
/**
* @author Kaibo Hao
*
*/
public class ObservableSet<E> extends ForwardingSet<E> {
/**
* @param s
*/
public ObservableSet(Set<E> s) {
super(s);
}
private final List<SetObserver<E>> observers = new ArrayList<SetObserver<E>>();
public void addObserver(SetObserver<E> observer) {
synchronized (observers) {
observers.add(observer);
}
}
public boolean removeObserver(SetObserver<E> observer) {
synchronized (observers) {
return observers.remove(observer);
}
}
private void notifyElementAdded(E element) {
synchronized (observers) {
for (SetObserver<E> observer : observers)
// calling the alien method
observer.added(this, element);
}
}
@Override
public boolean add(E element) {
boolean added = super.add(element);
if (added)
notifyElementAdded(element);
return added;
}
@Override
public boolean addAll(Collection<? extends E> c) {
boolean result = false;
for (E element : c)
result |= add(element); // calls notifyElementAdded
return result;
}
public static void main(String[] args) {
ObservableSet<Integer> set =
new ObservableSet<Integer>(new HashSet<Integer>());
set.addObserver(new SetObserver<Integer>() {
public void added(ObservableSet<Integer> s, Integer e) {
System.out.println(e);
}
});
for (int i = 0; i < 100; i++)
set.add(i);
}
}
Case 1: Failed to remove an element from a list in the midst of iterating over it, which is illegal.
Root Cause - The iteration in the notifyElementAdded method is in a synchronized block to prevent concurrent modification, but it doesn't prevent the iterating thread itself from calling back into the observable set and modifying its observers list.
// Removing the Observer during the iteration.
set.addObserver(new SetObserver<Integer>() {
public void added(ObservableSet<Integer> s, Integer e) {
System.out.println(e);
if (e == 23) s.removeObserver(this);
}
});
Case 2: Failed to background thread for removing.
Root Cause: - The object in the synchronized region are locked by the main thread which cannot be modified by the background thread.
// Observer that uses a background thread needlessly
set.addObserver(new SetObserver<Integer>() {
@Override
public void added(final ObservableSet<Integer> s, Integer e) {
System.out.println(e);
if (e == 23) {
ExecutorService executor = Executors
.newSingleThreadExecutor();
final SetObserver<Integer> observer = this;
try {
executor.submit(new Runnable() {
@Override
public void run() {
s.removeObserver(observer);
}
}).get();
} catch (ExecutionException ex) {
throw new AssertionError(ex.getCause());
} catch (InterruptedException ex) {
throw new AssertionError(ex.getCause());
} finally {
executor.shutdown();
}
}
}
});
Reentrant lock : Locks in Java programming language are reentrant, in other words such calls above won't deadlock.
Solution 1 - Taking snapshot and move Alien method outside of synchronized block - open calls
private void notifyElementAdded(E element) {
List<SetObserver<E>> snapshot = null;
synchronized(observers) {
snapshot = new ArrayList<SetObserver<E>>(observers);
}
for (SetObserver<E> observer : snapshot)
observer.added(this, element);
}
Solution 2(Prefered) - Thread-safe observable set with CopyOnWriteArrayList
// Thread-safe observable set with CopyOnWriteArrayList
private final List<SetObserver<E>> observers = new CopyOnWriteArrayList<SetObserver<E>>();
public void addObserver(SetObserver<E> observer) {
observers.add(observer);
}
public boolean removeObserver(SetObserver<E> observer) {
return observers.remove(observer);
}
private void notifyElementAdded(E element) {
for (SetObserver<E> observer : observers)
observer.added(this, element);
}
CopyOnWriteArrayList
It is a variant of ArrayList in which all write operations are implemented by making a fresh copy of the entire underlying array. Performance may be atrocious, but it's perfect for observer lists which are rarely modified and often traversed.
Note
In a multicore world, the real cost of excessive synchronization is not the CPU time spent obtaining locks; it is the lost opportunities for parallelism and the delays imposed by the need to ensure that every core has a consistent view of memory.
If you do synchronize your class internally, you can use various techniques to achieve high concurrency, such as lock splitting, lock striping, and nonblocking concurrency control.
Summary
To avoid deadlock and data corruption, never call an alien method from within a synchronized region. More generally, try to limit the amount of work that you do from within synchronized regions. When you are designing a mutable class, think about whether it should do its own synchronization. In the modern multicore era, it is more important than ever not to synchronize excessively. Synchronize your class internally only if there is a good reason to do so, and document your decision clearly (Item 70).