Still having race condition with boost::mutex

Question

I am trying an example, which causes race condition to apply the mutex. However, even with the mutex, it still happens. What's wrong? Here is my code:

#include <iostream>
#include <boost/thread.hpp>
#include <vector>
using namespace std;
class Soldier
{
private:
  boost::thread m_Thread;
public:
  static int count , moneySpent;
  static boost::mutex soldierMutex;   
  Soldier(){}
  void start(int cost)
  {
    m_Thread = boost::thread(&Soldier::process, this,cost);
  }

  void process(int cost)
  {
    {
    boost::mutex::scoped_lock lock(soldierMutex);
    //soldierMutex.lock();
    int tmp = count;
    ++tmp;
    count = tmp;
    tmp = moneySpent;
    tmp += cost;
    moneySpent = tmp;  
   // soldierMutex.unlock();
    }
  }  

  void join()
  {
    m_Thread.join();
  }
};

int Soldier::count, Soldier::moneySpent;
boost::mutex Soldier::soldierMutex;

int main()
{
  Soldier s1,s2,s3;
  s1.start(20);
  s2.start(30);
  s3.start(40);
  s1.join();
  s2.join();
  s3.join();
  for (int i = 0; i < 100; ++i)
    {
      Soldier s;
      s.start(30);
    }
  cout << "Total soldier: " << Soldier::count << '\n';
  cout << "Money spent: " << Soldier::moneySpent << '\n';
}

Answer 1

It looks like you're not waiting for the threads started in the loop to finish. Change the loop to:

 for (int i = 0; i < 100; ++i)
 {
   Soldier s;
   s.start(30);
   s.join();
 }

edit to explain further

The problem you saw was that the values printed out were wrong, so you assumed there was a race condition in the threads. The race in fact was when you printed the values - they were printed while not all the threads had a chance to execute

Answer 2

Based on this and your previous post (were it does not seem you have read all the answers yet). What you are looking for is some form of synchronization point to prevent the main() thread from exiting the application (because when the main thread exits the application all the children thread die).

This is why you call join() all the time to prevent the main() thread from exiting until the thread has exited. As a result of your usage though your loop of threads is not parallel and each thread is run in sequence to completion (so no real point in using the thread).

Note: join() like in Java waits for the thread to complete. It does not start the thread.

A quick look at the boost documentation suggests what you are looking for is a thread group which will allow you to wait for all threads in the group to complete before exiting.

//No compiler so this is untested.
// But it should look something like this.
// Note 2: I have not used boost::threads much.
int main()
{
    boost::thread_group               group;

    boost::ptr_vector<boost::thread>  threads;

    for(int loop = 0; loop < 100; ++loop)
    {
        // Create an object.
        // With the function to make it start. Store the thread in a vector
        threads.push_back(new boost::thread(<Function To Call>));

        // Add the thread to the group.
        group.add(threads.back());
    }

    // Make sure main does not exit before all the threads have completed.
    group.join_all();
}

If we go back to your example and retrofit your Soldier class:

int main()
{
  boost::thread batallion;

  // Make all the soldiers part of a group.
  // When you start the thread make the thread join the group.
  Soldier s1(batallion);
  Soldier s2(batallion);
  Soldier s3(batallion);

  s1.start(20);
  s2.start(30);
  s3.start(40);

  // Create 100 soldiers outside the loo
  std::vector<Soldier>  lotsOfSoldiers;
  lotsOfSoldiers.reserve(100);  // to prevent reallocation in the loop.
                                // Because you are using objects we need to 
                                // prevent copying of them after the thread starts.

  for (int i = 0; i < 100; ++i)
  {
      lotsOfSoldiers.push_back(Solder(batallion));
      lotsOfSoldiers.back().start(30);
  }

  // Print out values while threads are still running
  // Note you may get here before any thread.
  cout << "Total soldier: " << Soldier::count << '\n';
  cout << "Money spent: " << Soldier::moneySpent << '\n';

  batallion.join_all();

  // Print out values when all threads are finished.
  cout << "Total soldier: " << Soldier::count << '\n';
  cout << "Money spent: " << Soldier::moneySpent << '\n';
}