Updating global variables from a single worker thread: Do I need mutexes?

It seems that this question gets asked frequently, but I am not coming to any definitive conclusion. I need a little help on determining whether or not I should (or must!) implement locking code when accessing/modifying global variables when I have:

• global variables defined at file scope
• a single "worker" thread reading/writing to global variables
• calls from the main process thread calling accessor functions which return these globals

So the question is, should I be locking access to my global variables with mutexes?

More specifically, I am writing a C++ library which uses a webcam to track objects on a page of paper -- computer vision is CPU intensive, so performance is critical. I have a single worker thread which is spun off in an Open() function. This thread handles all of the object tracking. It is terminated (indirectly w/global flag) when a Close() function is called.

It feels like I'm just asking for memory corruption, but I have observed no deadlock issues nor have I experienced any bad values returned from th开发者_如何学JAVAese accessor functions. And after several hours of research, the general impression I get is, "Meh, probably. Whatever. Have fun with that." If I indeed should be using mutexes, why I have not experienced any problems yet?

Here is an over-simplification on my current program:

// *********** lib.h ***********
// Structure definitions
struct Pointer
{
  int x, y;
};
// more...

// API functions
Pointer GetPointer();
void Start();
void Stop();
// more...

The implementation looks like this...

// *********** lib.cpp ***********
// Globals
Pointer p1;
bool isRunning = false;
HANDLE hWorkerThread;
// more...

// API functions
Pointer GetPointer()
{
  // NOTE: my current implementation is actually returning a pointer to the
  // global object in memory, not a copy of it, like below...

  // Return copy of pointer data
  return p1;
}

// more "getters"...

void Open()
{
  // Create worker thread -- continues until Close() is called by API user
  hWorkerThread = CreateThread(NULL, 0, DoWork, NULL, 0, NULL);
}

void Close()
{
  isRunning = false;

  // Wait for the thread to close nicely or else you WILL get nasty
  // deadlock issues on close
  WaitForSingleObject(hWorkerThread, INFINITE);
}

DWORD WINAPI DoWork(LPVOID lpParam)
{
  while (isRunning)
  {
    // do work, including updating 'p1' about 10 times per sec
  }

  return 0;
}

Finally, this code is being called from an external executable. Something like this (pseudocode):

// *********** main.cpp ***********
int main()
{
  Open();

  while ( <esc not pressed> )
  {
    Pointer p = GetPointer();
    <wait 50ms or so>
  }
  Close();
}

Is there perhaps a different approach I should be taking? This non-issue issue is driving me nuts today :-/ I need to ensure this library is stable and returning accurate values. Any insight would be greatly appreciated.

Thanks

If only one thread access an object (both read and write) then no locks are required.

If an object is read only then no locks are required. (Assuming you can guarantee that only one thread access the object during construction).

If any thread writes (changes the state) of an object. If there are other threads that access that object then ALL accesses (both read and write) must be locked. Though you may use read locks that allow multiple readers. But write operations must be exclusive and no readers can access the object while the state is being changed.

I suppose it depends on what you are doing in your DoWork() function. Let's assume it writes a point value to p1. At the very least you have the following possibility of a race condition that will return invalid results to the main thread:

Suppose the worker thread wants to update the value of p1. For example, lets change the value of p1 from (A, B) to (C, D). This will involve at least two operations, store C in x and store D in y. If the main thread decides to read the value of p1 in the GetPointer() function, it must perform at least two operations also, load value for x and load value for y. If the sequence of operations is:

1. update thread: store C
2. main thread: load x (main thread receives C)
3. main thread: load y (main thread receives B)
4. update thread: store D

The main thread will get the point (C, B), which is not correct.

This particular problem is not a good use of threads, since the main thread isn't doing any real work. I would use a single thread, and an API like WaitForMultipleObjectsEx which allows you to simultaneously wait for input from the keyboard stdin handle, an I/O event from the camera, and a timeout value.

You won't get a deadlock, but you may see some occasional bad value with an extremely low probability: since reading and writing take fractions of a nanosecond, and you only read the variable 50 times per second, the chance of a collision is something like 1 in 50 million.

If this is happening on an Intel 64, "Pointer" is aligned to a 8 byte boundary, and it is read and written in one operation (all 8 bytes with one assembly instruction), then accesses are atomic and you don't need a mutex.

If either of those conditions are not satisfied, there's a possibility that the reader will get bad data.

I'd put a mutex just to be on the safe side, since it's only going to be used 50 times a second and it's not going to be a performance issue.

The situation's pretty clear cut - the reader may not see updates until something triggers synchronisation (a mutex, memory barrier, atomic operation...). Many things processes do implicitly trigger such synchronisation - e.g. external function calls (for reason's explained the the Usenet threading FAQ (http://www.lambdacs.com/cpt/FAQ.html) - see Dave Butenhof's answer re need for volatile, so if your code is dealing in values that are small enough that they can't be half-written (e.g. numbers rather than strings, fixed address rather than dynamic (re)allocations) then it can limp along without explicit syncs.

If your idea of performance is getting more loops through your write code, then you'll get a nicer number if you leave out the synchronisation. But if you're interested in minimising the average and worst-case latency, and how many distinct updates the reader cam actually see, then you should do synchronisation from the writer.

You may not be seeing problems because of the nature of the information in Pointer. If it is tracking coordinates of some object that is not moving very fast, and the position is updated during a read, then the coordinates might be a "little off", but not enough to notice.

For instance, assume that after an update, p.x is 100, and p.y is 100. The object your are tracking moves a bit, so after the next update, p.x is 102 and p.y is 102. If you happen to read in the middle of this update, after x is updated but before y is updated, you will end getting a pointer value of p.x as 102, and p.y as 100.