Sharing data with another thread using lock_guard

Question

I wrote a updater_thread() and a reader_thread(), and used lock_guard to protect a global instance.

Question

1. I've never used lock_guard. In this code, is lock_guard used properly?

2. Is this code thread-safe?

Code

#include <iostream>
#include <thread>
#include <mutex>
#include <iterator>
#include <algorithm>
#include <vector>
#include <initializer_list>
#include <unistd.h>
class C {
 std::vector<int> v;
 std::mutex mutex_;
public:
 C() {}
 void Update(const std::vector<int> &new_v) {
 std::lock_guard<std::mutex> lock(mutex_);
 v = new_v;
 }
 bool Check(const int x){
 std::lock_guard<std::mutex> lock(mutex_);
 return std::find(v.begin(), v.end(), x) != v.end();
 }
 /* dump() is not essential */
 void dump() {
 std::lock_guard<std::mutex> lock(mutex_);
 std::cout << "dump: ";
 std::copy(v.begin(), v.end(),
 std::ostream_iterator<int>(std::cout, " "));
 std::cout << "\n";
 }
};
// create an instance gloablly
C g_c;
void updater_thread() {
 std::cout << "start updater_thread\n";
 while (true) {
 std::vector<int> v;
 for (int i = 0; i < 10; i++) {
 v.push_back(rand() % 10 + 1);
 }
 std::sort(v.begin(), v.end());
 g_c.Update(v);
 std::cout << "updated!!!\n";
 std::copy(v.begin(), v.end(),
 std::ostream_iterator<int>(std::cout, " "));
 std::cout << "\n";
 sleep(5);
 }
}
void reader_thread() {
 std::vector<int> v {1,2,3,5};
 while (true) {
 std::cout << "check: non-exist item: ";
 for (int i = 0; i < v.size(); i++) {
 if (!g_c.Check(v[i])){
 std::cout << v[i] << " ";
 }
 }
 std::cout << "\n";
 sleep(1);
 }
}
int main() {
 std::thread t_up(updater_thread);
 std::thread t_r(reader_thread);
 t_up.join();
 t_r.join();
}

Answer 1

Yes, that all looks correct to me!

You don't use anything from <unistd.h> (non-standard) or <initializer_list> (obscure), so I recommend removing those includes.

---

You sigil the mutex_ data member with an underscore, but you don't sigil the v member. I recommend being consistent:

std::mutex mutex_;
std::vector<int> v_;

Personally I would spell the member's name mtx_, but that's just a personal habit; I don't know if that naming convention is widespread. (cv_ for a condition variable certainly is, though!)

---

Consider that your Check and dump methods don't need to mutate the object, so they should be declared const. This means that your mutex_ data member will need to be declared mutable so that you can still lock and unlock it inside your const member functions.

Also, consider picking a capitalization rule and sticking to it. Why Check and Update but dump (not Dump)?

Check's parameter x is marked const but that marking serves no purpose: eliminate it. (Const is a contract.)

---

for (int i = 0; i < v.size(); i++) {
 if (!g_c.Check(v[i])){
 std::cout << v[i] << " ";
 }
}

This could be rewritten more concisely as

for (int vi : v) {
 if (!g_c.Check(vi)) {
 std::cout << vi << " ";
 }
}

Your multithreading stuff all looks great!