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)
Advertisements

6 Responses to Why a lock should be a constant

  1. anjan bacchu says:

    hi there,

    nice/cool post.

    “concurrency mailinglist ” — can you give a link to this ? I’d like to join. I’m sure other readers would be, too.

    Thank you,

    BR,
    ~A

  2. Mathias Ricken says:

    Hi Peter:

    Could you please let me know how I can subscribe to that concurrency mailing list?
    Thanks!

    –Mathias

  3. ounos says:

    Cool bug.
    I saw the link in java.blogs and was just sure, from the title, that it referred to the (excellent) concurrency list 🙂

  4. peter lawrey says:

    If you have a large number of events the cost of creating an iterator and iteration may be important.

    One option is to have a list and an array.
    Use the list to create the array in a synchronised block when a listener is added/removed and iterate over a local variable holding the array (un synchronised) when calling each listener.
    This avoids locking and the cost of an iterator for each event.

  5. Nirav Thaker says:

    Nice, It’s always a good/efficient idea to lock immutable objects.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: