ListenerList - changes in early 2024 compromise thread safety

General JUCE discussion
1

Hi JUCE team,

I recently struggled a deep bug in legacy code of a customer because of a change in ListenerList (commit in Jan 15 2024 b05b73fb490e4520df4799f26b54e5bca746d025) and I would like to bring it to your attention because I suspect that I won’t be the only victim.

ListenerList has the appearance of introducing a lock to make it thread-safe when you read in most methods: “const ScopedLockType lock (listeners->getLock());”

However it isn’t because the CriticalSection is disabled by default:

using ScopedLockType = typename ArrayType::ScopedLockType;
ArrayType = Array<ListenerClass*> // ListenerList default template argument
TypeOfCriticalSectionToUse = DummyCriticalSection // Array default template argument

I think that you will agree that this isn’t obvious to the first reader of the code and it may create misunderstandings.

On the other hand, before the Jan 15 commit, if add(ListenerClass* listenerToAdd) and remove(ListenerClass* listenerToAdd) were called at the construction and destruction of a class, without any concurrent calls to call(Callback&& callback), most code would get away with using ListenerList without CriticalSections and concurrent calls to call(Callback&& callback) were fine and without crashes as long as the functions invoked by the listeners were thread safe.

However, in the commit in Jan 15 b05b73fb490e4520df4799f26b54e5bca746d025, the following code was added to prevent edge cases (I quote the most critical bit but there are changes in several methods):

indices->push_back (&index);
const ScopeGuard scope { [i = indices, &index]
{
     i->erase (std::remove (i->begin(), i->end(), &index), i->end());
} };

for (const auto end = localListeners->size(); index < jmin (end, localListeners->size()); ++index)

As a result, if you were using the default ListenerList (without extra arguments to declare a CriticalSection), this started causing data runs in concurrent calls to call(Callback&& callback) that didn’t exist before.

In other words, those changes made ListenerList less thread safe and made old code crash because of those data runs.

As a result I think that those bits of code introduced to handle edge cases should be only run if the CriticalSection is defined. Here’s an idea for the code in the example but there are probably much better ways of doing it:

if (!std::is_same<ArrayType::ScopedLockType, DummyCriticalSection::ScopedLockType>())
{
  indices->push_back (&index);
  const ScopeGuard scope { [i = indices, &index]
  {
     i->erase (std::remove (i->begin(), i->end(), &index), i->end());
  } };
}

Alternatively you can add a CriticalSection to your legacy code but this will introduce locks in new places, and yes you guessed, in the code I had to fix those calls were in the audio thread! :man_facepalming:

1 Like
2

Thank you very much!! for bringing this up. I think I was writing comments about this before (or very soon after) this was merged and was turned down by “ListenerList never made any promises to be thread safe or real time safe etc.” which, although correct, I thought was a bad excuse to get a bug fixed/improvement out rather fast, than thinking about the impact this seemingly small change has on a lot of code.

I think what JUCE should have done here is commit to ListenerList being thread and real time safe (which it always was if you commit to some very basic rules that are very easily checked and understood by looking at the code) and create a new more complex ListenerList class and adding that to the ValueTree structure, what I believe was the main reason the recent changes to ListenerList were made in the first place.

3

I think there might be some confusion here.

The line of code claiming to cause the problem here isn’t simply about thread safety but about doing the right thing when listener callbacks try to mutate the ListenerList during a callback.

It’s worth noting that the use of a lock and a DummyCriticalSection were there prior to the commit in question, I don’t think that has any relevance and I don’t think we can special case based on that. Although I agree it’s confusing to users reading the code, it’s been that way for a long time however, we have since made improvements to the documentation.

It’s also worth noting that @Rincewind your concerns were raised in November 2023, several months before the commit under question here, that being said I think your basic point remains.

If I understand correctly the desired behaviour is for the following code to be both thread safe and lock free? essentially as if the call methods were marked const.

#include <JuceHeader.h>

