summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorChris Wilson <chris+github@qwirx.com>2006-12-03 13:58:14 +0000
committerChris Wilson <chris+github@qwirx.com>2006-12-03 13:58:14 +0000
commitcff19bd8c52b58095aaf2aa35f38673c29f1aa88 (patch)
treedb721b29a1f290d228805429d3516a486dc13e81
parentbc82918ca28737fc934c33b69e13bb2efb28d89e (diff)
Fixed a race condition caused by rescheduling in signal handler (refs
#3, refs #9)
-rw-r--r--lib/common/Timer.cpp84
-rw-r--r--lib/common/Timer.h56
-rw-r--r--test/common/testcommon.cpp2
3 files changed, 90 insertions, 52 deletions
diff --git a/lib/common/Timer.cpp b/lib/common/Timer.cpp
index 76b700d3..10365e03 100644
--- a/lib/common/Timer.cpp
+++ b/lib/common/Timer.cpp
@@ -17,19 +17,7 @@
#include "MemLeakFindOn.h"
std::vector<Timer*>* Timers::spTimers = NULL;
-
-// --------------------------------------------------------------------------
-//
-// Function
-// Name: static void TimerSigHandler(int)
-// Purpose: Signal handler, notifies Timers class
-// Created: 19/3/04
-//
-// --------------------------------------------------------------------------
-static void TimerSigHandler(int iUnused)
-{
- Timers::Signal();
-}
+bool Timers::sRescheduleNeeded = false;
// --------------------------------------------------------------------------
//
@@ -43,15 +31,13 @@ void Timers::Init()
{
ASSERT(!spTimers);
- #ifdef PLATFORM_CYGWIN
- ASSERT(::signal(SIGALRM, TimerSigHandler) == 0);
- #elif defined WIN32
+ #if defined WIN32 && ! defined PLATFORM_CYGWIN
// no support for signals at all
InitTimer();
- SetTimerHandler(TimerSigHandler);
+ SetTimerHandler(Timers::SignalHandler);
#else
- ASSERT(::signal(SIGALRM, TimerSigHandler) == 0);
- #endif // PLATFORM_CYGWIN
+ ASSERT(::signal(SIGALRM, Timers::SignalHandler) == 0);
+ #endif // WIN32 && !PLATFORM_CYGWIN
spTimers = new std::vector<Timer*>;
}
@@ -68,15 +54,13 @@ void Timers::Cleanup()
{
ASSERT(spTimers);
- #ifdef PLATFORM_CYGWIN
- ASSERT(::signal(SIGALRM, NULL) == TimerSigHandler);
- #elif defined WIN32
+ #if defined WIN32 && ! defined PLATFORM_CYGWIN
// no support for signals at all
SetTimerHandler(NULL);
FiniTimer();
#else
- ASSERT(::signal(SIGALRM, NULL) == TimerSigHandler);
- #endif // PLATFORM_CYGWIN
+ ASSERT(::signal(SIGALRM, NULL) == Timers::SignalHandler);
+ #endif // WIN32 && !PLATFORM_CYGWIN
spTimers->clear();
delete spTimers;
@@ -142,17 +126,48 @@ void Timers::Reschedule()
{
ASSERT(spTimers);
+ // Set the reschedule-needed flag to false before we start.
+ // If a timer event occurs while we are scheduling, then we
+ // may or may not need to reschedule again, but this way
+ // we will do it anyway.
+ sRescheduleNeeded = false;
+
+ // scan for and remove expired timers, which requires
+ // us to restart the scan each time we remove one.
+ bool restart = true;
+ while (restart)
+ {
+ restart = false;
+
+ for (std::vector<Timer*>::iterator i = spTimers->begin();
+ i != spTimers->end(); i++)
+ {
+ Timer& rTimer = **i;
+ if (rTimer.HasExpired())
+ {
+ spTimers->erase(i);
+ restart = true;
+ break;
+ }
+ }
+ }
+
+ // Now the only remaining timers should all be in the future.
+ // Scan to find the next one to fire (earliest deadline).
+
box_time_t timeNow = GetCurrentBoxTime();
int64_t timeToNextEvent = 0;
-
+
for (std::vector<Timer*>::iterator i = spTimers->begin();
i != spTimers->end(); i++)
{
Timer& rTimer = **i;
- ASSERT(!rTimer.HasExpired());
-
int64_t timeToExpiry = rTimer.GetExpiryTime() - timeNow;
- if (timeToExpiry <= 0) timeToExpiry = 1;
+
+ if (timeToExpiry <= 0)
+ {
+ timeToExpiry = 1;
+ }
if (timeToNextEvent == 0 || timeToNextEvent > timeToExpiry)
{
@@ -183,14 +198,17 @@ void Timers::Reschedule()
// --------------------------------------------------------------------------
//
// Function
-// Name: static void Timers::Signal()
-// Purpose: Called by signal handler. Signals any timers which
+// Name: static void Timers::SignalHandler(unused)
+// Purpose: Called as signal handler. Signals any timers which
// are due or overdue, removes them from the set,
-// and reschedules next wakeup.
+// and requests a reschedule event. The actual
+// reschedule will happen next time someone calls
+// Timer::HasExpired(), which may be rather late,
+// but we can't do it from here, in signal context.
// Created: 5/11/2006
//
// --------------------------------------------------------------------------
-void Timers::Signal()
+void Timers::SignalHandler(int iUnused)
{
ASSERT(spTimers);
@@ -212,7 +230,7 @@ void Timers::Signal()
}
}
- Reschedule();
+ Timers::RequestReschedule();
}
Timer::Timer(size_t timeoutSecs)
diff --git a/lib/common/Timer.h b/lib/common/Timer.h
index e0eb34db..390442df 100644
--- a/lib/common/Timer.h
+++ b/lib/common/Timer.h
@@ -18,23 +18,7 @@
#include "MemLeakFindOn.h"
#include "BoxTime.h"
-class Timer
-{
-public:
- Timer(size_t timeoutSecs);
- virtual ~Timer();
- Timer(const Timer &);
- Timer &operator=(const Timer &);
-
-public:
- box_time_t GetExpiryTime() { return mExpires; }
- bool HasExpired () { return mExpired; }
- virtual void OnExpire ();
-
-private:
- box_time_t mExpires;
- bool mExpired;
-};
+class Timer;
// --------------------------------------------------------------------------
//
@@ -50,13 +34,49 @@ class Timers
private:
static std::vector<Timer*>* spTimers;
static void Reschedule();
+
+ static bool sRescheduleNeeded;
+ static void SignalHandler(int iUnused);
public:
static void Init();
static void Cleanup();
static void Add (Timer& rTimer);
static void Remove(Timer& rTimer);
- static void Signal();
+ static void RequestReschedule()
+ {
+ sRescheduleNeeded = true;
+ }
+
+ static void RescheduleIfNeeded()
+ {
+ if (sRescheduleNeeded)
+ {
+ Reschedule();
+ }
+ }
+};
+
+class Timer
+{
+public:
+ Timer(size_t timeoutSecs);
+ virtual ~Timer();
+ Timer(const Timer &);
+ Timer &operator=(const Timer &);
+
+public:
+ box_time_t GetExpiryTime() { return mExpires; }
+ virtual void OnExpire();
+ bool HasExpired()
+ {
+ Timers::RescheduleIfNeeded();
+ return mExpired;
+ }
+
+private:
+ box_time_t mExpires;
+ bool mExpired;
};
#include "MemLeakFindOff.h"
diff --git a/test/common/testcommon.cpp b/test/common/testcommon.cpp
index bac26ff3..ec33b003 100644
--- a/test/common/testcommon.cpp
+++ b/test/common/testcommon.cpp
@@ -238,7 +238,7 @@ int test(int argc, const char *argv[])
CommonException, AssertFailed);
TEST_CHECK_THROWS(Timers::Remove(*(Timer*)NULL),
CommonException, AssertFailed);
- TEST_CHECK_THROWS(Timers::Signal(), CommonException, AssertFailed);
+ // TEST_CHECK_THROWS(Timers::Signal(), CommonException, AssertFailed);
TEST_CHECK_THROWS(Timers::Cleanup(), CommonException, AssertFailed);
// Check that we can initialise the timers