ListenerList: Modify iterator during removals to guarantee callback

modules/juce_core/containers/juce_Array.h

@@ -829,9 +829,10 @@ public:
         If the item isn't found, no action is taken.
 
         @param valueToRemove   the object to try to remove
+        @returns               the index of the removed item, or -1 if the item isn't found
         @see remove, removeRange, removeIf
     */
-    void removeFirstMatchingValue (ParameterType valueToRemove)
+    int removeFirstMatchingValue (ParameterType valueToRemove)
     {
         const ScopedLockType lock (getLock());
         auto* e = values.begin();
@@ -841,9 +842,11 @@ public:
             if (valueToRemove == e[i])
             {
                 removeInternal (i);
-                break;
+                return i;
             }
         }
+
+        return -1;
     }
 
     /** Removes items from the array.

modules/juce_core/containers/juce_ListenerList.cpp

@@ -0,0 +1,317 @@
+/*
+  ==============================================================================
+
+   This file is part of the JUCE 7 technical preview.
+   Copyright (c) 2022 - Raw Material Software Limited
+
+   You may use this code under the terms of the GPL v3
+   (see www.gnu.org/licenses).
+
+   For the technical preview this file cannot be licensed commercially.
+
+   JUCE IS PROVIDED "AS IS" WITHOUT ANY WARRANTY, AND ALL WARRANTIES, WHETHER
+   EXPRESSED OR IMPLIED, INCLUDING MERCHANTABILITY AND FITNESS FOR PURPOSE, ARE
+   DISCLAIMED.
+
+  ==============================================================================
+*/
+
+namespace juce
+{
+
+#if JUCE_UNIT_TESTS
+
+class ListenerListTests  : public UnitTest
+{
+public:
+    //==============================================================================
+    class TestListener
+    {
+    public:
+        explicit TestListener (std::function<void()> cb) : callback (std::move (cb)) {}
+
+        void doCallback()
+        {
+            ++numCalls;
+            callback();
+        }
+
+        int getNumCalls() const { return numCalls; }
+
+    private:
+        int numCalls = 0;
+        std::function<void()> callback;
+    };
+
+    class TestObject
+    {
+    public:
+        void addListener (std::function<void()> cb)
+        {
+            listeners.push_back (std::make_unique<TestListener> (std::move (cb)));
+            listenerList.add (listeners.back().get());
+        }
+
+        void removeListener (int i) { listenerList.remove (listeners[(size_t) i].get()); }
+
+        void callListeners()
+        {
+            ++callLevel;
+            listenerList.call ([] (auto& l) { l.doCallback(); });
+            --callLevel;
+        }
+
+        int getNumListeners() const { return (int) listeners.size(); }
+
+        auto& getListener (int i) { return *listeners[(size_t) i]; }
+
+        int getCallLevel() const
+        {
+            return callLevel;
+        }
+
+        bool wereAllNonRemovedListenersCalled (int numCalls) const
+        {
+            return std::all_of (std::begin (listeners),
+                                std::end (listeners),
+                                [&] (auto& listener)
+                                {
+                                    return (! listenerList.contains (listener.get())) || listener->getNumCalls() == numCalls;
+                                });
+        }
+
+    private:
+        std::vector<std::unique_ptr<TestListener>> listeners;
+        ListenerList<TestListener> listenerList;
+        int callLevel = 0;
+    };
+
+    //==============================================================================
+    ListenerListTests() : UnitTest ("ListenerList", UnitTestCategories::containers) {}
+
+    void runTest() override
+    {
+        // This is a test that the pre-iterator adjustment implementation should pass too
+        beginTest ("All non-removed listeners should be called - removing an already called listener");
+        {
+            TestObject test;
+
+            for (int i = 0; i < 20; ++i)
+            {
+                test.addListener ([i, &test]
+                                  {
+                                      if (i == 5)
+                                          test.removeListener (6);
+                                  });
+            }
+
+            test.callListeners();
+            expect (test.wereAllNonRemovedListenersCalled (1));
+        }
+
+        // Iterator adjustment is necessary for passing this
+        beginTest ("All non-removed listeners should be called - removing a yet uncalled listener");
+        {
+            TestObject test;
+
+            for (int i = 0; i < 20; ++i)
+            {
+                test.addListener ([i, &test]
+                                  {
+                                      if (i == 5)
+                                          test.removeListener (4);
+                                  });
+            }
+
+            test.callListeners();
+            expect (test.wereAllNonRemovedListenersCalled (1));
+        }
+
+        // This test case demonstrates why we have to call --it.index instead of it.next()
+        beginTest ("All non-removed listeners should be called - one callback removes multiple listeners");
+        {
+            TestObject test;
+
+            for (int i = 0; i < 20; ++i)
+            {
+                test.addListener ([i, &test]
+                                  {
+                                      if (i == 19)
+                                      {
+                                          test.removeListener (19);
+                                          test.removeListener (0);
+                                      }
+                                  });
+            }
+
+            test.callListeners();
+            expect (test.wereAllNonRemovedListenersCalled (1));
+        }
+
+        beginTest ("All non-removed listeners should be called - removing listeners randomly");
+        {
+            auto random = getRandom();
+
+            for (auto run = 0; run < 10; ++run)
+            {
+                const auto numListeners = random.nextInt ({ 10, 100 });
+                const auto listenersThatRemoveListeners = chooseUnique (random,
+                                                                        numListeners,
+                                                                        random.nextInt ({ 0, numListeners / 2 }));
+
+                // The listener in position [key] should remove listeners in [value]
+                std::map<int, std::set<int>> removals;
+
+                for (auto i : listenersThatRemoveListeners)
+                {
+                    // Random::nextInt ({1, 1}); triggers an assertion
+                    removals[i] = chooseUnique (random,
+                                                numListeners,
+                                                random.nextInt ({ 1, std::max (2, numListeners / 10) }));
+                }
+
+                TestObject test;
+
+                for (int i = 0; i < numListeners; ++i)
+                {
+                    test.addListener ([i, &removals, &test]
+                                      {
+                                          const auto iter = removals.find (i);
+
+                                          if (iter == removals.end())
+                                              return;
+
+                                          for (auto j : iter->second)
+                                          {
+                                              test.removeListener (j);
+                                          }
+                                      });
+                }
+
+                test.callListeners();
+                expect (test.wereAllNonRemovedListenersCalled (1));
+            }
+        }
+
+        // Iterator adjustment is not necessary for passing this
+        beginTest ("All non-removed listeners should be called - add listener during iteration");
+        {
+            TestObject test;
+            const auto numStartingListeners = 20;
+
+            for (int i = 0; i < numStartingListeners; ++i)
+            {
+                test.addListener ([i, &test]
+                                  {
+                                      if (i == 5 || i == 6)
+                                          test.addListener ([] {});
+                                  });
+            }
+
+            test.callListeners();
+
+            // Only the Listeners added before the test can be expected to have been called
+            bool success = true;
+
+            for (int i = 0; i < numStartingListeners; ++i)
+                success = success && test.getListener (i).getNumCalls() == 1;
+
+            // Listeners added during the iteration must not be called in that iteration
+            for (int i = numStartingListeners; i < test.getNumListeners(); ++i)
+                success = success && test.getListener (i).getNumCalls() == 0;
+
+            expect (success);
+        }
+
+        beginTest ("All non-removed listeners should be called - nested ListenerList::call()");
+        {
+            TestObject test;
+
+            for (int i = 0; i < 20; ++i)
+            {
+                test.addListener ([i, &test]
+                                  {
+                                      const auto callLevel = test.getCallLevel();
+
+                                      if (i == 6 && callLevel == 1)
+                                      {
+                                          test.callListeners();
+                                      }
+
+                                      if (i == 5)
+                                      {
+                                          if (callLevel == 1)
+                                              test.removeListener (4);
+                                          else if (callLevel == 2)
+                                              test.removeListener (6);
+                                      }
+                                  });
+            }
+
+            test.callListeners();
+            expect (test.wereAllNonRemovedListenersCalled (2));
+        }
+
+        beginTest ("All non-removed listeners should be called - random ListenerList::call()");
+        {
+            const auto numListeners = 20;
+            auto random = getRandom();
+
+            for (int run = 0; run < 10; ++run)
+            {
+                TestObject test;
+                auto numCalls = 0;
+
+                auto listenersToRemove = chooseUnique (random, numListeners, numListeners / 2);
+
+                for (int i = 0; i < numListeners; ++i)
+                {
+                    // Capturing numListeners is a warning on MacOS, not capturing it is an error on Windows
+                    test.addListener ([&]
+                                      {
+                                          const auto callLevel = test.getCallLevel();
+
+                                          if (callLevel < 4 && random.nextFloat() < 0.05f)
+                                          {
+                                              ++numCalls;
+                                              test.callListeners();
+                                          }
+
+                                          if (random.nextFloat() < 0.5f)
+                                          {
+                                              const auto listenerToRemove = random.nextInt ({ 0, numListeners });
+
+                                              if (listenersToRemove.erase (listenerToRemove) > 0)
+                                                  test.removeListener (listenerToRemove);
+                                          }
+                                      });
+                }
+
+                while (listenersToRemove.size() > 0)
+                {
+                    test.callListeners();
+                    ++numCalls;
+                }
+
+                expect (test.wereAllNonRemovedListenersCalled (numCalls));
+            }
+        }
+    }
+
+private:
+    static std::set<int> chooseUnique (Random& random, int max, int numChosen)
+    {
+        std::set<int> result;
+
+        while ((int) result.size() < numChosen)
+            result.insert (random.nextInt ({ 0, max }));
+
+        return result;
+    }
+};
+
+static ListenerListTests listenerListTests;
+
+#endif
+
+} // namespace juce

