Fixed some Timer threading issues

modules/juce_events/timers/juce_Timer.cpp

@@ -32,6 +32,7 @@ public:
 
     TimerThread()  : Thread ("JUCE Timer")
     {
+        timers.reserve (32);
         triggerAsyncUpdate();
     }
 
@@ -49,7 +50,7 @@ public:
     void run() override
     {
         auto lastTime = Time::getMillisecondCounter();
-        MessageManager::MessageBase::Ptr messageToSend (new CallTimersMessage());
+        ReferenceCountedObjectPtr<CallTimersMessage> messageToSend (new CallTimersMessage());
 
         while (! threadShouldExit())
         {
@@ -90,27 +91,31 @@ public:
 
     void callTimers()
     {
-        // avoid getting stuck in a loop if a timer callback repeatedly takes too long
         auto timeout = Time::getMillisecondCounter() + 100;
 
         const LockType::ScopedLockType sl (lock);
 
-        while (firstTimer != nullptr && firstTimer->timerCountdownMs <= 0)
+        while (! timers.empty())
         {
-            auto* t = firstTimer;
-            t->timerCountdownMs = t->timerPeriodMs;
+            auto& first = timers.front();
 
-            removeTimer (t);
-            addTimer (t);
+            if (first.countdownMs > 0)
+                break;
+
+            auto* timer = first.timer;
+            first.countdownMs = timer->timerPeriodMs;
+            shuffleTimerBackInQueue (0);
+            notify();
 
             const LockType::ScopedUnlockType ul (lock);
 
             JUCE_TRY
             {
-                t->timerCallback();
+                timer->timerCallback();
             }
             JUCE_CATCH_EXCEPTION
 
+            // avoid getting stuck in a loop if a timer callback repeatedly takes too long
             if (Time::getMillisecondCounter() > timeout)
                 break;
         }
@@ -145,27 +150,24 @@ public:
             instance->removeTimer (tim);
     }
 
-    static inline void resetCounter (Timer* tim, int newCounter) noexcept
+    static inline void resetCounter (Timer* tim) noexcept
     {
         if (instance != nullptr)
-        {
-            tim->timerCountdownMs = newCounter;
-            tim->timerPeriodMs = newCounter;
-
-            if ((tim->nextTimer != nullptr && tim->nextTimer->timerCountdownMs < tim->timerCountdownMs)
-                 || (tim->previousTimer != nullptr && tim->previousTimer->timerCountdownMs > tim->timerCountdownMs))
-            {
-                instance->removeTimer (tim);
-                instance->addTimer (tim);
-            }
-        }
+            instance->resetTimerCounter (tim);
     }
 
     static TimerThread* instance;
     static LockType lock;
 
 private:
-    Timer* firstTimer = nullptr;
+    struct TimerCountdown
+    {
+        Timer* timer;
+        int countdownMs;
+    };
+
+    std::vector<TimerCountdown> timers;
+
     WaitableEvent callbackArrived;
 
     struct CallTimersMessage  : public MessageManager::MessageBase
@@ -180,76 +182,122 @@ private:
     };
 
     //==============================================================================
-    void addTimer (Timer* t) noexcept
+    void addTimer (Timer* t)
     {
-       #if JUCE_DEBUG
-        // trying to add a timer that's already here - shouldn't get to this point,
+        // Trying to add a timer that's already here - shouldn't get to this point,
         // so if you get this assertion, let me know!
-        jassert (! timerExists (t));
-       #endif
+        jassert (std::find_if (timers.begin(), timers.end(),
+                               [t](TimerCountdown i) { return i.timer == t; }) == timers.end());
 
-        auto* i = firstTimer;
-
-        if (i == nullptr || i->timerCountdownMs > t->timerCountdownMs)
-        {
-            t->nextTimer = firstTimer;
-            firstTimer = t;
-        }
-        else
-        {
-            while (i->nextTimer != nullptr && i->nextTimer->timerCountdownMs <= t->timerCountdownMs)
-                i = i->nextTimer;
-
-            jassert (i != nullptr);
-
-            t->nextTimer = i->nextTimer;
-            t->previousTimer = i;
-            i->nextTimer = t;
-        }
-
-        if (auto* next = t->nextTimer)
-            next->previousTimer = t;
-
-        jassert ((t->nextTimer == nullptr || t->nextTimer->timerCountdownMs >= t->timerCountdownMs)
-                  && (t->previousTimer == nullptr || t->previousTimer->timerCountdownMs <= t->timerCountdownMs));
+        auto pos = timers.size();
 
+        timers.push_back ({ t, t->timerPeriodMs });
+        t->positionInQueue = pos;
+        shuffleTimerForwardInQueue (pos);
         notify();
     }
 
-    void removeTimer (Timer* t) noexcept
+    void removeTimer (Timer* t)
     {
-       #if JUCE_DEBUG
-        // trying to remove a timer that's not here - shouldn't get to this point,
-        // so if you get this assertion, let me know!
-        jassert (timerExists (t));
-       #endif
+        auto pos = t->positionInQueue;
+        auto lastIndex = timers.size() - 1;
 
-        if (t->previousTimer != nullptr)
+        jassert (pos <= lastIndex);
+        jassert (timers[pos].timer == t);
+
+        for (auto i = pos; i < lastIndex; ++i)
         {
-            jassert (firstTimer != t);
-            t->previousTimer->nextTimer = t->nextTimer;
-        }
-        else
-        {
-            jassert (firstTimer == t);
-            firstTimer = t->nextTimer;
+            timers[i] = timers[i + 1];
+            timers[i].timer->positionInQueue = i;
         }
 
-        if (t->nextTimer != nullptr)
-            t->nextTimer->previousTimer = t->previousTimer;
-
-        t->nextTimer = nullptr;
-        t->previousTimer = nullptr;
+        timers.pop_back();
     }
 
-    int getTimeUntilFirstTimer (int numMillisecsElapsed) const
+    void resetTimerCounter (Timer* t) noexcept
+    {
+        auto pos = t->positionInQueue;
+
+        jassert (pos < timers.size());
+        jassert (timers[pos].timer == t);
+
+        auto lastCountdown = timers[pos].countdownMs;
+        auto newCountdown = t->timerPeriodMs;
+
+        if (newCountdown != lastCountdown)
+        {
+            timers[pos].countdownMs = newCountdown;
+
+            if (newCountdown > lastCountdown)
+                shuffleTimerBackInQueue (pos);
+            else
+                shuffleTimerForwardInQueue (pos);
+
+            notify();
+        }
+    }
+
+    void shuffleTimerBackInQueue (size_t pos)
+    {
+        auto numTimers = timers.size();
+
+        if (pos < numTimers - 1)
+        {
+            auto t = timers[pos];
+
+            for (;;)
+            {
+                auto next = pos + 1;
+
+                if (next == numTimers || timers[next].countdownMs >= t.countdownMs)
+                    break;
+
+                timers[pos] = timers[next];
+                timers[pos].timer->positionInQueue = pos;
+
+                ++pos;
+            }
+
+            timers[pos] = t;
+            t.timer->positionInQueue = pos;
+        }
+    }
+
+    void shuffleTimerForwardInQueue (size_t pos)
+    {
+        if (pos > 0)
+        {
+            auto t = timers[pos];
+
+            while (pos > 0)
+            {
+                auto& prev = timers[(size_t) pos - 1];
+
+                if (prev.countdownMs <= t.countdownMs)
+                    break;
+
+                timers[pos] = prev;
+                timers[pos].timer->positionInQueue = pos;
+
+                --pos;
+            }
+
+            timers[pos] = t;
+            t.timer->positionInQueue = pos;
+        }
+    }
+
+    int getTimeUntilFirstTimer (int numMillisecsElapsed)
     {
         const LockType::ScopedLockType sl (lock);
 
-        for (auto* t = firstTimer; t != nullptr; t = t->nextTimer)
-            t->timerCountdownMs -= numMillisecsElapsed;
+        if (timers.empty())
+            return 1000;
 
-        return firstTimer != nullptr ? firstTimer->timerCountdownMs : 1000;
+        for (auto& t : timers)
+            t.countdownMs -= numMillisecsElapsed;
+
+        return timers.front().countdownMs;
     }
 
     void handleAsyncUpdate() override
@@ -257,17 +305,6 @@ private:
         startThread (7);
     }
 
