CODEWITHSUNDEEP
c++multithreadingmutexrace-conditionmonitor
Jason
Jason

Reputation: 2197

Deadlock when using condition.wait()

Code found here: http://coliru.stacked-crooked.com/a/7942a18fe11ea544

I'm experimenting with the condition.wait without a timeout and am finding myself in a deadlock. The gist of what i'm trying to do is sit in a lock until i've determined that i've created a new file. I'd set a flag and notify the thread, which will then (in my real program) trigger a callback function. at the end of the running, i want to shut down all my threads. I set my loop variable to false, then notify one, which i assumed would unblock the thread. Am i mistaken that this would ignore what the predicate evaluated?

Could someone suggest a better thread layout that would correct the deadlock? Thanks.

#include <iostream>
#include <string>
#include <thread>
#include <chrono>
#include <atomic>
#include <mutex>
#include <condition_variable>

using namespace std::chrono_literals;
std::atomic_bool new_file_created_{false};
std::atomic_bool run_data_logger_{false};
std::condition_variable file_monitor_condition_;
std::mutex file_monitor_mutex_;
std::thread test_thread_;

void file_monitor_thread_func()
{
    using namespace std::chrono_literals;
    while (run_data_logger_)
    {
        std::unique_lock<std::mutex> lock(file_monitor_mutex_);
        file_monitor_condition_.wait(lock, [] { return new_file_created_.load(); });
        if (new_file_created_)
        {
            std::cout<< "New File Created" << std::endl;
            new_file_created_ = false;                       
            //do some stuff
        }
        else
        {}                
    }
}

void end_the_thread()
{
    std::cout << "Ending the Thread" << std::endl;
    run_data_logger_ = false;
    file_monitor_condition_.notify_one();
    if (test_thread_.joinable())
        test_thread_.join();

    std::cout << "Thread Ended" << std::endl;
}

void trigger_new_file()
{
   new_file_created_ = true;
   file_monitor_condition_.notify_one(); 
}

void start_the_thread()
{
    run_data_logger_ = true;
    test_thread_ = std::thread(file_monitor_thread_func);

    trigger_new_file();
}

int main()
{
    for (int j = 0; j<10; j++)
    {

        start_the_thread();
        std::this_thread::sleep_for(500ms);            
        end_the_thread();
    }
}

Upvotes: 0

Views: 360

Answers (2)

curiousguy
curiousguy

Reputation: 8270

A condition_variable is used together with a mutex, which is not a cosmetic feature. You seem to be under the impression that 

    std::unique_lock<std::mutex> lock(file_monitor_mutex_);

is enough to satisfy the precondition of the wait method; but a mutex conceptually only makes sense if it's used for mutual exclusion. You aren't using it as such, the mutex is only ever locked inside file_monitor_thread_func, so mutual exclusion on state changing operations is not guaranteed.

A telltale sign that you are using the monitor incorrectly is that you felt the need to use atomic objects, as the mutual exclusion (that is a prerequisite) guarantees that normal objects can be used.

There is simply no way to combine a condition and atomic objects. They belong in different designs.

Upvotes: 0

Igor Tandetnik
Igor Tandetnik

Reputation: 52471

After new_file_created_ becomes true for the first time, file_monitor_thread_func resets it to false then loops around and waits for it to become true again. But no one ever sets it to true again.

Upvotes: 3

Related Questions