int main()
{
    struct Listener
    {
        void callback()
        {
            const std::scoped_lock lock { mutex };
            std::cout << "Callback" << std::endl;
        }

        std::mutex mutex;
    };

    juce::ListenerList<Listener> listeners;
    Listener listener;
    listeners.add (&listener);

    {
        std::array<std::thread, 100> threads;
        std::atomic<int> numThreadsReady = 0;

        for (auto& thread : threads)
        {
            thread = std::thread {[&]
            {
                // just doing this to increase the odds of concurrent calls to listeners.call()
                ++numThreadsReady;
                while (numThreadsReady < threads.size()){}

                listeners.call (&Listener::callback);
            }};
        }

        for (auto& thread : threads)
            thread.join();
    }

    listeners.remove (&listener);
    return 0;
}

I’ve looked through the history and I think this hasn’t been the case since the release of JUCE 7 in June 2022.

I’ll take a look, but I certainly don’t want to make any promises! For now I just to make sure we’re in agreement as to the request being made.

4

Hi Anthony,

Thank you very much for looking at this.

My point was precisely that the class was previously written in a confusing manner and because of it, it would be used without CriticalSections (DummyCriticalSection) and people would assume it to be thread safe. I’m not saying it’s good practice (it’s terrible) but in practice many programmers write non-ideal/bad code and get away with it because it works …

Your changes are indeed doing the right thing but they assume that 1) There is a CriticalSection (which is not defined by default) or 2) There are no concurrent calls to the callback. What I mean is that the class as it was written previously would allow uses without CriticalSection with concurrent calls to the callback. So when you introduced this code, this created data runs and broke legacy code (at least the one I am looking at).

Yes. The patch that I suggested is only a temporary way to prevent silent crashes in legacy code (normally JUCE is really good at pointing you to bad practices with a jassert but not in this case). You can try it with your example code and you will see how it removes the crashes.

What I don’t know is if ListenerList can be thread safe and lock free for all kinds of scenarios.

5

Thank you, as always, very much for getting back to us!

For the sake of transparency: ListenerList crashes on AudioProcessor destruction - #41 by Rincewind
this is the comment we are talking about and it was also concerning a „missing“ garantee of ListenerList to be thread safe when only calling from multiple threads. And I believe this is the main point outlined by the OP.

I also think your conclusion is correct, that making call const would solve this problem, even though it never was const in the first place.
I’m making the case, that a lot of code used this function as if it were const (with all benefits of it) after looking at the implementation detail and noticing it would not be a problem for thread safety or real time threads.

EDIT: of course @anthony-nicholls your are correct from a programmers point of view. I’m just making the case for the legacy code base and beginners or people, that looked for thread safety and deemed it to be. Before the changes to ListenerList the past months, it was a very simple class IMO. That’s not the case anymore and that suprised at least me. I have been using the class a lot not needing all the safety rails.

6

I do have to say that the use of a lock in a generic class like Array is generally inappropriate and confusing (even though the default case resolves to no lock).

I would also assume most people and internal JUCE usages don’t know/care/want a lock in it, and instead would rather place an explicit lock in the use of the array only whenever needed.

I think it would be much better to create a breaking change that would separate Array and LockedArray, and force the (hopefully few) users of that feature to be explicit whenever they do want a lock.

3 Likes
7

I personally also have never used the array with an active lock (whenever I need to lock I normally have a realtime thread involved), but I would argue that the current approach leaves enough room to do just that with a type alias. And since the default is no lock, IMO the library is doing everything right here.

I think what I am missing now is a low footprint ListenerList. The original one was that (when I started using it) and now it’s seemingly doing more stuff, that is all useful, but not were I started using the class. And since the class has been around for some time I can’t believe I’m alone with that problem.
So an inheritance from LightweightListenerList with only add, remove, call, callExcluding without any guards what so ever would be my dream scenario.

8

You can already add a lock to ListenerList as follows:

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

I think you misunderstand me. I know that the Array type of listener list can be templated.

IMO having a lock as part of the interface of Array (which should be an equivalent class to std::vector) is confusing and unneeded, and shouldn’t be a part of such a widely used container IMO.

That lock adds quite a lot of complication to the class interface and implementation for a feature that’s rarely used. It’s an intrusive call that’s implemented on every single member function of Array - and does nothing on 99% of use cases.

I think it would be more correct to make Array be as similar as possible to std::vector. If it’s needed to have another, separate type that wraps and Array and adds a lock - I’m not against it but it should be a composable type and not a part of Array.

