Thread-Safety of ListenerList

Hello,
i just discovered a thread-safety issue in the juce::ListenerList, in JUCE 5.4.7.
The issue is the following: The call to juce::ListenerList::call is guarded by a mutex:

template <typename Callback>
void callExcluding (ListenerClass* listenerToExclude, Callback&& callback)
{
    typename ArrayType::ScopedLockType lock (listeners.getLock());

    for (Iterator<DummyBailOutChecker, ThisType> iter (*this); iter.next();)
    {
        auto* l = iter.getListener();

        if (l != listenerToExclude)
            callback (*l);
    }
}

However, the calls to ListenerList::add and ListenerList::remove are not!

void add (ListenerClass* listenerToAdd)
{
    if (listenerToAdd != nullptr)
        listeners.addIfNotAlreadyThere (listenerToAdd);
    else
        jassertfalse;  // Listeners can't be null pointers!
}

/** Removes a listener from the list.
     If the listener wasn't in the list, this has no effect.
*/
void remove (ListenerClass* listenerToRemove)
{
   jassert (listenerToRemove != nullptr); // Listeners can't be null pointers!
   listeners.removeFirstMatchingValue (listenerToRemove);
}

While the underlaying Array that holds the actual listeners is locked, but because the ListenerList::call builds a loop around the array, the size of the array can change at any point during looping.
The trivial solution would be to also lock the mutex on ListenerList::add, ListenerList::remove and `ListenerList::clear’:

void add (ListenerClass* listenerToAdd)
{
    typename ArrayType::ScopedLockType lock (listeners.getLock());
    if (listenerToAdd != nullptr)
        listeners.addIfNotAlreadyThere (listenerToAdd);
    else
        jassertfalse;  // Listeners can't be null pointers!
}
.... 
void remove (ListenerClass* listenerToRemove)
{
    typename ArrayType::ScopedLockType lock (listeners.getLock());
    jassert (listenerToRemove != nullptr); // Listeners can't be null pointers!
    listeners.removeFirstMatchingValue (listenerToRemove);
}
....
void clear()
{
    typename ArrayType::ScopedLockType lock (listeners.getLock());
    listeners.clear();
}

Any thoughts on this?
Best, Stephan

You’re missing the fact that the listeners array that is being used internally is of type juce::Array, which takes a CriticalSection type as a generic parameter. This CriticalSection is used to guard calls to juce::Array::addIfNotAlreadyThere and juce::Array::removeFirstMatchingValue. Therefore, these functions are thread-safe.

In fact, if you look closely, this CriticalSection is the one that’s being used to guard the call to juce::ListenerList::call!

Beware though:
By default, juce::ListenerList uses a juce::Array<ListenerClass*>, which in turn defaults to using a juce::DummyCriticalSection. This DummyCriticalSection doesn’t actually do any locking, so by default, calls to juce::ListenerList::call aren’t actually thread-safe!

If you need your ListenerList to be thread-safe, use a juce::ListenerList<YourListenerClass, juce::Array<YourListenerClass*, juce::CriticalSection>>. Have fun! :blush:

1 Like

Oh ok i see! Well, i encountered this problem on the ListenerList in AudioProcessorValueTreeState.

There in the private section the Parameter-Listener-List is declared as

...
private:    
...
    RangedAudioParameter& parameter;
    ListenerList<Listener> listeners;
    ...

So shouldn’t this be changed to:

...
ListenerList<Listener, Array<Listener*, CriticalSection>> listeners;
...

Otherwise the whole listener-handling of the APVTS is not thread safe, right?

You usually create the APVTS and it’s parameters in the processor constructor (run in the message thread only). So why the listener list needs to be thread safe?

While it is true that a APVTS with it’s parameters is constructed once in the processor constructor, adding/deleting parameter listeners after construction should not be related to this and obviously allowed by the APVTS interface – or did I get you wrong :thinking:

I just realized that we are talking about two different things here.
It is true that it is in fact possible to use the juce::Array inside the ListenerList with the critical section, so the modification of the underlying array would be thread-safe.
However, the ListenerList::call uses a second, different and completely unrelated mutex. This means that it is still possible to add a listener to the array, while the array is modified using ListenerList::remove.

Exactly, the APVTS is only constructed once, but listeners may (de-)register at any point in time. In our case this happens on opening/closing the editor.
Consider this case:
The Editor is registered as a listener to a APVTS::Parameter. The parameter is changed by automation (so from a different thread than the MessageThread) and the ListenerList::call is executed. While this is being done, the editor is closed on the message thread, calls ListenerList::remove. This will definitely result in an array-out-of-bounds.

However, the ListenerList::call uses a second, different and completely unrelated mutex.

Where are you getting that from? It is quite literally calling listeners.getLock(), so it’s using the same mutex.

You’re wrong on that one - the listeners are always called from the message thread. If you looks at the APVTS code, the tree’s values are updated in the timerCallback method, which (via call to ParameterAdapter::flushToTree) uses atomics to load the values from the audio thread into the APVTS tree.

In general, the APVTS is built for usage only in the message thread.

True- i didn’t see that. Still- then the AVPTS should not use a Dummy-CriticalSection, but a real one.
Attached you can see a case where exactly the situation as described before occured, triggered by pluginval.
On the left you have the message-thread, reacting to some event and deregistering a listener from the AudioProcessorParameter ListenerList.
On the right you have the processing thread, which via automation changed a parameter-value and on the same thread iterates through the listeners. In this case the plugin crashes, because the right thread tries to access the last element of the ListenerList, which was just removed by the message-thread.

Yup, looking at ParameterAdapter::parameterValueChanged it indeed looks like that just calls the listeners from whatever thread the parameter is changed on (most hosts probably use the audio thread for that).

However, using a proper juce::CriticalSection inside ListenerList::call wouldn’t be a good idea, because you must never under any circumstances acquire a lock on the audio thread to ensure realtime-safety!

It’d be nice to get a JUCE dev’s opinion on this matter, because I don’t see an easy fix for this. @reuk I see you recently made improvements to the APVTS’s thread-safety, can you give us some insight here?

1 Like

I agree, it’s probably best not to add/remove listeners on parameters outside of the AudioProcessor constructor + destructor. I also don’t think there’s an easy one-size-fits-all fix. The AudioProcessorParameters themselves protect their internal ListenerLists with a normal CriticalSection which is thread-safe but not necessarily realtime-safe. We could do something similar in the APVTS to make it more thread-safe, but this may have an impact on realtime safety.

A different ‘opt-in’ solution would be to connect all of your parameters (in the AudioProcessor constructor) to some kind of ‘broadcaster’ class. When any parameter changes, use a wait-free queue or an AsyncUpdater to transfer the update onto the message thread, and then use a second ListenerList to re-broadcast the update message on the message thread. Adding/removing listeners on this second ListenerList will be thread-safe and realtime-safe, as long as all the additions+removals happen on the message thread too.

2 Likes

I agree with your proposed solutions, but I think this thread-unsafe behaviour should be reflected in the documentation to avoid confusion.

I think using an AsyncUpdater for this is the best solution if adding/removing listeners dynamically is absolutely necessary.

Ah well- while the AudioProcessorParameter uses a CriticalSection for listener-access, the AudioProcessorValueTreeState::ParameterAdapter does not! However, for e.g. ButtonAttachment's ect. the adapter is used.

I like this solution- it makes it absolutely clear on which thread the message is processed.

But still- this makes the whole ButtonAttachment ect. non-thread-safe, since the callback for the parameter-change will be executed on the processing thread, directly interacting with the gui elements. This should definitely be stated in the documentation! In my opinion there should be a definitive guide on how to properly use the messaging/listener system in JUCE.

Beware that AsyncUpdater can cause the run-time allocation (at least on some platform) of the message to be posted on the message queue, therefore it is not strictly realtime safe: although you may get away with it on any reasonable Desktop setup, that may become a problem in embedded contexts like the Elk Audio OS, AFAIK

The ParameterAttachment classes are AsyncUpdaters, and only update linked UI elements in their constructors/destructors, and within handleAsyncUpdate. They also directly attach to parameters, bypassing the APVTS ListenerLists, so they shouldn’t be susceptible to the APVTS add/remove safety issues mentioned earlier in this thread.

All attachments derive from AttachedControlBase, which just adds itself as a listener to AudioProcessorValueTreeState::ParameterAdapter::listeners. Different class, same problem.

You mentioned earlier that its best not to add/remove listeners at runtime. How is that feasable for the creation/destruction of the editor? Each real-world application probably needs to add some listeners on runtime in this situation.

I see that this is a exotic case, but still I think that this is still a very fundamental issue, especially as attachments are the juce-proposed solution on how to connect the processor and editor.

I didn’t know that! Luckily, I’m using my own system for this, which is strictly realtime-safe on any architecture, because it uses an std::atomic_flag (which is required by the C++ spec to be lock-free).

Feel free to use this in your own code:

/**
 * Allows change messages to be scheduled realtime- and thread-safely.
 */
class RealtimeSafeChangeBroadcaster : public juce::ChangeBroadcaster,
                                      protected juce::Timer {
public:
	RealtimeSafeChangeBroadcaster() :
			notDirty ATOMIC_FLAG_INIT {

		notDirty.test_and_set();
		startTimerHz(60);
	}

	/**
	 * Schedules a change message to be sent on the message thread.
	 * This function is thread- and realtime-safe.
	 */
	void sendChangeMessageRealtimeSafely() {
		notDirty.clear();
	}

private:
	std::atomic_flag notDirty;

protected:
	void timerCallback() override {
		if (!notDirty.test_and_set()) {
			sendChangeMessage();
		}
	}
};

Adding the talk from @dave96 for background:

This has changed on the juce6 branch, where attachments listen to parameters directly.

I was talking specifically about adding/removing listeners to parameters, or the APVTS. I suggested earlier having a permanent ‘message-thread-only’ listener list which lives in the processor. Parameters are connected to trigger this ListenerList (on the message thread only) during the PluginProcessor constructor. Then, the editor (or really any GUI object) can connect/disconnect to this secondary listener list at any point without introducing thread-safety or realtime-safety issues.