-   #if JUCE_DEBUG
-    bool timerExists (Timer* t) const noexcept
-    {
-        for (auto* tt = firstTimer; tt != nullptr; tt = tt->nextTimer)
-            if (tt == t)
-                return true;
-
-        return false;
-    }
-   #endif
-
     JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (TimerThread)
 };
 
@@ -291,16 +328,13 @@ void Timer::startTimer (int interval) noexcept
 
     const TimerThread::LockType::ScopedLockType sl (TimerThread::lock);
 
-    if (timerPeriodMs == 0)
-    {
-        timerCountdownMs = interval;
-        timerPeriodMs = jmax (1, interval);
+    bool wasStopped = (timerPeriodMs == 0);
+    timerPeriodMs = jmax (1, interval);
+
+    if (wasStopped)
         TimerThread::add (this);
-    }
     else
-    {
-        TimerThread::resetCounter (this, interval);
-    }
+        TimerThread::resetCounter (this);
 }
 
 void Timer::startTimerHz (int timerFrequencyHz) noexcept

modules/juce_events/timers/juce_Timer.h

@@ -126,8 +126,8 @@ public:
 private:
     class TimerThread;
     friend class TimerThread;
-    int timerCountdownMs = 0, timerPeriodMs = 0;
-    Timer* previousTimer = {}, *nextTimer = {};
+    size_t positionInQueue = (size_t) -1;
+    int timerPeriodMs = 0;
 
     Timer& operator= (const Timer&) = delete;
 };