Should I synchronize listener notifications, or not?

I am always very hesitant to bring my locks in the open, to make them public. I always try to keep the locks restricted to my implementation. Not doing that, is a recipe for deadlocks, I believe.

I have the following class:

class SomeClass {
    protected ArrayList<Listener> mListeners = new ArrayList<Listener>();

    protected void addListener(Listener listener) {
        synchronized (mListeners) {
            mListeners.add(listener);
        }
    }

    protected void removeListener(Listener listener) {
        synchronized (mListeners) {
            mListeners.remove(listener);
        }
    }

    ...
}

When SomeClass wants to notify his listeners, would you do:

    synchronized (mListeners) {
        for (Listener l : mListeners) {
             l.event();
        }
    }

or

    Listener[] listeners = null;
            
    synchronized (mListeners) {
        listeners = mListeners.toArray();
    }
    for (Listener l : listeners) {
        l.event();
    }

I would choose the second option. The downside is that listeners can get events, even though they are already unregistered. The upside is that a thread, on which a listener calllback is waiting, cannot run into a deadlock when he wants to unregister a listener. I believe the upside is way more important than the downside, which can be easily documented.

So the question here is basically: would you expose your lock, or not?

My question is NOT if you would choose a plain ArrayList, a LinkedList, a ConcurrentLinkedQueue, a CopyOnWriteArrayList, a ...! It is whether you would mind if a listener can get a notification while it is already unregistered. It is whether you would bring the lock in the open, or not. It's about avoiding deadlocks, or not.

Please share your thoughts. Thanks!

Use a CopyOnWriteArrayList for your listener arrays.

This is perfect for listener arrays which change infrequently. When you iterate through them, you're iterating through the underlying array. With a CopyOnWriteArrayList, this array is copied every time it is modified. So there is no need to synchronize with it when iterating because each underlying array is guaranteed to be static, even past its use within the CopyOnWriteArrayList.

Since CopyOnWriteArrayList is also thread safe, you do not need to synchronize the add & remove operations.

Declaration:

private final CopyOnWriteArrayList<Listener> listeners;

Event trigger:

for (Listener l: this.listeners) {
  l.event();
}