C++ Threading Question

Question

I have an object Foo which has a global variable, Time currentTime

Foo has two methods which are called from different threads.

update()
{
    currentTime = currentTime + timeDelay;
}

restart(Time newTime)
{
    currentTime = newTime;
}

I am seeing behavior on a restart, the time changes correctly and other times where currentTime does not seem to reset (or it does reset but then update sets it back somehow.

The method update is called roughly every second or so while restart only occurs when a user initiates a restart event (presses a button). I think this is threading timing issue, any suggestions or comments on what is happening are welcome.

Answer 1

You certainly have a race condition here. The most straitforward solution is to protect the use of the shared variable currentTime by using a lock. I am using the Boost.Threads mutex class here:

class Foo
{
  boost::mutex _access;
  update()
  {
    boost::mutex::scoped_lock lock(_access);
    currentTime = currentTime + timeDelay;
  }

  restart(Time newTime)
  {
    boost::mutex::scoped_lock lock(_access);
    currentTime = newTime;
  }
};

Answer 2

Add some printfs in each function to create a log of what is happening.

For example, what do you expect to happen if update() was executed in another thread immediately after "currentTime=newTime;"? - or even worse - during the assignment in that line.

After you've done that, have a look at: http://en.wikipedia.org/wiki/Mutual_exclusion

Answer 3

Accessing the same variable from two threads (as you are doing) requires some sort of syncronization. Use a mutex to guarenteed that only one thread is accessing that variable at a time, ie:

update()
{
    // Lock mutex
    currentTime = currentTime + timeDelay;
    // unlock mutex
}
// Same idea for restart()

The problem is that access to the same variable, without a syncronization primitive such as a mutex, is problematic with threads. Say update() reads currentTime, does the addition, but before it can store the result, we switch threads, restart() does it's thing. Now we switch back to update(), which writes the (now invalid) result of the addition back into currentTime, overwriting restart()'s work. Mutex's prevent this by allowing you to guarentee an operation is atomic. Google for some tutorials - you'll need to know a lot of other things, such as deadlocks.

How exactly you create/lock a mutex depends on your OS/what libraries you want to use. Native solutions are pthreads on *nix systems, Critical sections on Win32. (pthreads implementations exist for Win32) The Boost library also has a threads section.

Answer 4

Thread 1 calls update, gets a copy of currentTime and saves it in its thread local memory. Thread 2 calls restart, sets currentTime to newTime. Thread 2 finishes. Thread 1 continues, reassigns currentTime to the currentTime in its thread local memory (which is the old value of currentTime prior to your restart call) + the timeDelay. Thread 1 now finishes.

Hence your restart will have failed. There are many other situations which can occur resulting in unexpected behavior. Always synchronize variables shared amongst a different threads, to avoid such problems.

You should use a mutex suggested by others.

Answer 5

The use of a mutex feels way too heavy to me in this situation. I'd use InterlockedExchange and InterlockedAdd instead.

Answer 6

You are having a race condition as there is no locking while entering the critical region and each are updating the current_time. Each thread has a memory, when a thread needs to obtain something from the shared memory it copies this into it's local memory. The first step is to obtain a lock first then clearing it's memory guaranteeing that the variable will be loaded from the shared memory. Now operate in the critical region and once done unlock the critical region guaranteeing the local variable will be written out to the shared memory. Since you do not have a lock you can not predict what is going to happen.

The use of a mutex would be what you need for your case as there is only one key for current_time variable. The other type of locking is a semaphore which allows for multiple keys.

Answer 7

If currentTime is realy a global variable like you say it is, you'll need a global mutex to protect the variable. ( PTHREAD_MUTEX_INITIALIZER or BOOST.call_once construction )

In that case the BOOST.Threads example is incorrect because two instances of the Foo class living in different threads will have different instances of the _access mutex (I realy do not prefer the _ prefex!) and will lock their own instance and do not protect the currentTime variable.

If currentTime is a instance variable the BOOST.Threads example is correct.