From cff19bd8c52b58095aaf2aa35f38673c29f1aa88 Mon Sep 17 00:00:00 2001 From: Chris Wilson Date: Sun, 3 Dec 2006 13:58:14 +0000 Subject: Fixed a race condition caused by rescheduling in signal handler (refs #3, refs #9) --- lib/common/Timer.cpp | 84 ++++++++++++++++++++++++++++------------------ lib/common/Timer.h | 56 +++++++++++++++++++++---------- test/common/testcommon.cpp | 2 +- 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* 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; } @@ -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::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::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* 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 -- cgit v1.2.3