Why a lock should be a constant

Last week I read a post on the concurrency mailinglist about a piece of code from Apache Tomcat. The code was analyzed with Findbugs by Bill Pough (one of the inventors) and it is riddled with concurrency errors.

private InstanceListener[] listeners = new InstanceListener[0];

public void addInstanceListener(InstanceListener listener) {

        synchronized (listeners) {

            InstanceListener[] results = new InstanceListener[listeners.length + 1];

            for (int i = 0; i < listeners.length; i++) results[i] = listeners[i];

            results[listeners.length] = listener;

            listeners = results;

        }

}

Apart from a missing happens-before relation on the write and read of the listeners array causing visibility problems, and the same goes for the content of the array (piggy backing on synchronization won’t work), there are serious ‘old school’ concurrency problems as well.

Imagine what happens when the lock on the monitor of listeners array is obtained by one thread. When another thread wants to obtain the lock, it is blocked because the lock is not available. In the meantime the first thread continuous en replaced the listeners array by a new one (containing all previous InstanceListeners and the new one). When this thread completes, it releases the lock. The blocked thread obtains the lock on the monitor of the stale listeners array and starts working on the new listeners array without having a lock on it.

These problems could have been prevented by:

1. using a constant Object reference for a lock
2. using the CopyOnWriteArrayList (the CopyOnWriteArraySet maybe would be a better performing alternative if the order of the listeners doesn’t matter)