Sender/Receiver threads using std::unique_lock and std::condition_variable

Question

The code below is a sender/receiver setup in C++ where the sender is in one thread, the receiver is in another, and the data being sent/received is "shared" (global). The code uses the <thread> header, std::unique_lock, and std::condition_variable.

I would love a critique on everything below ConditionVariable.notify_one(); in the Receiver function and everything below ConditionVariable.notify_one(); in the Sender function.

Regarding those final line(s) of Receiver and Sender: I'm having some doubts around the effects of blocking that happens upon lock and wait. Also, at the bottom of each of the two functions, I'm not entirely sure about the need for lock... since, as soon as the loop iteration ends, the destructor of unique_lock will immediately do an unlock.

#include <mutex>
#include <thread>
#include <string>

// Declaration-only for brevity. Assume this returns the data that `Sender` will send.
std::string GenerateMyString();

// Global variables for threads (we'll have two threads, one for `Sender` and
// one for `Receiver`
std::mutex Mutex;
std::string DataGeneratedWithinSender;
bool Free = false;
std::condition_variable ConditionVariable;

// `Sender` and `Receiver` functions will each run in their own thread.
void Sender() {
  for (int i = 0; i < 5; ++i) {
    std::unique_lock<std::mutex> unq_lck{Mutex};
    DataGeneratedWithinSender = GenerateMyString();
    std::this_thread::sleep_for(std::chrono::milliseconds(100));
    Free = true;
    unq_lck.unlock();
    ConditionVariable.notify_one();
    unq_lck.lock();
    ConditionVariable.wait(unq_lck, []() { return !Free; });
  }
}

void Receiver() {
  std::string data;
  for (int i = 0; i < 5; ++i) {
    std::unique_lock<std::mutex> unq_lck{Mutex};
    ConditionVariable.wait(unq_lck, []() { return Free; });
    data = DataGeneratedWithinSender;
    Free = false;
    unq_lck.unlock();
    ConditionVariable.notify_one();
    // *** DO SOMETHING WITH `data` HERE ***
    // This `Receiver` function is now ready for the next piece of data to be sent to it.
    unq_lck.lock();
  }
}

int main() {
  std::thread t_1(Receiver);
  std::thread t_2(Sender);
  t_1.join();
  t_2.join();
  return 0;
}

Answer 1

Avoid manual locking and unlocking

It's great that you use std::unique_lock, and using that to call lock() and unlock() is much safer than if you were to lock and unlock a std::mutex directly. However, it can be avoided by using scopes to cause unlocking to happen at the right point. Furthermore, if possible try to reduce the number of times you need to unlock and relock a std::unique_lock. For example, the sender could be rewritten like so:

void Sender() {
  for (int i = 0; i < 5; ++i) {
    {
      std::unique_lock<std::mutex> unq_lck{Mutex};
      ConditionVariable.wait(unq_lck, []() { return !Free; });
      DataGeneratedWithinSender = GenerateMyString();
      std::this_thread::sleep_for(std::chrono::milliseconds(100));
      Free = true;
    }

    ConditionVariable.notify_one();
  }
}

Generate the data without holding the lock

You call GenerateMyString() while the lock is held. But what if generating the string takes a long time? Just like the receiver copies DataGeneratedWithinSender into data, so it can then unlock and process the data without blocking the sender, you can do the same in the sender itself:

void Sender() {
  for (int i = 0; i < 5; ++i) {
    auto data = GenerateMyString();
    std::this_thread::sleep_for(std::chrono::milliseconds(100));

    {
      std::unique_lock<std::mutex> unq_lck{Mutex};
      ConditionVariable.wait(unq_lck, []() { return !Free; });
      DataGeneratedWithinSender = data;
      Free = true;
    }

    ConditionVariable.notify_one();
  }
}

std::move() data instead of copying it

You are copying the strings around, but this takes some time and might involve allocating memory. You can avoid that by invoking the move assignment operator of std::string using std::move():

DataGeneratedWithinSender = std::move(data);
...
data = std::move(DataGeneratedWithinSender);

Here is sender and receiver after incorporating everything from above:

void Sender() {
  for (int i = 0; i < 5; ++i) {
    auto data = GenerateMyString();
    std::this_thread::sleep_for(std::chrono::milliseconds(100));

    {
      std::unique_lock<std::mutex> unq_lck{Mutex};
      ConditionVariable.wait(unq_lck, []() { return !Free; });
      DataGeneratedWithinSender = std::move(data);
      Free = true;
    }

    ConditionVariable.notify_one();
  }
}

void Receiver() {
  std::string data;

  for (int i = 0; i < 5; ++i) {
    {
      std::unique_lock<std::mutex> unq_lck{Mutex};
      ConditionVariable.wait(unq_lck, []() { return Free; });
      data = std::move(DataGeneratedWithinSender);
      Free = false;
    }

    ConditionVariable.notify_one();
    // Presumably receiver does something with `data` here.
  }
}