clarify OSystem specification (setTimerCallback() must be 'atomic')

svn-id: r13391

2 files changed, 9 insertions, 10 deletions

diff --git a/common/system.h b/common/system.h
index 73e89155ab..2a8d178e19 100644
--- a/common/system.h
+++ b/common/system.h
@@ -421,9 +421,14 @@ public:
 	 * the previous one. In particular, passing a callback pointer value of 0
 	 * is legal and can be used to clear the current timer callback.
 	 * @see Common::Timer
+	 * @note The implementation of this method must be 'atomic' in the sense
+	 *       that when the method returns, the previously set callback must
+	 *       not be in use anymore (in particular, if timers are implemented
+	 *       via threads, then it must be ensured that the timer thread is
+	 *       not using the old callback function anymore).
 	 *
 	 * @param callback	pointer to the callback. May be 0 to reset the timer
-	 * @param interval	the intervall (in milliseconds) between invocations
+	 * @param interval	the interval (in milliseconds) between invocations
 	 *                  of the callback
 	 */
 	virtual void setTimerCallback(TimerProc callback, int interval) = 0;
diff --git a/common/timer.cpp b/common/timer.cpp
index 1655a2770c..f1e09a9ff0 100644
--- a/common/timer.cpp
+++ b/common/timer.cpp
@@ -51,6 +51,9 @@ Timer::Timer(OSystem *system) :
 }
 
 Timer::~Timer() {
+	// Remove the timer callback. 
+	// Note: backends *must* gurantee that after this method call returns,
+	// the handler is not in use anymore; else race condtions could occurs.
 	_system->setTimerCallback(0, 0);
 
 	{
@@ -62,15 +65,6 @@ Timer::~Timer() {
 		}
 	}
 
-
-	// FIXME: There is still a potential race condition here, depending on how
-	// the system backend implements setTimerCallback: If timers are done using
-	// threads, and if setTimerCallback 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.
 	_system->deleteMutex(_mutex);
 }