HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Thread test mutex implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
implementationmutexthreadtest

Problem

I am trying to make a thread test in which 4 threads are all trying to set a bool. Only one should be allowed to set it. After the first thread has set the bool (m_isRunning) to true, no other threads should set it to true because of the check isRunning().

This implementation works, but I wonder if this is the best/most efficient approach to this common situation.

#include
#include
#include

std::mutex mut;
bool m_running = false;

bool isRunning()
{
  return m_running;
}

void setRunning(bool running)
{
  m_running = running;
}

void func1(bool isSet)
{
  mut.lock();
  if(!isRunning())
  {
    setRunning(true);
  }
  mut.unlock();
}

int main()
{
  std::thread t1(func1,true);
  std::thread t2(func1,true);
  std::thread t3(func1,true);
  std::thread t4(func1,true);

  t1.join();
  t2.join();
  t3.join();
  t4.join();

  return 0;
}


Edit: Changed !isRunning to !isRunning() (typo)

Solution

In this case, you really just need an atomic variable, and you can do a compare_exchange_strong to set the value once:

#include 
#include 

std::atomic_bool m_running;
bool wrote[4];

void func1(int num) {
    bool f{ false };
    if (m_running.compare_exchange_strong(f, true))
        wrote[num] = true;
}

int main(){
    std::vector t;

    for (int i = 0; i < 4; i++)
        t.emplace_back(func1, i);

    for (auto &t1 : t)
        t1.join();

    for (bool b : wrote)
        std::cout << std::boolalpha << b << "\n";
}


Given that thread 1 starts running first, and it's not doing very much, chances are that the first thread will do the write to the variable virtually every time. In case you're not familiar with it, atomic_bool is (obviously enough) an atomic Boolean type, and compare_exchange_strong does an atomic comparison and exchange. Specifically, it compares the current value to the first parameter, then if (and only if) they're equal, sets the value to the second parameter. The first parameter has to be passed by reference, because it also writes the new value to that variable. Finally, it returns true if and only if it actually wrote to the variable. In this case I've used that to record which thread actually wrote to the variable, so the final printout should show true once, and false three times.

Looking at the code as you've written it:

void func1(bool isSet)
{
  mut.lock();
  if (!isRunning())
  {
    setRunning(true);
  }
  mut.unlock();
}


You almost certainly want to use an std::lock_guard to automate unlocking the mutex. Right now, the code also ignores the isSet parameter, and always sets the value to true. I'd assume you really want to compare to isSet, and set it to that value if it's not already equal to it. Combining these, the code would come out something like:

void func1(bool isSet) { 
     std::lock_guard g(mut);

     if (isRunning() != isSet)
        setRunning(isSet);
}


That's somewhat simpler, but I still think the atomic_bool with compare_exchange_strong is neater. The atomic variable can degenerate to pretty much what you've done with a mutex in the worst case, but in quite a few cases, it'll be substantially more efficient.

Code Snippets

#include <thread>
#include <iostream>

std::atomic_bool m_running;
bool wrote[4];

void func1(int num) {
    bool f{ false };
    if (m_running.compare_exchange_strong(f, true))
        wrote[num] = true;
}

int main(){
    std::vector<std::thread> t;

    for (int i = 0; i < 4; i++)
        t.emplace_back(func1, i);

    for (auto &t1 : t)
        t1.join();

    for (bool b : wrote)
        std::cout << std::boolalpha << b << "\n";
}
void func1(bool isSet)
{
  mut.lock();
  if (!isRunning())
  {
    setRunning(true);
  }
  mut.unlock();
}
void func1(bool isSet) { 
     std::lock_guard g(mut);

     if (isRunning() != isSet)
        setRunning(isSet);
}

Context

StackExchange Code Review Q#43708, answer score: 6

Revisions (0)

No revisions yet.