Author: astitcher
Date: Thu Jul 5 19:38:26 2012
New Revision: 1357827
URL: http://svn.apache.org/viewvc?rev=1357827&view=rev
Log:
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.
Modified:
qpid/trunk/qpid/cpp/src/qpid/sys/Timer.cpp
qpid/trunk/qpid/cpp/src/qpid/sys/Timer.h
Modified: qpid/trunk/qpid/cpp/src/qpid/sys/Timer.cpp
URL:
http://svn.apache.org/viewvc/qpid/trunk/qpid/cpp/src/qpid/sys/Timer.cpp?rev=1357827&r1=1357826&r2=1357827&view=diff
==============================================================================
--- qpid/trunk/qpid/cpp/src/qpid/sys/Timer.cpp (original)
+++ qpid/trunk/qpid/cpp/src/qpid/sys/Timer.cpp Thu Jul 5 19:38:26 2012
@@ -35,7 +35,7 @@ TimerTask::TimerTask(Duration timeout, c
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
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);
Modified: qpid/trunk/qpid/cpp/src/qpid/sys/Timer.h
URL:
http://svn.apache.org/viewvc/qpid/trunk/qpid/cpp/src/qpid/sys/Timer.h?rev=1357827&r1=1357826&r2=1357827&view=diff
==============================================================================
--- qpid/trunk/qpid/cpp/src/qpid/sys/Timer.h (original)
+++ qpid/trunk/qpid/cpp/src/qpid/sys/Timer.h Thu Jul 5 19:38:26 2012
@@ -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();
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
