C++ thread safe class, not working as expected

Question

I am trying to implement a thread-safe class. I put lock_guard for setter and getter for each member variable.

#include <iostream>
#include <omp.h>
#include <mutex>
#include <vector>
#include <map>
#include <string>

class B {
  std::mutex m_mutex;
  std::vector<int> m_vec;
  std::map<int,std::string> m_map;

public:

  void insertInVec(int x) {
    std::lock_guard<std::mutex> lock(m_mutex);
    m_vec.push_back(x);
  }

  void insertInMap(int x, const std::string& s) {
    std::lock_guard<std::mutex> lock(m_mutex);
    m_map[x] = s;
  }

  const std::string& getValAtKey(int k) {
    std::lock_guard<std::mutex> lock(m_mutex);
    return m_map[k];
  }

  int getValAtIdx(int i) {
    std::lock_guard<std::mutex> lock(m_mutex);
    return m_vec[i];
  }
};

int main() {
  B b;

  #pragma omp parallel for num_threads(4)
  for (int i = 0; i < 100000; i++) {
    b.insertInVec(i);
    b.insertInMap(i, std::to_string(i));
  }

  std::cout << b.getValAtKey(20) << std::endl;
  std::cout << b.getValAtIdx(20) << std::endl;

  return 0;
}

when I run this code, the output from map is correct but output from vector is garbage. I get o/p as

20
50008

Ofcourse the second o/p changes at each run.
1. what is wrong with this code? (I also have to consider the scenario, where there can be multiple instances of class B, running at multiple threads)
2. For each member variable do I need separate mutex variable? like

std::mutex vec_mutex;
std::mutex map_mutex;

not the problem here, but you forgot the special member functions that the compiler generates for you. They will not use your getters/setters and will not be thread safe — 463035818_is_not_an_ai, May 15 '20 at 19:39
@idclev463035818 Oh, I really do not know about them. Could you elaborate please? — Sourabh, May 15 '20 at 19:43
btw I ignored your second question in my answer. Please only one question per question — 463035818_is_not_an_ai, May 15 '20 at 19:55

score 3 · Answer 1 · answered May 15 '20 at 19:54

I don't understand why you think that the output is garbage. Your loop is executed in 4 threads, so a possible sharing of the tasks could be:

Thread 1: 0 <= i < 25000
Thread 2: 25000 <= i < 50000
Thread 3: 50000 <= i < 75000
Thread 4: 75000 <= i < 100000

Each thread is doing a push_back of i to the vector. If now thread 1 starts and writes "0, 1, 2, 3, 4, 5, 6, 7, 8, 9" and then thread 1 writes "25000, 25001" and then thread 3 writes "50000, 50001, 50002, 50003, 50004, 50005, 50006, 50007, 50008". So you will end up with value 50008 at index 20. Of course other thread interleaving is also possible and you might also see values like for example 25003 or 75004.

463035818_is_not_an_ai · Accepted Answer · 2020-05-15T20:07:00.117

The output you see is fine, only your expectations are off.

You add elements into the vector via:

void insertInVec(int x) {
  std::lock_guard<std::mutex> lock(m_mutex);
  m_vec.push_back(x);
}

and then retrive them via:

int getValAtIdx(int i) {
  std::lock_guard<std::mutex> lock(m_mutex);
  return m_vec[i];
}

Because the loop is executed in parallel, there is no guarantee that the values are inserted in the order you expect. Whichever thread first grabs the mutex will insert values first. If you wanted to insert the values in some specified order you would need to resize the vector upfront and then use something along the line of:

void setInVecAtIndex(int x,size_t index) {
  std::lock_guard<std::mutex> lock(m_mutex);  // maybe not needed, see PS
  m_vec[index] = x;
}

So this isn't the problem with your code. However, I see two problems:

getValAtKey returns a reference to the value in the map. It is a const reference, but that does not prevent somebody else to modify the value via a call to insertInMap. Returning a reference here defeats the purpose of using the lock. Using that reference is not thread safe! To make it thread safe you need to return a copy.
You forgot to protect the compiler generated methods. For an overview, see What are all the member-functions created by compiler for a class? Does that happen all the time?. The compiler generated methods will not use your getters and setter, hence are not thread-safe by default. You should either define them yourself or delete them (see also rule of 3/5).

PS: Accessing different elements in a vector from different threads needs no synchronization. As long as you do not resize the vector and only access different elements you do not need the mutex if you can ensure that no two threads access the same index.

C++ thread safe class, not working as expected

2 Answers2