Although I take your point that the ListenerList previously had no data races if two threads were only calling listeners at the same time. To the best of my knowledge the ListenerList has never been “thread safe”, has never claimed to be thread safe, and I doubt it was ever intended to be thread safe (unless you pass the second template argument with an ArrayType that uses a CriticalSection).
This change might significantly increase the chance of data races but I’m not sure it’s fair to call it a “breaking” change, no contracts were broken, I’m not aware this was ever a guarantee, it just happened to work that way, it’s a classic example of Hyrums law!
I suspect in this case the issue is a contention between the audio thread and the GUI thread. Probably both are trying to perform sets on a ValueTree? Unfortunately I couldn’t confirm this based on the shared logs as the stack from the audio thread “failed to restore”. If this is the case I would strongly recommend avoiding this.
Having a look it might be possible to restore the JUCE 6 behaviour in the current version, but I wouldn’t want to make any promises. That being said all this would do is hide what I believe is a user error. Any reliance on this not crashing is likely masking potentially more subtle issues that could be lurking in user code.
I also suspect if we mimic the JUCE 6 behaviour we will only be reducing the chance of a crash (albeit significantly) as it’s likely listeners are being added/removed when the editor is created/destroyed and the processor is running and trying to call all the listeners. Also imagine the same value on a ValueTree is set from both threads, the order in which the listeners receive the two calls would be indeterministic.
The real fix is for the user to ensure thread safety. I can think of at least three ways to do this…
Wrap all the calls in a mutex to prevent a data race (not a good idea if an audio thread is involved!)
Only use the ValueTree / ListenerList on a single thread, then use a different mechanism to synchronise the data across threads (personally this is the approach I would normally reach for)
Create separate ValueTree’s / ListenerList’s for each thread, so each one is single threaded. This should also make it clear which callbacks are occurring on the audio thread.
Point taken, and from a strictly “code-legal” point of view, your are absolutely right and the change was well within your bounds. I would argue however, that a considerable amount of people used ListenerList in a multi-threaded way anyhow after confirming, that it is save given the current state of the code.
I’m not saying that wasn’t also kind of stupid (the methods aren’t even marked const), but JUCE is pretty old and I think this kind of change should been marked more clearly or separated into a sub-class.
One of the fundamental design guidelines from C++ is “Don’t pay for what you don’t use” and if I know I’m not adding/removing listeners while iterating them, I shouldn’t pay for these checks. As this wasn’t guarded for years and million lines of code don’t relay on it being checked, I think this should be capsulated differently.
I inherit juce::Timer and use juce::startTimerHz(60); and have Timer::stopTimer(); in the deconstructor.
EDIT: the timer itself is just calling repaint(); which in turn calls my Play Markers function.
EDIT2: I just commented out the timer start/stop and still got a shutdown crash.
Reporting the exact same issue pointed by andym-2 here.
In my case the crash happens on closure once every ~50 times and is within this line (line 110 as of juce v7.0.8 on master) of juce_ListenerList.h when trying to remove a listener from a valid object
Could the timer problem be caused by the timer thread creating a timer message, then the destructor gets called, in which we call stopTimer, but since the message already exists in the queue, the listener is still called, but since that doesn’t exist anymore, the call leads to a crash?
The number #1 crash in our crash-logs (that customers supply!) is in the timerCallback. Stack trace is always during destruction of our editor.
I’ve got some internal changes I’m pretty positive they will take some time to make it through an MR though! My main goal is to try and detect and assert for possible issues early allowing us to communicate some possible solutions.
@fravezAt your use case sounds interesting, given you are already passing a criticalSection. I think the destructor of the ListenerList isn’t locking the arrays internal lock so I guess you are destroying the ListenerList while it’s iterating through the listeners, maybe try calling clear() on your listeners list before it is destroyed?
@reFX that’s interesting I’ve not experienced an issue like that before. I’ll see if I can do something to repro that.
Just an update on my situation, after discussion with Anthony, it looks like my issues were caused by an incorrect assumption that VT’s were thread safe. I was using a processor VT in the GUI to trigger listeners in the GUI from a mouseListener to send a message to an info display component. I’ve now replaced that with a dedicated VT for that purpose, which is obvious in hindsight really. I also now have 2 ValueTrees I use between the threads, each one being single direction like a motorway when writing.
The only other use was for a few .onClick() lambdas to set properties that triggered listeners/file loaders in the processor, but now I have a dedicated Send VT which I use instead.
I have done a bit of testing with the v7 code & it seems to be OK so far, but I’m still using the v6 for the moment until I have more time next week to do more tests.
Yeah the timer stuff seems independent - don’t want to hijack this thread but it is useful. I feel I understand the ListenerList issues now. In most cases I think it’s because it’s being used in multi-threaded cases and with some relatively minor changes I think most users should be able to prevent this. I think however we can improve diagnostics and there are a couple of very minor edge cases we could deal with better too. I’ll try and get something on develop to help with that as soon as possible.
I feel the timer issues are related because internally the timer class also uses a ListenerList and we have crashes in juce::CriticialSection when the timer class itself modifies the listener list.
Isn’t the timer just its own thread that wakes up periodically and sends a message through the MessageManager, which then calls the ListernList callbacks for that timer? What if the Timer thread changes the list while the MessageManager is iterating through it? What if the message is added to the queue, but before the listener list gets iterated, the listener gets destroyed?
To me this feels all connected. Juce internally doesn’t handle these ListernerLists right in the multi-threaded context it itself creates.
So it doesn’t matter if it’s a ValueTree, or Timer, or whatever else’s uses ListenerLists. If Juce doesn’t handle them right internally, then what hope does the user have?
It’s a good thought but unfortunately there isn’t a ListenerList in the Timer, there is one in Thread but this is only used to call exitSignalSent on any listeners and Timer (or any of the related classes) isn’t a Thread::Listener either. Unless I’m missing something?
So I’ve added some extra diagnostics and I plan to run it up on all the examples we have to see if anything triggers but when I had a little look around manually I didn’t spot anything obviously off with the way ListenerList is used internally. It absolutely could be but I haven’t found that to be the case yet.
I have since found three issues with the ListenerList itself…
If clear is called on the ListenerList during a callback then it will still try to iterate through all the listeners. No one seems to have reported this to the best of my knowledge so I’m assuming that’s not a common thing to do, but as we support removing listeners in callbacks I think we should supporting clearing the listeners too.
If a CriticalMutex is passed in and callbacks are firing while the destructor to the ListenerList is called there is still a potential data race. I think in most cases it’s best practice to signal that you want callbacks to stop and wait until it’s safe to destroy the object, however I think there are likely legitimate use cases where that will be difficult and non-obvious so protecting against this makes sense if we can.
If a CriticalMutex is passed in and one of the callbacks results in the destructor of the ListenerList itself, although the iterator will immediately bail preventing access to the listener array (that’s the point of the code in the destructor) it will then call the destructor of the scoped lock which will try to unlock the mutex which has been deleted. Although this is easy enough to reproduce I’m finding that it doesn’t crash for me and address sanitiser doesn’t pick it up! which is probably why it was missed.
I have fixes internally for all the above but they’ll need more testing and some team review.
I think most of the issues with ListenerList are just where it’s used in multiple threads and I think we can do better to diagnose and communicate how ListenerList should be used as I think it is relatively easy to do accidentally and not immediately obvious when it goes wrong (as is so often the case with threading issues). I have a proof of concept for this so hopefully that can make it in too.
We don’t do any of those things. We don’t add, remove, or clear any listeners in any callbacks. Ever. We only add them in our constructors and remove them in our destructors.
For the Timer issue it’s literally just a call to startTimeHz ( 60 ); in the constructor, and in the destructor we call stopTimer (). Nothing else.
Crashes during Timer creation/start in CriticialSection, which tries to modify some ReferenceCountedArray or during shutdown, when listeners are called during destruction, which may then points to objects which don’t exist anymore.
Happens way more frequently on Mac than on Windows. The CriticalSection issue occurs only on Mac.
It might well be but as far as I could tell it’s not literally using the ListerList class (maybe it should!), I haven’t looked properly yet.
I’m not saying you do, I was only referring to the literal use of the ListenerList class.
I think this just highlights that the Timer issue is a separate issue, even if the problem is very similar. The point is “fixing” (including any extra diagnostics I add) ListenerList isn’t likely to fix the Timer so it warrants a separate investigation that I’ll likely dive into almost immediately.
Personally I think a lot of the JUCE codebase that rely on on the message thread for correctness (ValueTree, ListenerList, etc) should assert as such. I think that’s already done in a lot of the Component methods anyway.
Alternatively you can store the thread ID during construction and assert if its not that thread in any other call. That will allow to run intentional operations in a side thread but not use multi threaded calls.