Should I synchronize listener notifications, or not?

Question

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!

Kiowa

use synchronizedList. For notifications, the synchronized block around the loop is good. – Jessie

The CopyOnWriteArrayList is appropriate for situations when traversal is much more common than updating. – Mariannmarianna

Please read my edit. It's about the lock, not about the container. – Kiowa

Writing this by hand when the j.u.c toolbox is to hand is asking for trouble. The toolkit provides not only code, but also standard behavioral conventions that are likely to be familiar to your clients. – Trumantrumann

-1 You're asking a loaded question. You're assuming that you need to use a lock and asking if it should be exposed. The proper answer is that you shouldn't need a lock, and several options to do this have been presented. – Pharmacology

@ErickRobertson No he's not. It's a perfectly fine question. – Cleaves

Okay, I'm talking to @JaperD in chat, and he basically wanted to ask a different question. If you want to answer this one, or close it, whatever. It's too late to change this question, I feel. chat.stackoverflow.com/transcript/message/1946316#1946316 – Pharmacology

Sorry, it's my English! I indeed wanted to ask a different question. I did not want to focus on the data structure, but on the idea of getting events while already being unregistered. I tried again on #8260705 . – Kiowa

You should use the second option, but for a different reason: you want the listener to be able to unregister itself, or register another listener, during its own execution. – Stratum

@Stratum That's besides the point. You can create a copy of the list and traverse the copy inside the synchronized block too. And besides, the already suggested CopyOnWriteArrayList would solve your suggested problem without reinventing the wheel. – Eunuchoidism

Answer 1

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();
}

Pharmacology

If it's vital that you never get another event call once the listener has been removed, then you'll either need to use the first code sample or set a flag in the listener to ignore the event. But if you do this, it really complicates your addListener and removeListener methods, because you will have to detect if this method is being called from within your event() method on a listener. If you use the CopyOnWriteArrayList, then you don't have to synchronize when iterating. This invalidates your question. – Pharmacology

Thanks, makes sense. By the way, CopyOnWriteArrayList = my second option, conceptually wise. – Kiowa

@ErickRobertson "Note you should still synchronize the add & remove operations." I would have thought not. Can you explain? – Lifesaving

@Lifesaving I stand corrected. CopyOnWriteArrayList is thread-safe, so there is no need to synchronize. I will update my answer. Thanks! – Pharmacology

Answer 2

I would use a ConcurrentLinkedQueue<Listener> which is made for this kind of problems: adding, removing and iterating simultaneously on a collection.

A precision : this solution prevents a listener from being called from the very moment it is deregistered. This solution has the best accuracy, the finest granularity, and is probably the solution that is the less deadlock-prone.

If you're adamant about your two proposals, I would choose the first one because it is more secure, but it's likely to provoke longer locks and reduce overall performance (well it depends on how often you add or remove listeners). The second solution is broken because when a listener deregisters itself, it might well be because he's become unable to handle events. In such a case, calling it would be a very bad idea, and in any case it would be a violation of the listener contract.

Authors

For my question, that's just syntax sugar. The question here is whether you would notify the listeners outside or inside the synchronization lock. – Kiowa

Not sugar, because in this case there's no lock: the queue has atomic locks on add and remove and the iterator remains consistant. It means that you start iterating, an element is added, it will be called, another is removed, it won't, all this in the same iteration cycle. It's the best of both world: ensuring synchronization, while being fine grained on who gets called and who's not. – Authors

I see. That solution is the same as my second option. There, a listener can get a notification while it might already been unregistered. – Kiowa

No again :) Because if your listener deregister itself while the loop is being done and the loop has not yet notified it, it will never be notified. Another way to put it: the granularity of your code is collection-wide (Q is whether to record collection before notifying (fast, innacurate) or preventing collection modif while notifying (slow because of collection-wide lock)). The Concurrent stuff has a listener granularity : you don't lock your collection, yet you take collection modifications into account while your loop is iterating ! – Authors

Updated my answer with an analysis of the two solutions you propose. – Authors

I get it now solendil. Thanks, this answer is spot on! – Kiowa

Note that this is a great answer if you're adding and removing listeners constantly. Typically a listener is being added at startup and removed rarely. In this case, the full thread protections of ConcurrentLinkedQueue are not required and a CopyOnWriteArrayList is appropriate. – Pharmacology

The analysis is incorrect, as removal of listener may happen after its retrieval during iteration but before its execution. – Infancy

Answer 3

I guess I would choose the second option too. Like I think you said, the second option doesn't hold the lock when it's notifying the listeners. So, if one of the listeners takes a long time to do its thing, other threads can still call the addListener or removeListener methods without having to wait for the lock to be released.

Cleaves

So it's up to the listener implementations to filter late-coming notifications? – Kiowa

@Japer D. So the problem is: if an event causes the listeners to fire, what happens if a thread wants to add or remove a listener while it's iterating through the list of listeners? I would just say: tough luck. The change that was made to the list of listeners won't be "recognized" until the next time an event occurs. I mean, I don't know how else to handle it. – Cleaves

No, it's more simple than that. If a thread calls removeListener(himself), he still can get a notificaiton, since the copy of the listeners might have been made before, in the second option. In the first option, that's not possible, but you risk introducing deadlocks since you expose the lock. – Kiowa

@JaperD. Exactly, I agree. :) But I don't think that's a big deal (getting one more notification after calling removeListener(himself)) – Cleaves

Answer 4

there are several datastructures available that allow concurrent adding, removing and iterating

a ConcurrentLinkedQueue is fully lockless and fast on add and remove (bar the O(n) traversel to find it) and add, but there may be some interference of other threads (a bulk remove may only be partially visible to the iterator)

the copyOnWrite list and set are slower on add and remove because they entail a array allocation and copy, however the iteration is completely free of interference and as fast as traversing a ArrayList of the same size (the iteration happens on a snapshot of the set)

you can build a ConcurrentHashSet on the ConcurrentHashMap but this has the same (usefull)properties besides a fast O(1) remove and the locks used for adding and removing

Dunham