Correctness of using std::atomic in combination with std::mutex

Question

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;
}

Please narrow down the question. There seems to be some lack of focus — Passer By, Jun 07 '17 at 08:15
What focus do you mean? What is not clear from the question? I would specify the question... — Dom, Jun 07 '17 at 08:21
You are asking in the title, the "correctness" of using atomics with the threading library, which is already vague. Then you asked two questions in the body: whether locking is required before a condition variable; which of many implementation is "better", which is also vague. Choose one and, as objectively as possible, be specific about what you want answered — Passer By, Jun 07 '17 at 08:23
Hope it is better now ;-) I have excluded the "whether locking is required before a condition variable". — Dom, Jun 07 '17 at 08:35
This code does nothing, because nothing is waiting on the condition variable. Your code needs to be *both* minimal *and* complete. This isn't easy, but failing to be both minimal and complete means I have to write the code you are missing before understanding your question. — Yakk - Adam Nevraumont, Jun 07 '17 at 12:55

score 2 · Answer 1 · answered Jun 07 '17 at 13:46

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).

Thanks, that is the exact answer explaining I was looking for :-) — Dom, Jun 19 '17 at 06:49

score 1 · Answer 2 · answered Jun 07 '17 at 09:39

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.

Correctness of using std::atomic in combination with std::mutex

2 Answers2