modules/juce_core/containers/juce_ListenerList.h

@@ -47,10 +47,11 @@ namespace juce
     listeners.call ([] (MyListenerType& l) { l.myCallbackMethod (1234, true); });
     @endcode
 
-    If you add or remove listeners from the list during one of the callbacks - i.e. while
-    it's in the middle of iterating the listeners, then it's guaranteed that no listeners
-    will be mistakenly called after they've been removed, but it may mean that some of the
-    listeners could be called more than once, or not at all, depending on the list's order.
+    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.
+
+    Listeners added during an iteration are guaranteed to be not called in that 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
@@ -73,7 +74,13 @@ public:
     ListenerList() = default;
 
     /** Destructor. */
-    ~ListenerList() = default;
+    ~ListenerList()
+    {
+        WrappedIterator::forEach (activeIterators, [&] (auto& iter)
+        {
+            iter.invalidate();
+        });
+    }
 
     //==============================================================================
     /** Adds a listener to the list.
@@ -95,7 +102,16 @@ public:
     void remove (ListenerClass* listenerToRemove)
     {
         jassert (listenerToRemove != nullptr); // Listeners can't be null pointers!
-        listeners.removeFirstMatchingValue (listenerToRemove);
+
+        typename ArrayType::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;
+        });
     }
 
     /** Returns the number of registered listeners. */
