CODEWITHSUNDEEP
c++thread-safety
user2912091902102109
user2912091902102109

Reputation: 39

Thread-safe array counter C++

I have a vector that contains more than 1000 elements. I want to take every element and make an HTTP request with that element, then give me the results without multi-threading. It will be so slow so I made multi-threading that takes like 100 elements to check every time.

My problem is, the counter isn't working, as I planned to work to the counter reach the maximum value without checking all the elements.

This is my code snippet:

for(int i=0; i<threads; i++){
    threadlist.push_back(thread([&]{
        while(true){
            mutex lock;
            lock.lock();
            if(counter >= Files::getUsers().size()){
                exit(0);
            }else {
                counter++;
            }
            lock.unlock();

Upvotes: 1

Views: 465

Answers (4)

Micka&#235;l C. Guimar&#227;es
Micka&#235;l C. Guimar&#227;es

Reputation: 1112

First divide the work to be done in separate vectors for each thread

First prepare the work to be done for each thread, such that each thread will have its own separate workload :

   const int nThreads = NUMBER_OF_THREADS;
   const int sizePerThread = Files::getUsers().size() / nThreads;
   std::vector<std::thread> threadlist;

   // Fills index limits for each thread
   std::vector<int> threadLimitIndex;
   for (int i=0; i<nThreads; ++i)
     threadLimitIndex.push_back(i * sizePerThread);
   threadLimitIndex.push_back(Files::getUsers().size());

Then use the limits for each thread to have them work on their own dataset :

   // Does the calculation
   for (int i=0; i<nThreads; ++i)
   {
     threadlist.push_back(thread([&threadLimitIndex]{
        for (int myValue=threadLimitIndex[i]; myValue<threadLimitIndex[i+1]; ++myValue)
        {
           // Do calculation with myValue, no other thread will have the same
        }
       }
     ));
   }

No need for complicated control code ;-)

CAVEAT : this is a simple way to separate the work and assumes that work for each value is roughly the same. If the work to be done varies greatly for each value, some threads will finish earlier and stall while remaining ones will still have work to do, which is not optimal. To guarantee that all threads will work until the end in all cases, you need to implement a work queue.

Upvotes: 1

PeterT
PeterT

Reputation: 8284

You can't use a seperate mutex per thread. You can either use one mutex across all threads (or some other synchronization primitive), or you can use an atomic value in this case.

With a mutex:

  std::vector<std::thread> threadlist;
  int counter = 0;
  std::mutex m;
  int num_threads = 8;
  for (int i = 0; i < num_threads; i++) {
      threadlist.push_back(thread([&]{
      while (true) {
        int myValue;
        {  // keep critical section minimal to avoid lock contention as much as possible
          std::lock_guard<std::mutex> lock(m);
          myValue = counter++;
        }
        if (myValue >= Files::getUsers().size()) {
          return;
        }
        //do calculation with myValue, no other thread will have the same
      }
     }));
  }

with atomics

  std::vector<std::thread> threadlist;
  std::atomic<int> counter {0};
  int num_threads = 8;
  for (int i = 0; i < num_threads; i++) {
      threadlist.push_back(thread([&]{
      while (true) {
        int myValue = counter.fetch_add(1);
        if (myValue >= Files::getUsers().size()) {
          return;
        }
        //do calculation with myValue, no other thread will have the same
      }
     }));
  }

Upvotes: 1

NathanOliver
NathanOliver

Reputation: 180415

You are defining lock inside the loop inside the thread, meaning each iteration in each thread is going to have its own mutex, and so you don't get any thread synchronization to protect counter. This gives you a data race, which is undefined behavior.

What you need to do is define lock outside of the for loop, like you do with counter, and then capture the mutex so all threads share it.

Alternatively, you could just make counter a std::atomic<whatever_integer_type> and then you don't even need a mutex, as counter will take care of synchronizing itself.

Upvotes: 3

eerorika
eerorika

Reputation: 238301

You seem to be using separate mutex in each thread. You need to use the same mutex in each thread in order for it to do any synchronisation.

Upvotes: 3

Related Questions