1

I try to write a program for physical simulations. I use two threads, one for the calculations and one for the gui. To exchange data between them I use a struct

struct sim_data {  
     int running;  
     int steps;  
    int progress;  
 ...  
};

and include it in the different threads

void *sim(void *args) {  
    struct sim_data *my_data;  
    my_data=(struct sim_data *)args;  
    ...  
}

When setting a value by

my_data->progress=1000;

the data is available in the same thread but not reliably in the second thread. I would guess a chance of 10% when starting the program to read a different value in the second thread then writing in the first one. While the data is written in a loop, I don't think it's a timing problem.

I think this is very strange. Any guess what's going wrong?

Some programmer dude
  • 400,186
  • 35
  • 402
  • 621
Scheddi
  • 11
  • 2
  • 1
    More accurately, a synchronization problem (data race). Have you used proper synchronization primitives (mutexes, semaphores...)? – Mark Garcia Apr 08 '13 at 07:59
  • At first read, it sounds like a race condition – Casey Apr 08 '13 at 08:03
  • @MarkGarcia is right. Take a look at [this](http://en.wikipedia.org/wiki/ABA_problem) – Adri C.S. Apr 08 '13 at 08:03
  • 1
    Multithreaded programming is really hard. You don't just casually "run two things simultaneously". You have to think very carefully about synchronising your code, and the result of this must be visible in the code that deals with inter-thread communication. – Kerrek SB Apr 08 '13 at 08:04
  • 2
    @KerrekSB With greater power comes greater responsibility. :) – Mark Garcia Apr 08 '13 at 08:05
  • If you don't use mutexes/semaphores you should at least declare the struct `volatile` to prevent the compiler from caching its values in registers. – Klas Lindbäck Apr 08 '13 at 08:28
  • @KlasLindbäck: That's terrible advice! – Kerrek SB Apr 08 '13 at 08:31
  • @MarkGarcia: Even the "greater power" is debatable. Someone recently argued convincingly that multithreading is the wrong way round: everything is shared by default, even though you only want very specific data to be shared among threads. Instead, multi-*processing* is no more expensive, and you set up shared data explicitly, so you can't have accidental, implicit races. Something I'm keeping in mind these days. – Kerrek SB Apr 08 '13 at 08:32
  • @Kerrek SB: That would depend. There are cases where `volatile` is sufficient. This could be such a case. I know that I could code the consumer thread (gui) so that it is. Considering the knowledge level of someone asking a question such as this, it is probably not sufficient. – Klas Lindbäck Apr 08 '13 at 08:41
  • 1
    @Scheddi: There is a reasonably good FAQ/Wiki on multithreading: http://stackoverflow.com/questions/2118090/what-are-the-things-to-know-when-diving-into-multi-threaded-programming-in-c. @KlasLindbäck: but the threads are sharing a struct, one member of which, `running`, looks like a flag. You simply can not use `volatile` to guarantee that ordering of reads/writes to the flag have any relationship at all to the reads/writes of any other member of the struct. – Wandering Logic Apr 08 '13 at 12:57
  • @Wandering Logic: I know. Yet there are ways, provided that the struct is declared volatile. – Klas Lindbäck Apr 08 '13 at 13:34
  • this is a producer consumer problem. You can try implementing this pattern it will help you. Also try to use references in your code that might help you reading it – Gabriel Aug 20 '13 at 21:45

2 Answers2

1

The C++11 specification declares a data race case to be any time when one thread writes to a location while a second thread can read or write to the same. It declares that you get undefined behavior when this occurs. In your case, one thread is writing to my_data->progress while another is reading it.

The solution is to use synchronization, such as atomic ints, or locking.

The C++ compiler does very impressive optimizations to make your single threaded programs run faster. For example, it may prove that, in a single threaded world, there is no way to see the 1000 value, and simply choose not to print it.

There's even more ugly cases that are legal. If the compiler knows that you wanted to store 1000, it might choose not to store anything at all, and instead use that memory as a space to 'spill' temporary results into it rather than allocating more space, before eventually storing the 1000. During the mean time, you may read ANY arbitrary value.

For a rather humorous view on the issue: http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong

Cort Ammon
  • 10,221
  • 31
  • 45
0

Simplest solution:

struct sim_data {  
     int running;  
     int steps;  
     atomic<int> progress;  //this will guarantee the data are coherent between two threads
 ...  
};
Gabriel
  • 3,564
  • 1
  • 27
  • 49