@@ -120,8 +136,8 @@ public:
     {
         typename ArrayType::ScopedLockType lock (listeners.getLock());
 
-        for (Iterator<DummyBailOutChecker, ThisType> iter (*this); iter.next();)
-            callback (*iter.getListener());
+        for (WrappedIterator iter (*this, activeIterators); iter.get().next();)
+            callback (*iter.get().getListener());
     }
 
     /** Calls a member function with 1 parameter, on all but the specified listener in the list.
@@ -132,9 +148,9 @@ public:
     {
         typename ArrayType::ScopedLockType lock (listeners.getLock());
 
-        for (Iterator<DummyBailOutChecker, ThisType> iter (*this); iter.next();)
+        for (WrappedIterator iter (*this, activeIterators); iter.get().next();)
         {
-            auto* l = iter.getListener();
+            auto* l = iter.get().getListener();
 
             if (l != listenerToExclude)
                 callback (*l);
@@ -149,8 +165,10 @@ public:
     {
         typename ArrayType::ScopedLockType lock (listeners.getLock());
 
-        for (Iterator<BailOutCheckerType, ThisType> iter (*this); iter.next (bailOutChecker);)
-            callback (*iter.getListener());
+        for (WrappedIterator iter (*this, activeIterators); iter.get().next (bailOutChecker);)
+        {
+            callback (*iter.get().getListener());
+        }
     }
 
     /** Calls a member function, with 1 parameter, on all but the specified listener in the list
@@ -164,9 +182,9 @@ public:
     {
         typename ArrayType::ScopedLockType lock (listeners.getLock());
 
-        for (Iterator<BailOutCheckerType, ThisType> iter (*this); iter.next (bailOutChecker);)
+        for (WrappedIterator iter (*this, activeIterators); iter.get().next (bailOutChecker);)
         {
-            auto* l = iter.getListener();
+            auto* l = iter.get().getListener();
 
             if (l != listenerToExclude)
                 callback (*l);
@@ -187,15 +205,12 @@ public:
 
     //==============================================================================
     /** Iterates the listeners in a ListenerList. */