1 Like
10

In general I think most people here are confusing the use of the lock, taking the lock away will NOT change the functionality of the call function. The code is dealing with the single threaded use case that listeners may mutate the ListenerList in their callback in any of the following ways

  1. A new listener is added during a callback
  2. An existing listener in removed during a callback
  3. The ListenerList is cleared during a callback
  4. The ListenerList is destroyed during a callback

What is assumed however, is that non-const methods will not be called concurrently without using a mutex of some description, this could be managed externally to the class or internally by using ThreadSafeListenerList (which is just an alias for ListenerList <Listener, Array <Listener*, CriticalSection>>).

A very simple ListenerList class that doesn’t deal with any of these edge cases might look something like this (untested)…

template <typename ListenerClass>
class SimpleListenerList
{
public:
    void add (ListenerClass* listenerToAdd)
    {
        if (listenerToAdd == nullptr)
            return;

        listeners.addIfNotAlreadyThere (listenerToAdd);
    }

    void remove (ListenerClass* listenerToRemove)
    {
        listeners.removeFirstMatchingValue (listenerToRemove);
    }

    ErasedScopeGuard addScoped (ListenerClass& listenerToAdd)
    {
        add (&listenerToAdd);
        return ErasedScopeGuard { [this, &listenerToAdd] { remove (&listenerToAdd); } };
    }

    int size() const noexcept { return listeners.size(); }

    bool isEmpty() const noexcept { return listeners.isEmpty(); }

    void clear() { listeners.clear(); }

    bool contains (ListenerClass* listener) const noexcept { return listeners.contains (listener); }

    //==============================================================================
    template <typename Callback>
    void call (Callback&& callback) const
    {
        callCheckedExcluding (nullptr,
                              std::forward<Callback> (callback));
    }

    template <typename Callback>
    void callExcluding (ListenerClass* listenerToExclude, Callback&& callback) const
    {
        for (auto* listener : listeners)
            if (listener != listenerToExclude)
                callback (*listener);
    }

    //==============================================================================
    template <typename... MethodArgs, typename... Args>
    void call (void (ListenerClass::*callbackFunction) (MethodArgs...), Args&&... args) const
    {
        callCheckedExcluding (nullptr,
                              callbackFunction,
                              std::forward<Args> (args)...);
    }

    template <typename... MethodArgs, typename... Args>
    void callExcluding (ListenerClass* listenerToExclude,
                        void (ListenerClass::*callbackFunction) (MethodArgs...),
                        Args&&... args)
    {
        callExcluding (listenerToExclude, [&] (ListenerClass& l)
        {
            (l.*callbackFunction) (args...);
        });
    }

private:
    Array<ListenerClass*> listeners;
};
11

I think I’m missing something.

Won’t clear()

listeners->clear();

for (auto* it : *iterators)
    it->end = 0;

clash with callCheckedExcluding()

Iterator it{};
it.end = localListeners->size();
iterators->push_back (&it);

if there is no CriticalSection?

Isn’t ListenerList, as it is without a CriticalSection by default, assuming that all of its methods are called sequentially, not only the callbacks?

Wouldn’t the removal of the extra code in callCheckedExcluding() allow at least concurrent callbacks?

12

Even if you get rid of the iterators, clear() and callCheckedExcluding() can’t be called concurrently because one clears the list while the other iterates through the list! They are not atomic operations and so without a mutex they will likely cause a data race, this will have always been true for ListenerList, just as it is for juce::Array, std::vector, std::array, etc.

The point I’m trying to make however is that those iterators aren’t there to deal with any kind of threading issue they are there to deal with what happens when clear() (among other things) is called from a callback, therefore we can’t simply ignore them because there is a DummyCriticalSection.

The bit under question here is not add(), remove(), or clear() but the fact that callCheckedExcluding() now mutates the state of the ListenerList meaning you can’t make concurrent calls to callCheckedExcluding(). Prior to JUCE 7 it seems you could make concurrent calls to callCheckedExcluding() as long as none of the callbacks in turn mutated the sate of the ListenerList.

Yes but only in so much as any non-const member function does on any class. If you call any non-const member function on a class concurrently without a mutex then you will likely encounter a data race (unless maybe the type being mutated is atomic). It just happened that in the past callCheckedExcluding() was observably const so no data races occurred.

