Timer now uses a mutex, which should make it thread safe (it wasn't before, particuarly bad if timers are implemented via threads), plus this should help in fixing race conditions in classes using class Timer

svn-id: r8722

3 files changed, 62 insertions, 86 deletions

diff --git a/common/engine.cpp b/common/engine.cpp
index d68b9e46da..f89793b14a 100644
--- a/common/engine.cpp
+++ b/common/engine.cpp
@@ -41,7 +41,6 @@ Engine::Engine(GameDetector *detector, OSystem *syst)
 	g_system = _system;
 	g_mixer = _mixer;
 	_timer = new Timer(this);
-	_timer->init();
 }
 
 Engine::~Engine() {
diff --git a/common/timer.cpp b/common/timer.cpp
index a0b90232a7..126e86e573 100644
--- a/common/timer.cpp
+++ b/common/timer.cpp
@@ -26,17 +26,49 @@
 
 static Timer *g_timer = NULL;
 
-Timer::Timer(Engine * engine) {
-	memset(this,0,sizeof(Timer)); //palmos
-	
-	_initialized = false;
-	_timerRunning = false;
-	_engine = engine;
+Timer::Timer(Engine * engine) :
+	_engine(engine),
+	_mutex(0),
+	_timerHandler(0),
+	_lastTime(0) {
+
+	_mutex = _engine->_system->create_mutex();
+
 	g_timer = this;
+	
+	for (int i = 0; i < MAX_TIMERS; i++) {
+		_timerSlots[i].procedure = NULL;
+		_timerSlots[i].interval = 0;
+		_timerSlots[i].counter = 0;
+	}
+
+	_thisTime = _engine->_system->get_msecs();
+
+	// Set the timer last, after everything has been initialised
+	_engine->_system->set_timer(10, &timer_handler);
+
 }
 
 Timer::~Timer() {
-	release();
+	_engine->_system->set_timer(0, NULL);
+
+	_engine->_system->lock_mutex(_mutex);
+	for (int i = 0; i < MAX_TIMERS; i++) {
+		_timerSlots[i].procedure = NULL;
+		_timerSlots[i].interval = 0;
+		_timerSlots[i].counter = 0;
+	}
+	_engine->_system->unlock_mutex(_mutex);
+
+	// FIXME: There is still a potential race condition here, depending on how
+	// the system backend implements set_timer: If timers are done using
+	// threads, and if set_timer does *not* gurantee that after it terminates
+	// that timer thread is not run anymore, we are fine. However, if the timer
+	// is still running in parallel to this destructor, then it might be that
+	// it is still waiting for the _mutex. So, again depending on the backend,
+	// we might end up unlocking the mutex then immediately deleting it, while
+	// the timer thread is about to lock it.
+	_engine->_system->delete_mutex(_mutex);
 }
 
 int Timer::timer_handler(int t) {
@@ -48,77 +80,32 @@ int Timer::timer_handler(int t) {
 int Timer::handler(int t) {
 	uint32 interval, l;
 
-	if (_timerRunning) {
-		_lastTime = _thisTime;
-		_thisTime = _engine->_system->get_msecs();
-		interval = 1000 * (_thisTime - _lastTime);
-
-		for (l = 0; l < MAX_TIMERS; l++) {
-			if ((_timerSlots[l].procedure) && (_timerSlots[l].interval > 0)) {
-				_timerSlots[l].counter -= interval;
-				if (_timerSlots[l].counter <= 0) {
-					_timerSlots[l].counter += _timerSlots[l].interval;
-					_timerSlots[l].procedure (_engine);
-				}
-			}
-		}
-	}
+	_engine->_system->lock_mutex(_mutex);
 
-	return t;
-}
-
-bool Timer::init() {
-	int32 l;
-
-	if (_engine->_system == NULL) {
-		warning("Timer: OSystem not initialized!");
-		return false;
-	}
-
-	if (_initialized) 
-		return true;
+	_lastTime = _thisTime;
+	_thisTime = _engine->_system->get_msecs();
+	interval = 1000 * (_thisTime - _lastTime);
 
 	for (l = 0; l < MAX_TIMERS; l++) {
-		_timerSlots[l].procedure = NULL;
-		_timerSlots[l].interval = 0;
-		_timerSlots[l].counter = 0;
+		if ((_timerSlots[l].procedure) && (_timerSlots[l].interval > 0)) {
+			_timerSlots[l].counter -= interval;
+			if (_timerSlots[l].counter <= 0) {
+				_timerSlots[l].counter += _timerSlots[l].interval;
+				_timerSlots[l].procedure (_engine);
+			}
+		}
 	}
 
-	_thisTime = _engine->_system->get_msecs();
-	_engine->_system->set_timer(10, &timer_handler);
-
-	_timerRunning = true;
-	_initialized = true;
-	return true;
-}
-
-void Timer::release() {
-	int32 l;
-
-	if (_initialized == false) 
-		return;
+	_engine->_system->unlock_mutex(_mutex);
 
-	_timerRunning = false;
-	_engine->_system->set_timer(0, NULL);
-	_initialized = false;
-
-	for (l = 0; l < MAX_TIMERS; l++) {
-		_timerSlots[l].procedure = NULL;
-		_timerSlots[l].interval = 0;
-		_timerSlots[l].counter = 0;
-	}
+	return t;
 }
 
 bool Timer::installProcedure (TimerProc procedure, int32 interval) {
 	int32 l;
 	bool found = false;
 
-	if (_initialized == false) {
-		warning("Timer: is not initialized!");
-		return false;
-	}
-
-	_timerRunning = false;
+	_engine->_system->lock_mutex(_mutex);
 	for (l = 0; l < MAX_TIMERS; l++) {
 		if (!_timerSlots[l].procedure) {
 			_timerSlots[l].procedure = procedure;
@@ -128,25 +115,18 @@ bool Timer::installProcedure (TimerProc procedure, int32 interval) {
 			break;
 		}
 	}
+	_engine->_system->unlock_mutex(_mutex);
 
-	_timerRunning = true;
-	if (!found)	{
-		warning("Can't find free slot!");
-		return false;
-	}
+	if (!found)
+		warning("Couldn't find free timer slot!");
 
-	return true;
+	return found;
 }
 
 void Timer::releaseProcedure (TimerProc procedure) {
 	int32 l;
 
-	if (_initialized == false) {
-		warning("Timer: is not initialized!");
-		return;
-	}
-
-	_timerRunning = false;
+	_engine->_system->lock_mutex(_mutex);
 	for (l = 0; l < MAX_TIMERS; l++) {
 		if (_timerSlots[l].procedure == procedure) {
 			_timerSlots[l].procedure = 0;
@@ -154,7 +134,7 @@ void Timer::releaseProcedure (TimerProc procedure) {
 			_timerSlots[l].counter = 0;
 		}
 	}
-	_timerRunning = true;
+	_engine->_system->unlock_mutex(_mutex);
 }
 
 #endif
diff --git a/common/timer.h b/common/timer.h
index 0ed416679d..78877963f4 100644
--- a/common/timer.h
+++ b/common/timer.h
@@ -36,8 +36,7 @@ class Timer {
 
 private:
 	Engine *_engine;
-	bool _initialized;
-	bool _timerRunning;
+	void *_mutex;
 	void *_timerHandler;
 	int32 _thisTime;
 	int32 _lastTime;
@@ -49,11 +48,9 @@ private:
 	} _timerSlots[MAX_TIMERS];
 
 public:
-	  Timer(Engine *engine);
-	 ~Timer();
+	Timer(Engine *engine);
+	~Timer();
 
-	bool init();
-	void release();
 	bool installProcedure(TimerProc procedure, int32 interval);
 	void releaseProcedure(TimerProc procedure);