From 32abde7f19c55d0e5140ac6f2040787fa511087f Mon Sep 17 00:00:00 2001
From: Michael Davidsaver <mdavidsaver@gmail.com>
Date: Wed, 28 Mar 2018 15:49:22 -0700
Subject: [PATCH] redo Timer to avoid data race

run() was accessing this->head w/o locking
---
 src/misc/pv/timer.h |  11 +++--
 src/misc/timer.cpp  | 116 +++++++++++++++++++++++++-------------------
 2 files changed, 72 insertions(+), 55 deletions(-)

diff --git a/src/misc/pv/timer.h b/src/misc/pv/timer.h
index 3cc6277..6b827f9 100644
--- a/src/misc/pv/timer.h
+++ b/src/misc/pv/timer.h
@@ -56,7 +56,7 @@ public:
     virtual void timerStopped() = 0;
 private:
     TimerCallbackPtr next;
-    TimeStamp timeToRun;
+    epicsTime timeToRun;
     double period;
     bool onList;
     friend class Timer;
@@ -111,23 +111,24 @@ public:
      * @param timerCallback the timerCallback.
      * @return (false,true) if (not, is) scheduled.
      */
-    bool isScheduled(TimerCallbackPtr const &timerCallback);
+    bool isScheduled(TimerCallbackPtr const &timerCallback) const;
     /**
      * show the elements in the timer queue.
      * @param o The output stream for the output
      */
-    void dump(std::ostream& o);
+    void dump(std::ostream& o) const;
 
 private:
+    // call with mutex held
     void addElement(TimerCallbackPtr const &timerCallback);
     TimerCallbackPtr head;
-    Mutex mutex;
+    mutable Mutex mutex;
     Event waitForWork;
     bool alive;
     Thread thread;
 };
 
-epicsShareExtern std::ostream& operator<<(std::ostream& o, Timer& timer);
+epicsShareExtern std::ostream& operator<<(std::ostream& o, const Timer& timer);
 
 }}
 #endif  /* TIMER_H */
diff --git a/src/misc/timer.cpp b/src/misc/timer.cpp
index 28f5686..de9587d 100644
--- a/src/misc/timer.cpp
+++ b/src/misc/timer.cpp
@@ -16,6 +16,7 @@
 #include <iostream>
 
 #include <epicsThread.h>
+#include <epicsGuard.h>
 
 #define epicsExportSharedSymbols
 #include <pv/timer.h>
@@ -36,16 +37,21 @@ Timer::Timer(string threadName,ThreadPriority priority)
   thread(threadName,priority,this)
 {}
 
+// call with mutex held
 void Timer::addElement(TimerCallbackPtr const & timerCallback)
 {
+    assert(!timerCallback->onList);
+    assert(!timerCallback->next);
+
     timerCallback->onList = true;
-    if(head.get()==NULL) {
+    if(!head) {
         head = timerCallback;
         timerCallback->next.reset();
         return;
     }
-    TimerCallbackPtr nextNode(head);
-    TimerCallbackPtr prevNode;
+
+    TimerCallbackPtr nextNode(head), prevNode;
+
     while(true) {
         if(timerCallback->timeToRun < nextNode->timeToRun) {
             if(prevNode) {
@@ -90,7 +96,7 @@ void Timer::cancel(TimerCallbackPtr const &timerCallback)
     throw std::logic_error(string(""));
 }
 
-bool Timer::isScheduled(TimerCallbackPtr const &timerCallback)
+bool Timer::isScheduled(TimerCallbackPtr const &timerCallback) const
 {
     Lock xx(mutex);
     return timerCallback->onList;
@@ -99,43 +105,56 @@ bool Timer::isScheduled(TimerCallbackPtr const &timerCallback)
 
 void Timer::run()
 {
-    TimeStamp currentTime;
-    while(true) {
-         double period = 0.0;
-         TimerCallbackPtr nodeToCall;
-         {
-             Lock xx(mutex);
-             currentTime.getCurrent();
-             if (!alive) break;
-             TimerCallbackPtr timerCallback = head;
-             if(timerCallback) {
-                 double diff = TimeStamp::diff(
-                     timerCallback->timeToRun,currentTime);
-                 if(diff<=0.0) {
-                     nodeToCall = timerCallback;
-                     nodeToCall->onList = false;
-                     head = head->next;
-                     period = timerCallback->period;
-                     if(period>0.0) {
-                         timerCallback->timeToRun += period;
-                         addElement(timerCallback);
-                     }
-                     timerCallback = head;
+    epicsGuard<epicsMutex> G(mutex);
+
+    while(alive) {
+
+         TimerCallbackPtr next;
+
+         epicsTime currentTime(epicsTime::getCurrent());
+
+         double delay = -1;
+
+         if(head) {
+             // there may be work to be done
+
+             delay = head->timeToRun - currentTime;
+
+             if(delay <= 0.0) {
+                 // head timer has expired
+
+                 // we take head, move head = head->next
+                 next.swap(head);
+                 head.swap(next->next);
+
+                 next->onList = false;
+
+                 // re-schedule periodic
+                 if(next->period > 0.0) {
+                     next->timeToRun += next->period;
+                     addElement(next);
+                 }
+
+                 if(head) {
+                     delay = head->timeToRun - currentTime;
                  }
              }
-         }
-         if(nodeToCall) {
-             nodeToCall->callback();
-         }
+         };
+
+         bool hasHead = !!head;
+
          {
-             Lock xx(mutex);
-             if(!alive) break;
-         }
-         if(head.get()==NULL) {
-            waitForWork.wait();
-         } else {
-             double delay = TimeStamp::diff(head->timeToRun,currentTime);
-             waitForWork.wait(delay);
+             epicsGuardRelease<epicsMutex> U(G);
+
+             if(next) {
+                 next->callback();
+             }
+
+             if(hasHead) {
+                 waitForWork.wait(delay);
+             } else {
+                 waitForWork.wait();
+             }
          }
     } 
 }
@@ -195,24 +214,21 @@ void Timer::schedulePeriodic(
     if(isFirst) waitForWork.signal();
 }
 
-void Timer::dump(std::ostream& o)
+void Timer::dump(std::ostream& o) const
 {
     Lock xx(mutex);
     if(!alive) return;
-    TimeStamp currentTime;
+    epicsTime currentTime(epicsTime::getCurrent());
     TimerCallbackPtr nodeToCall(head);
-    currentTime.getCurrent();
-    while(true) {
-         if(nodeToCall.get()==NULL) return;
-         TimeStamp timeToRun = nodeToCall->timeToRun;
-         double period = nodeToCall->period;
-         double diff = TimeStamp::diff(timeToRun,currentTime);
-         o << "timeToRun " << diff << " period " << period << std::endl;
-         nodeToCall = nodeToCall->next;
-     }
+
+    while(nodeToCall) {
+        o << "timeToRun " << (nodeToCall->timeToRun - currentTime)
+          << " period " << nodeToCall->period << "\n";
+        nodeToCall = nodeToCall->next;
+    }
 }
 
-std::ostream& operator<<(std::ostream& o, Timer& timer)
+std::ostream& operator<<(std::ostream& o, const Timer& timer)
 {
     timer.dump(o);
     return o;