This is basically Hyrums Law

With a sufficient number of users of an API,
it does not matter what you promise in the contract:
all observable behaviors of your system
will be depended on by somebody.

So because you and others observed the behaviour that callCheckedExcluding() was const, you depended on it being const, but the contract was always non-const.

Yes but it would break the contract that clear() (among other things) can be called from a Listener callback.

For example take this code (untested)

struct Listener
{
    Listener (std::function<void()> callbackIn) callback (std::move (callbackIn)) {}
    std::function<void()> callback;
    void notify() { callback(); }
};

ListenerList<Listener> listeners;

Listener listener1 { [&]{ listeners.clear(); } };
Listener listener2 { []{ std::cout << "Hello, world!" << std::ends; } };

listeners.add (&listener1);
listeners.add (&listener2);

// the first callback should call `listeners.clear()` just once
// the second callback should be ignored
listeners.call (&Listener::notify);

If we remove the iterators and we naively loop through the list we risk UB. It’s effectively the same as something like this…

std::vector<int> numbers;
numbers.push_back (0);
numbers.push_back (1);
numbers.push_back (2);

for (auto number : numbers)
{
    std::cout << number << ": " << numbers.size() << std::endl;
    numbers.clear();
}

In the very specific case of clear() we could probably protect against that another way but we still need to protect against add(), remove(), and deleting the ListenerList itself. These protections are regarding callbacks calling these functions whilst the ListenerList is looping through the listeners on the same thread.

13

I was referring to concurrent calls to callCheckedExcluding() from different threads (from the processing thread and from the UI-message thread). Specifically:

listeners.call(function,arguments);

I think @Rincewind is doing the same. I understand perfectly that they were not supposed to but it would work before Jan 2024 so the code stayed in place…

Ok, now I understand the motivation of the changes. Was this requested by other users or a requirement for internal use of ListenerList in JUCE? If it was internal use, you could always create a new internal class with those requirements. If other users are requesting it, then well, yes, as you said, you can’t make everybody happy. In that case, probably creating a new class is anyway the best solution. I would argue (with my vote and @Rincewind’s) that if the old version allowed the assumption of const calls, this should be the one to stay (before that you could not call clear() inside a callCheckedExcluding()) however I don’t know how important or how many are the users who requested this other use of ListenerList :sweat_smile:.

2 Likes
14

Following this conversation we’ve added a LightweightListenerList class that can be used in place of a ListenerList where you know there won’t be mutations of the list during a callback.

Throughout most of the JUCE library we’ll probably continue to use ListenerList as the extra guarantees are useful as users can find themselves mutating the list from callbacks unknowingly.

There’s also some extra work here to try and catch misuse of the classes in order to encourage switching to a better suited version of the class.

I would suggest in many simple use cases the LightweightListenerList would make a good default, the jasserts should help too. We considered making this implementation the default but decided against the breaking change in the end.

3 Likes
15

This is excellent. Thanks! You are surely in the best position to decide if it had to be default or not. I would only argue that maybe the name of “ImmutableListenerList” would be more self-explanatory than LightweightListenerList.

16

Just curious, what’s a known case through which one might accidentally mutate the list from callbacks?

17

When the listener is removed from the list during the listener callback, ie when iterating the list.

1 Like
18

Thanks for this! I too was hitting the assertion after updating to JUCE 8 recently, and was wondering why I hadn’t had the issue before - which led me to this thread.

Long story short I’m trying to see how I can address it most easily, and the main option I’m considering is LightweightListenerList.

My problem however is that I cannot use it as it is in my code, because I’m using my own CustomValueSource class passed into the Value::Value (ValueSource* const v) constructor. My CustomValueSource inherits from Juce’s ValueSource. The issue here being that the Juce ValueSource base class doesn’t allow replacing the ListenerList with e.g. a Lightweight ditto, so I cannot use the LightweightListenerList in this case.

This is ancient code in my software, I don’t remember where I read that I should inherit from ValueSource, and if it’s something library users should be able to do, and not only Juce internals, but that’s where I’m at now.

Do you have any tips on how I can get the LightweightListenerList into my CustomValueSource, or how I can work around it?