CODEWITHSUNDEEP
c++multithreadingc++11stdatomicstdmutex
Dom
Dom

Reputation: 583

Correctness of using std::atomic<bool> in combination with std::mutex

I have written a shared priority queue class.
To signal to stop serving data I use method Cancel(), which sets a sign done to false and application is not allowed to write / read any data from the queue. I am not sure about using std::atomic<bool> in combination with std::mutex and std::condition_variable. I am not sure, if my solution is thread safe or race condition can happen:

The original version of Enqueue method is:

std::deque<T> deque;
std::mutex mtx;
std::condition_variable cv;
std::atomic<bool> done;

SharedPriorityQueue() : done(false)
{
}

~SharedPriorityQueue()
{
    Cancel();
}

void Enqueue(T item)
{
    if (done)
    {
        return;
    }

    std::lock_guard<std::mutex> lock(mtx);
    deque.push_back(item);
    cv.notify_one();
}

However, can be the variable done (atomic bool) separated from the locking mechanism by mutex?

To cancel the queue I use this construction:

void Cancel()
{
    if (done)
    {
        return;
    }
    done = true;

    cv.notify_all();
}

What is the best solution of the designs bellow?

// A)
void Enqueue(T item)
{
    if (done)
    {
        return;
    }

    {
        std::lock_guard<std::mutex> lock(mtx); // lock is released before notify call
        deque.push_back(item);
    }
    cv.notify_one();
}

// B)
void Enqueue(T item)
{
    {
        std::lock_guard<std::mutex> lock(mtx); // done is atomic bool and protected by the lock along with data (deque)
        if (done) // atomic bool
        {
            return;
        }

        deque.push_back(item);
    }
    cv.notify_one();
}

// C)
void Enqueue(T item)
{
    {
        std::lock_guard<std::mutex> lock(mtx); // done is NOT atomic bool and is protected by the lock along with data (deque)
        if (done) // simple bool
        {
            return;
        }

        deque.push_back(item);
    }
    cv.notify_one();
}

Waiting staff:

bool Dequeue(T& item)
{
    std::unique_lock<std::mutex> lock(mtx);
    while (!done && deque.empty())
    {
        cv.wait(lock);
    }

    if (!deque.empty())
    {
        item = deque.front();
        deque.pop_front();
    }

    if (done)
    {
        return false;
    }
    return true;
}

Upvotes: 0

Views: 1501

Answers (2)

Persixty
Persixty

Reputation: 8579

To ensure correctness you must modify the data pertaining to the "condition" holding the lock that the condition_variable acquires in the .wait(...).

While not normative the clearest statement of that I can find is:

Even if the shared variable is atomic, it must be modified under the mutex in order to correctly publish the modification to the waiting thread.

Here: http://en.cppreference.com/w/cpp/thread/condition_variable

It quite explicitly exactly answers your question!

Cancel() needs to hold mtx. At which point making it atomic stops helping.

I'm not sure where the normative statement is but I do know it is the case on MSVC++ Community.

You do not need to hold the lock when you notify_one() (or notify_all())but you must hold it when you modify 'the shared state' (in this case the queue or the flag).

Upvotes: 2

Maxim Egorushkin
Maxim Egorushkin

Reputation: 136208

The normal / most frequent case is probably that the queue is ready (not done), whereas done state is likely used during termination only. It may make little sense to optimize for the done case by using an atomic.

You need to understand what you are optimizing for and use a profiler.

Upvotes: 1

Related Questions