CODEWITHSUNDEEP
c++multithreadingmutex
Qalam
Qalam

Reputation: 49

Multithreads not behaving as expected


#include <iostream>
#include <thread>
#include <mutex>
using namespace std;


std::mutex g_m;
std::string messageGlobal = "";

void threadFunc() // run in the log thread
{
    while (1)
    {
        g_m.lock();
        if (messageGlobal != "")
        {
            // logging takes a long time
            sleep(10000)
            cout << messageGlobal << endl;
            messageGlobal = "";
        }
        g_m.unlock();
    }
}


// logging api
void log(const string& message)
{
   g_m.lock();
   messageGlobal = message;
   g_m.unlock();
}

int main()
{
    std::thread th(threadFunc);
    
    log("Hello world!");
    log("Hello World2!");
    log("Hello World3!");
    log("Hello World4!");

    // Important work

    th.join();
    return 0;
}

New to threading here and I don't understand why only the last message is being printed.

The two threads here are main thread and an extra thread which runs permanently and outputs to the screen whenever there is a message to be printed.

Would appreciate if someone shows me where I went wrong.

Edit: the goal is for the code in "important code" to execute while the very long logging function takes place.

Upvotes: 0

Views: 84

Answers (3)

Tiger Yu
Tiger Yu

Reputation: 764

As other people suggested, you'd better use a queue to hold the messages and synchronize the access of the message queue between threads. However, here is a simple fix of your code here:

#include <iostream>
#include <thread>
#include <mutex>
using namespace std;


std::mutex g_m;
std::string messageGlobal = "";
bool g_all_done = false;

void threadFunc() // run in the log thread
{
    while (1)
    {
        g_m.lock();
        if (messageGlobal != "")
        {
            cout << messageGlobal << endl;
            messageGlobal = "";
        }
        bool all_done = g_all_done;
        g_m.unlock();
        if (all_done) break;
    }
}


// logging api
void log(const string& message)
{
  bool logged = false;
  do {
    g_m.lock();
    if (messageGlobal == "") {
      messageGlobal = message;
      logged = true;
    }
    g_m.unlock();
  } while(!logged);
}

void all_done() {
  g_m.lock();
  g_all_done = true;
  g_m.unlock();
}

int main()
{
    std::thread th(threadFunc);
    
    log("Hello world!");
    log("Hello World2!");
    log("Hello World3!");
    log("Hello World4!");

    all_done();  // this tells the print thread to finish.

    th.join();
    return 0;
}

Upvotes: 2

Slava
Slava

Reputation: 44268

Would appreciate if someone shows me where I went wrong.

You are wrong in assumption that threads would lock mutex in order, which is not guaranteed. So what happened that the same thread (main) locked the mutex multiple times and modified the message multiple times and second thread only had a chance to print the last message. To make it work you should make main thread to wait until message is emptied and only then to publish again, but most probably you should do that using condition variable as otherwise you would peg CPU doing this in code as written. And even better to create a queue of log messages and only wait when queue is full.

Note that you are missing condition for log thread to finish so th.join(); would hang.

Here is example on how it could work with single message:

std::mutex g_m;
std::condition_variable g_notifyLog;
std::condition_variable g_notifyMain;
bool g_done = false;
std::string messageGlobal = "";

void threadFunc() // run in the log thread
{
    while (1)
    {
        std::lock_guard<std::mutex> lk( g_m );
        g_notifyLog.wait( g_m, []() { return !messageGlobal.empty() || g_done; } );
        if( g_done ) break;
        cout << messageGlobal << endl;
        messageGlobal = "";
        g_notifyMain.notify_one();
    }
}


// logging api
void log(const string& message)
{
   std::lock_guard<std::mutex> lk( g_m );
   g_notifyMain.wait( g_m, []() { return messageGlobal.empty(); } );
   messageGlobal = message;
   g_notifyLog.notify_one();
}

void stop_log()
{
   std::lock_guard<std::mutex> lk( g_m );
   g_done = true;
   g_notifyLog.notify_one();
}

Upvotes: 1

ALX23z
ALX23z

Reputation: 4713

You didn't implement any mechanism that ensures that the threads operate interleaved. It is much more likely that the thread that unlocked mutex will be the one to lock it in the next moment as locking mutex/unlocking mutexes are fast operations unless sleep/wait is triggered.

Furthermore, the ThreadFunc is an endless loop. So it theoretically the program might just run the loop repeatedly without letting any execution of log to trigger.

You need to utilise std::condition_variable to signal between threads when data is available for logging and rewrite log method so it won't overwrite existing data-to-be-printed.

Upvotes: 0

Related Questions