diff --git a/BREAKING_CHANGES.md b/BREAKING_CHANGES.md index 54e9ff450d..c583808231 100644 --- a/BREAKING_CHANGES.md +++ b/BREAKING_CHANGES.md @@ -2,6 +2,26 @@ # develop +## Change + +The ListenerList::Iterator class has been removed. + +**Possible Issues** + +Any code directly referencing the ListenerList::Iterator will fail to compile. + +**Workaround** + +In most cases there should be a public member function that does the required +job, for example, call, add, remove, or clear. In other cases you can access the +raw array of listeners to iterate through them by calling getListeners(). + +**Rationale** + +Iterating through the listeners using the ListenerList::Iterator could in a +number of cases lead to surprising results and undefined behavior. + + ## Change The background colour of the Toolbar::CustomisationDialog has been changed from diff --git a/modules/juce_core/containers/juce_ListenerList.cpp b/modules/juce_core/containers/juce_ListenerList.cpp index 3404ce889d..a88e00e9fb 100644 --- a/modules/juce_core/containers/juce_ListenerList.cpp +++ b/modules/juce_core/containers/juce_ListenerList.cpp @@ -300,6 +300,119 @@ public: expect (test.wereAllNonRemovedListenersCalled (numCalls)); } } + + beginTest ("Deleting the listener list from a callback"); + { + struct Listener + { + std::function onCallback; + void notify() { onCallback(); } + }; + + auto listeners = std::make_unique>(); + + const auto callback = [&] + { + expect (listeners != nullptr); + listeners.reset(); + }; + + Listener listener1 { callback }; + Listener listener2 { callback }; + + listeners->add (&listener1); + listeners->add (&listener2); + + listeners->call (&Listener::notify); + + expect (listeners == nullptr); + } + + beginTest ("Using a BailOutChecker"); + { + struct Listener + { + std::function onCallback; + void notify() { onCallback(); } + }; + + ListenerList listeners; + + bool listener1Called = false; + bool listener2Called = false; + bool listener3Called = false; + + Listener listener1 { [&]{ listener1Called = true; } }; + Listener listener2 { [&]{ listener2Called = true; } }; + Listener listener3 { [&]{ listener3Called = true; } }; + + listeners.add (&listener1); + listeners.add (&listener2); + listeners.add (&listener3); + + struct BailOutChecker + { + bool& bailOutBool; + bool shouldBailOut() const { return bailOutBool; } + }; + + BailOutChecker bailOutChecker { listener2Called }; + listeners.callChecked (bailOutChecker, &Listener::notify); + + expect ( listener1Called); + expect ( listener2Called); + expect (! listener3Called); + } + + beginTest ("Using a critical section"); + { + struct Listener + { + std::function onCallback; + void notify() { onCallback(); } + }; + + struct TestCriticalSection + { + TestCriticalSection() { isAlive() = true; } + ~TestCriticalSection() { isAlive() = false; } + + static void enter() noexcept { numOutOfScopeCalls() += isAlive() ? 0 : 1; } + static void exit() noexcept { numOutOfScopeCalls() += isAlive() ? 0 : 1; } + + static bool tryEnter() noexcept + { + numOutOfScopeCalls() += isAlive() ? 0 : 1; + return true; + } + + using ScopedLockType = GenericScopedLock; + + static bool& isAlive() + { + static bool inScope = false; + return inScope; + } + + static int& numOutOfScopeCalls() + { + static int numOutOfScopeCalls = 0; + return numOutOfScopeCalls; + } + }; + + auto listeners = std::make_unique>>(); + + const auto callback = [&]{ listeners.reset(); }; + + Listener listener { callback }; + + listeners->add (&listener); + listeners->call (&Listener::notify); + + expect (listeners == nullptr); + expect (TestCriticalSection::numOutOfScopeCalls() == 0); + } } private: diff --git a/modules/juce_core/containers/juce_ListenerList.h b/modules/juce_core/containers/juce_ListenerList.h index 2908f1c3da..0fedcbea09 100644 --- a/modules/juce_core/containers/juce_ListenerList.h +++ b/modules/juce_core/containers/juce_ListenerList.h @@ -25,11 +25,12 @@ namespace juce //============================================================================== /** - Holds a set of objects and can invoke a member function callback on each object - in the set with a single call. + Holds a set of objects and can invoke a member function callback on each + object in the set with a single call. Use a ListenerList to manage a set of objects which need a callback, and you - can invoke a member function by simply calling call() or callChecked(). + can invoke a member function by simply calling call(), callChecked(), or + callExcluding(). E.g. @code @@ -47,20 +48,30 @@ namespace juce listeners.call ([] (MyListenerType& l) { l.myCallbackMethod (1234, true); }); @endcode - It is guaranteed that every Listener is called during an iteration if it's inside the - ListenerList before the iteration starts and isn't removed until its end. This guarantee - holds even if some Listeners are removed or new ones are added during the iteration. + It is safe to add listeners, remove listeners, clear the listeners, and + delete the ListenerList itself during any listener callback. - Listeners added during an iteration are guaranteed to be not called in that iteration. + If a Listener is added during a callback, it is guaranteed not to be called + in the same iteration. - Sometimes, there's a chance that invoking one of the callbacks might result in the - list itself being deleted while it's still iterating - to survive this situation, you can - use callChecked() instead of call(), passing it a local object to act as a "BailOutChecker". - The BailOutChecker must implement a method of the form "bool shouldBailOut()", and - the list will check this after each callback to determine whether it should abort the - operation. For an example of a bail-out checker, see the Component::BailOutChecker class, - which can be used to check when a Component has been deleted. See also - ListenerList::DummyBailOutChecker, which is a dummy checker that always returns false. + If a Listener is removed during a callback, it is guaranteed not to be + called if it hasn't already been called. + + If the ListenerList is cleared or deleted during a callback, it is + guaranteed that no more listeners will be called. + + By default a ListenerList is not thread safe. If thread-safety is required, + you can provide a thread-safe Array type as the second type parameter e.g. + @code + using ThreadSafeList = ListenerList>; + @endcode + + When calling listeners the iteration can be escaped early by using a + "BailOutChecker". A BailOutChecker is a type that has a public member function + with the following signature: + @code bool shouldBailOut() const @endcode + This function will be called before making a call to each listener. + For an example see the DummyBailOutChecker. @tags{Core} */ @@ -74,44 +85,42 @@ public: ListenerList() = default; /** Destructor. */ - ~ListenerList() - { - WrappedIterator::forEach (activeIterators, [&] (auto& iter) - { - iter.invalidate(); - }); - } + ~ListenerList() { clear(); } //============================================================================== /** Adds a listener to the list. A listener can only be added once, so if the listener is already in the list, this method has no effect. + + If a Listener is added during a callback, it is guaranteed not to be called + in the same iteration. + @see remove */ void add (ListenerClass* listenerToAdd) { if (listenerToAdd != nullptr) - listeners.addIfNotAlreadyThere (listenerToAdd); + listeners->addIfNotAlreadyThere (listenerToAdd); else - jassertfalse; // Listeners can't be null pointers! + 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. + + If a Listener is removed during a callback, it is guaranteed not to be + called if it hasn't already been called. */ void remove (ListenerClass* listenerToRemove) { jassert (listenerToRemove != nullptr); // Listeners can't be null pointers! - typename ArrayType::ScopedLockType lock (listeners.getLock()); + const ScopedLockType lock (listeners->getLock()); - const auto index = listeners.removeFirstMatchingValue (listenerToRemove); - - WrappedIterator::forEach (activeIterators, [&] (auto& iter) - { - if (0 <= index && index < iter.get().index) - --iter.get().index; - }); + if (const auto index = listeners->removeFirstMatchingValue (listenerToRemove); index >= 0) + for (auto* indexPtr : *indices) + if (index <= *indexPtr) + --*indexPtr; } /** Adds a listener that will be automatically removed again when the Guard is destroyed. @@ -127,250 +136,188 @@ public: } /** Returns the number of registered listeners. */ - int size() const noexcept { return listeners.size(); } + int size() const noexcept { return listeners->size(); } /** Returns true if no listeners are registered, false otherwise. */ - bool isEmpty() const noexcept { return listeners.isEmpty(); } + bool isEmpty() const noexcept { return listeners->isEmpty(); } - /** Clears the list. */ - void clear() { listeners.clear(); } + /** Clears the list. + + If the ListenerList is cleared during a callback, it is guaranteed that + no more listeners will be called. + */ + void clear() { listeners->clear(); } /** Returns true if the specified listener has been added to the list. */ - bool contains (ListenerClass* listener) const noexcept { return listeners.contains (listener); } + bool contains (ListenerClass* listener) const noexcept { return listeners->contains (listener); } - /** Returns the raw array of listeners. */ - const ArrayType& getListeners() const noexcept { return listeners; } + /** Returns the raw array of listeners. + + Any attempt to mutate the array may result in undefined behaviour. + + If the array uses a mutex/CriticalSection, reading from the array without first + obtaining the lock may potentially result in undefined behaviour. + + @see add, remove, clear, contains + */ + const ArrayType& getListeners() const noexcept { return *listeners; } //============================================================================== - /** Calls a member function on each listener in the list, with multiple parameters. */ + /** Calls an invokable object for each listener in the list. */ template void call (Callback&& callback) { - typename ArrayType::ScopedLockType lock (listeners.getLock()); - - for (WrappedIterator iter (*this, activeIterators); iter.get().next();) - callback (*iter.get().getListener()); + callCheckedExcluding (nullptr, + DummyBailOutChecker{}, + std::forward (callback)); } - /** Calls a member function with 1 parameter, on all but the specified listener in the list. - This can be useful if the caller is also a listener and needs to exclude itself. + /** Calls an invokable object for each listener in the list, except for the + listener specified by listenerToExclude. */ template void callExcluding (ListenerClass* listenerToExclude, Callback&& callback) { - typename ArrayType::ScopedLockType lock (listeners.getLock()); + callCheckedExcluding (listenerToExclude, + DummyBailOutChecker{}, + std::forward (callback)); - for (WrappedIterator iter (*this, activeIterators); iter.get().next();) - { - auto* l = iter.get().getListener(); - - if (l != listenerToExclude) - callback (*l); - } } - /** Calls a member function on each listener in the list, with 1 parameter and a bail-out-checker. + /** Calls an invokable object for each listener in the list, additionally + checking the bail-out checker before each call. + See the class description for info about writing a bail-out checker. */ template void callChecked (const BailOutCheckerType& bailOutChecker, Callback&& callback) { - typename ArrayType::ScopedLockType lock (listeners.getLock()); - - for (WrappedIterator iter (*this, activeIterators); iter.get().next (bailOutChecker);) - { - callback (*iter.get().getListener()); - } + callCheckedExcluding (nullptr, + bailOutChecker, + std::forward (callback)); } - /** Calls a member function, with 1 parameter, on all but the specified listener in the list - with a bail-out-checker. This can be useful if the caller is also a listener and needs to - exclude itself. See the class description for info about writing a bail-out checker. + /** Calls an invokable object for each listener in the list, except for the + listener specified by listenerToExclude, additionally checking the + bail-out checker before each call. + + See the class description for info about writing a bail-out checker. */ template void callCheckedExcluding (ListenerClass* listenerToExclude, const BailOutCheckerType& bailOutChecker, Callback&& callback) { - typename ArrayType::ScopedLockType lock (listeners.getLock()); + const auto localListeners = listeners; + const ScopedLockType lock { localListeners->getLock() }; - for (WrappedIterator iter (*this, activeIterators); iter.get().next (bailOutChecker);) + int index{}; + + indices->push_back (&index); + const ScopeGuard scope { [i = indices, &index] { - auto* l = iter.get().getListener(); + i->erase (std::remove (i->begin(), i->end(), &index), i->end()); + } }; - if (l != listenerToExclude) - callback (*l); + for (const auto end = localListeners->size(); index < jmin (end, localListeners->size()); ++index) + { + if (bailOutChecker.shouldBailOut()) + return; + + auto* listener = localListeners->getUnchecked (index); + + if (listener == listenerToExclude) + continue; + + callback (*listener); } } //============================================================================== - /** A dummy bail-out checker that always returns false. - See the ListenerList notes for more info about bail-out checkers. - */ - struct DummyBailOutChecker - { - bool shouldBailOut() const noexcept { return false; } - }; - - using ThisType = ListenerList; - using ListenerType = ListenerClass; - - //============================================================================== - /** Iterates the listeners in a ListenerList. */ - struct Iterator - { - explicit Iterator (const ListenerList& listToIterate) noexcept - : list (listToIterate), index (listToIterate.size()) - {} - - //============================================================================== - bool next() noexcept - { - if (index <= 0) - return false; - - auto listSize = list.size(); - - if (--index < listSize) - return true; - - index = listSize - 1; - return index >= 0; - } - - template - bool next (const BailOutCheckerType& bailOutChecker) noexcept - { - return (! bailOutChecker.shouldBailOut()) && next(); - } - - ListenerClass* getListener() const noexcept - { - return list.getListeners().getUnchecked (index); - } - - private: - const ListenerList& list; - int index; - - friend ListenerList; - - JUCE_DECLARE_NON_COPYABLE (Iterator) - }; - - //============================================================================== - #ifndef DOXYGEN - void call (void (ListenerClass::*callbackFunction)()) - { - call ([=] (ListenerClass& l) { (l.*callbackFunction)(); }); - } - - void callExcluding (ListenerClass* listenerToExclude, void (ListenerClass::*callbackFunction)()) - { - callExcluding (listenerToExclude, [=] (ListenerClass& l) { (l.*callbackFunction)(); }); - } - - template - void callChecked (const BailOutCheckerType& bailOutChecker, void (ListenerClass::*callbackFunction)()) - { - callChecked (bailOutChecker, [=] (ListenerClass& l) { (l.*callbackFunction)(); }); - } - - template - void callCheckedExcluding (ListenerClass* listenerToExclude, - const BailOutCheckerType& bailOutChecker, - void (ListenerClass::*callbackFunction)()) - { - callCheckedExcluding (listenerToExclude, bailOutChecker, [=] (ListenerClass& l) { (l.*callbackFunction)(); }); - } - + /** Calls a specific listener method for each listener in the list. */ template void call (void (ListenerClass::*callbackFunction) (MethodArgs...), Args&&... args) { - typename ArrayType::ScopedLockType lock (listeners.getLock()); - - for (Iterator iter (*this); iter.next();) - (iter.getListener()->*callbackFunction) (static_cast::type> (args)...); + callCheckedExcluding (nullptr, + DummyBailOutChecker{}, + callbackFunction, + std::forward (args)...); } + /** Calls a specific listener method for each listener in the list, except + for the listener specified by listenerToExclude. + */ template void callExcluding (ListenerClass* listenerToExclude, void (ListenerClass::*callbackFunction) (MethodArgs...), Args&&... args) { - typename ArrayType::ScopedLockType lock (listeners.getLock()); - - for (Iterator iter (*this); iter.next();) - if (iter.getListener() != listenerToExclude) - (iter.getListener()->*callbackFunction) (static_cast::type> (args)...); + callCheckedExcluding (listenerToExclude, + DummyBailOutChecker{}, + callbackFunction, + std::forward (args)...); } + /** Calls a specific listener method for each listener in the list, + additionally checking the bail-out checker before each call. + + See the class description for info about writing a bail-out checker. + */ template void callChecked (const BailOutCheckerType& bailOutChecker, void (ListenerClass::*callbackFunction) (MethodArgs...), Args&&... args) { - typename ArrayType::ScopedLockType lock (listeners.getLock()); - - for (Iterator iter (*this); iter.next (bailOutChecker);) - (iter.getListener()->*callbackFunction) (static_cast::type> (args)...); + callCheckedExcluding (nullptr, + bailOutChecker, + callbackFunction, + std::forward (args)...); } + /** Calls a specific listener method for each listener in the list, except + for the listener specified by listenerToExclude, additionally checking + the bail-out checker before each call. + + See the class description for info about writing a bail-out checker. + */ template void callCheckedExcluding (ListenerClass* listenerToExclude, const BailOutCheckerType& bailOutChecker, void (ListenerClass::*callbackFunction) (MethodArgs...), Args&&... args) { - typename ArrayType::ScopedLockType lock (listeners.getLock()); - - for (Iterator iter (*this); iter.next (bailOutChecker);) - if (iter.getListener() != listenerToExclude) - (iter.getListener()->*callbackFunction) (static_cast::type> (args)...); + callCheckedExcluding (listenerToExclude, bailOutChecker, [&] (ListenerClass& l) + { + (l.*callbackFunction) (args...); + }); } - #endif -private: - class WrappedIterator + //============================================================================== + /** A dummy bail-out checker that always returns false. + See the class description for info about writing a bail-out checker. + */ + struct DummyBailOutChecker { - public: - WrappedIterator (const ListenerList& listToIterate, WrappedIterator*& listHeadIn) - : it (listToIterate), listHead (listHeadIn), next (listHead) - { - // GCC 12.2 with O1 and above gets confused here - JUCE_BEGIN_IGNORE_WARNINGS_GCC_LIKE ("-Wdangling-pointer") - listHead = this; - JUCE_END_IGNORE_WARNINGS_GCC_LIKE - } - - ~WrappedIterator() - { - if (valid) - listHead = next; - } - - auto& get() noexcept { return it; } - - template - static void forEach (WrappedIterator* wrapped, Callback&& cb) - { - for (auto* p = wrapped; p != nullptr; p = p->next) - cb (*p); - } - - void invalidate() noexcept { valid = false; } - - private: - Iterator it; - WrappedIterator*& listHead; - WrappedIterator* next = nullptr; - bool valid = true; + constexpr bool shouldBailOut() const noexcept { return false; } }; //============================================================================== - ArrayType listeners; - WrappedIterator* activeIterators = nullptr; + using ThisType = ListenerList; + using ListenerType = ListenerClass; +private: + //============================================================================== + using ScopedLockType = typename ArrayType::ScopedLockType; + + //============================================================================== + using SharedListeners = std::shared_ptr; + const SharedListeners listeners = std::make_shared(); + + using SafeIndices = std::vector; + using SharedIndices = std::shared_ptr; + const SharedIndices indices = std::make_shared(); + + //============================================================================== JUCE_DECLARE_NON_COPYABLE (ListenerList) };