2
\$\begingroup\$

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();
}
\$\endgroup\$
2
\$\begingroup\$

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!

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.