NO-JIRA: Fix for potential Timer deadlock issue:

- Previously we used a mutex to prevent cancelling a TimerTask whilst it
  was still executing from within its callback in another thread.

  This violates the principle that you shouldn't hold locks when calling
  the arbitrary code in a callback, and so is subject to potential
  deadlock problems.

- This fix only works if no timer callback calls TimerTask::cancel();
  this is true with the current code. And there is no good reason to
  call cancel() from within a callback, as cancel is the default
  behviour in any case - you have to specifically reschedule a
  recurring timer.

git-svn-id: https://svn.apache.org/repos/asf/qpid/trunk@1357827 13f79535-47bb-0310-9956-ffa450edef68

2 files changed, 51 insertions, 11 deletions

diff --git a/qpid/cpp/src/qpid/sys/Timer.cpp b/qpid/cpp/src/qpid/sys/Timer.cpp
index 47752e4584..0b3544e7b0 100644
--- a/qpid/cpp/src/qpid/sys/Timer.cpp
+++ b/qpid/cpp/src/qpid/sys/Timer.cpp
@@ -35,7 +35,7 @@ TimerTask::TimerTask(Duration timeout, const std::string&  n) :
     sortTime(AbsTime::FarFuture()),
     period(timeout),
     nextFireTime(AbsTime::now(), timeout),
-    cancelled(false)
+    state(WAITING)
 {}
 
 TimerTask::TimerTask(AbsTime time, const std::string&  n) :
@@ -43,7 +43,7 @@ TimerTask::TimerTask(AbsTime time, const std::string&  n) :
     sortTime(AbsTime::FarFuture()),
     period(0),
     nextFireTime(time),
-    cancelled(false)
+    state(WAITING)
 {}
 
 TimerTask::~TimerTask() {}
@@ -52,27 +52,48 @@ bool TimerTask::readyToFire() const {
     return !(nextFireTime > AbsTime::now());
 }
 
+bool TimerTask::prepareToFire() {
+    Monitor::ScopedLock l(stateMonitor);
+    if (state != CANCELLED) {
+        state = CALLBACK;
+        return true;
+    } else {
+        return false;
+    }
+}
+
 void TimerTask::fireTask() {
-    cancelled = true;
     fire();
 }
 
+void TimerTask::finishFiring() {
+    Monitor::ScopedLock l(stateMonitor);
+    if (state != CANCELLED) {
+        state = WAITING;
+        stateMonitor.notifyAll();
+    }
+}
+
 // This can only be used to setup the next fire time. After the Timer has already fired
 void TimerTask::setupNextFire() {
     if (period && readyToFire()) {
         nextFireTime = max(AbsTime::now(), AbsTime(nextFireTime, period));
-        cancelled = false;
     } else {
         QPID_LOG(error, name << " couldn't setup next timer firing: " << Duration(nextFireTime, AbsTime::now()) << "[" << period << "]");
     }
 }
 
 // Only allow tasks to be delayed
-void TimerTask::restart() { nextFireTime = max(nextFireTime, AbsTime(AbsTime::now(), period)); }
+void TimerTask::restart() {
+    nextFireTime = max(nextFireTime, AbsTime(AbsTime::now(), period));
+}
 
 void TimerTask::cancel() {
-    ScopedLock<Mutex> l(callbackLock);
-    cancelled = true;
+    Monitor::ScopedLock l(stateMonitor);
+    while (state == CALLBACK) {
+        stateMonitor.wait();
+    }
+    state = CANCELLED;
 }
 
 void TimerTask::setFired() {
@@ -96,6 +117,22 @@ Timer::~Timer()
     stop();
 }
 
+class TimerTaskCallbackScope {
+    TimerTask& tt;
+public:
+    explicit TimerTaskCallbackScope(TimerTask& t) :
+        tt(t)
+    {}
+
+    operator bool() {
+        return !tt.prepareToFire();
+    }
+
+    ~TimerTaskCallbackScope() {
+        tt.finishFiring();
+    }
+};
+
 // TODO AStitcher 21/08/09 The threshholds for emitting warnings are a little arbitrary
 void Timer::run()
 {
@@ -112,8 +149,8 @@ void Timer::run()
             AbsTime start(AbsTime::now());
             Duration delay(t->sortTime, start);
             {
-            ScopedLock<Mutex> l(t->callbackLock);
-            if (t->cancelled) {
+            TimerTaskCallbackScope s(*t);
+            if (s) {
                 {
                     Monitor::ScopedUnlock u(monitor);
                     drop(t);
diff --git a/qpid/cpp/src/qpid/sys/Timer.h b/qpid/cpp/src/qpid/sys/Timer.h
index fccb17dbc2..af0b1b6803 100644
--- a/qpid/cpp/src/qpid/sys/Timer.h
+++ b/qpid/cpp/src/qpid/sys/Timer.h
@@ -40,6 +40,7 @@ class Timer;
 
 class TimerTask : public RefCounted {
   friend class Timer;
+  friend class TimerTaskCallbackScope;
   friend bool operator<(const boost::intrusive_ptr<TimerTask>&,
                         const boost::intrusive_ptr<TimerTask>&);
 
@@ -47,9 +48,11 @@ class TimerTask : public RefCounted {
     AbsTime sortTime;
     Duration period;
     AbsTime nextFireTime;
-    Mutex callbackLock;
-    volatile bool cancelled;
+    qpid::sys::Monitor stateMonitor;
+    enum {WAITING, CALLBACK, CANCELLED} state;
 
+    bool prepareToFire();
+    void finishFiring();
     bool readyToFire() const;
     void fireTask();