-    template <class BailOutCheckerType, class ListType>
     struct Iterator
     {
-        Iterator (const ListType& listToIterate) noexcept
+        explicit Iterator (const ListenerList& listToIterate) noexcept
             : list (listToIterate), index (listToIterate.size())
         {}
 
-        ~Iterator() = default;
-
         //==============================================================================
         bool next() noexcept
         {
@@ -211,21 +226,23 @@ public:
             return index >= 0;
         }
 
+        template <class BailOutCheckerType>
         bool next (const BailOutCheckerType& bailOutChecker) noexcept
         {
             return (! bailOutChecker.shouldBailOut()) && next();
         }
 
-        typename ListType::ListenerType* getListener() const noexcept
+        ListenerClass* getListener() const noexcept
         {
             return list.getListeners().getUnchecked (index);
         }
 
-        //==============================================================================
     private:
-        const ListType& list;
+        const ListenerList& list;
         int index;
 
+        friend ListenerList;
+
         JUCE_DECLARE_NON_COPYABLE (Iterator)
     };
 
@@ -260,7 +277,7 @@ public:
     {
         typename ArrayType::ScopedLockType lock (listeners.getLock());
 
-        for (Iterator<DummyBailOutChecker, ThisType> iter (*this); iter.next();)
+        for (Iterator iter (*this); iter.next();)
             (iter.getListener()->*callbackFunction) (static_cast<typename TypeHelpers::ParameterType<Args>::type> (args)...);
     }
 
@@ -271,7 +288,7 @@ public:
     {
         typename ArrayType::ScopedLockType lock (listeners.getLock());
 
-        for (Iterator<DummyBailOutChecker, ThisType> iter (*this); iter.next();)
+        for (Iterator iter (*this); iter.next();)
             if (iter.getListener() != listenerToExclude)
                 (iter.getListener()->*callbackFunction) (static_cast<typename TypeHelpers::ParameterType<Args>::type> (args)...);
     }
@@ -283,7 +300,7 @@ public:
     {
         typename ArrayType::ScopedLockType lock (listeners.getLock());
 
-        for (Iterator<BailOutCheckerType, ThisType> iter (*this); iter.next (bailOutChecker);)
+        for (Iterator iter (*this); iter.next (bailOutChecker);)
             (iter.getListener()->*callbackFunction) (static_cast<typename TypeHelpers::ParameterType<Args>::type> (args)...);
     }
 
@@ -295,15 +312,49 @@ public:
     {
         typename ArrayType::ScopedLockType lock (listeners.getLock());
 
-        for (Iterator<BailOutCheckerType, ThisType> iter (*this); iter.next (bailOutChecker);)
+        for (Iterator iter (*this); iter.next (bailOutChecker);)
             if (iter.getListener() != listenerToExclude)
                 (iter.getListener()->*callbackFunction) (static_cast<typename TypeHelpers::ParameterType<Args>::type> (args)...);
     }
    #endif
 
 private:
+    class WrappedIterator
+    {
+    public:
+        WrappedIterator (const ListenerList& listToIterate, WrappedIterator*& listHeadIn)
+            : it (listToIterate), listHead (listHeadIn), next (listHead)
+        {
+            listHead = this;
+        }
+
+        ~WrappedIterator()
+        {
+            if (valid)
+                listHead = next;
+        }
+
+        auto& get() noexcept { return it; }
+
+        template <typename Callback>
+        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;
+    };
+
     //==============================================================================
     ArrayType listeners;
+    WrappedIterator* activeIterators = nullptr;
 
     JUCE_DECLARE_NON_COPYABLE (ListenerList)
 };

modules/juce_core/juce_core.cpp

@@ -128,6 +128,7 @@
 //==============================================================================
 #include "containers/juce_AbstractFifo.cpp"
 #include "containers/juce_ArrayBase.cpp"
+#include "containers/juce_ListenerList.cpp"
 #include "containers/juce_NamedValueSet.cpp"
 #include "containers/juce_OwnedArray.cpp"
 #include "containers/juce_PropertySet.cpp"