Issue with calling a function on multiple threads

Question

Reposting this since my prev question got closed..I've tried to explain it better this time...pls let me know if u need further clarifications:

I have a class A with a function doSomething(int count) Within class A, I want to open 100 threads each of which call the function doSomething() and pass in the count 1-100 in each thread.

Which means...when the 1st thread calls this function, it should call doSomething(1), 2nd thread should call doSomething(2) and so on...

This is what my code looks like:

struct input {
  A* in;
  int count;
};


myFunc(void* data)
{
  input* tP = (input*) data;
  A* obj = tP->in;
  int ct = tP->count;
  obj->doSomething(ct);
}

class A {
  doSomething(int count);
  Thread2doSomething();
}

doSomething(int count)
{
  cout<<"Print value is"<<count;
}

Thread2doSomething()
{
   for (i = 1 to 100)
   {
      input myIN;
      myIN.in = this;
      myIN.count = i;
      beginthreadex(myFunc, &myIN);
   }
 }

I expect that the above code will spawn 100 threads here..each of which will have a new value of count 1,2,3...100 when it calls doSomething();

each invocation of doSomething on a new thread should have a different value of count passed to it - 1,2,3,...to 100.

But that does not happen. The count values passed to it are pretty random...often it gets the same value multiple times...and does not get some values at all.Sometimes the value of count passed to doSomething is the same in all the threads...

the calls looks more like this: doSOmething(4),doSomething(4),doSomething(7),doSomething(10),doSomething(10) and so on.

Hope I've clarified things...pls advise.

Answer 1

There are two problems with your code.

The first problem is in the Thread2doSomething() function: you pass the address of a temporary to beginthreadex(). The temporary goes out of scope when the function exits, and threads are accessing an object which does not exist anymore.

You have two possibilities for fixing this : either you wait for all the threads to complete before exiting from Thread2DoSomething(), so that your stack-allocated object won't get destroyed before threads have done their job, or you allocate the input of each thread on the heap (but don't forget to deallocate them if you use raw pointers).

The second problem is that you are passing the same input to all threads and modify it while they are accessing it (inside the for loop), which introduces a data race. Because of this, your program has Undefined Behavior.

To fix this, you must create a new instance of input for each thread, so that you won't overwrite the same object while threads will try to access it.

Finally, keep in mind that there is no guarantee about the order of execution of your threads. Even if you start them in a certain order, you might still see a permutation of your numbers being printed out rather than the ordered sequence 1..100.

Answer 2

You're re-using one instance of input for all the threads. Of course they grab random data from it. Since you can't change the signature of myFunc() (I presume it's mandated by the thread creation function), you'll have to use dynamic allocation:

Thread2doSomething()
{
   input *myIN;
   for (i = 1 to 100) {
     myIN = new input;
     myIN->in = this;
     myIN->count = i;
     beginthreadex(myFunc, myIN);
   }
}

myFunc(void* data)
{
  std::unique_ptr<input> tP(reinterpret_cast<input*>(data));
  A* obj = tP->in;
  int ct = tP->count;
  obj->doSomething(ct);
}

Using unique_ptr in myFunc() will ensure the object gets deallocated when myFunc() terminates.