CODEWITHSUNDEEP
c++multithreadingc++11thread-safety
Nelarius
Nelarius

Reputation: 181

Why does the condition variable wake up erroneously with only one lock and an atomic counter?

I'm debugging some threading code, and I encountered some behavior that I don't understand.

I create a vector of threads. I have the variables atomic_uint Counter and an atomic_bool Stop, telling when the threads should stop. Each thread thread waits on a condition that the counter is not zero, and then decrements it.

In the main thread I increment Counter, and call notify_one() on the condition. The code is as follows.

#include <thread>
#include <condition_variable>
#include <atomic>
#include <vector>
#include <iostream>
#include <cstdlib>
#include <mutex>

int main() {
    const std::size_t       Tasks = 100u;
    const std::size_t       Repetitions = 100u;
    const std::size_t       Threads = 4u;
    std::mutex              Mutex;
    std::condition_variable Condition;
    std::atomic_uint        Counter(0);
    std::atomic_uint        MainCounter(0);
    std::atomic_uint        ThreadCounter(0);
    std::atomic_bool        Stop( false );


    std::vector<std::thread> v;
    for ( std::size_t i = 0; i < Threads; i++ ) {
        v.emplace_back( [&ThreadCounter,&Mutex, &Condition, &Counter, &Stop]() -> void {
            while ( true ) {
                {
                    std::unique_lock<std::mutex> lock( Mutex );
                    Condition.wait( lock, [&Counter, &Stop]() -> bool {
                        //wait while this is false
                        return Counter.load() >= 0u || Stop;
                    } );

                    if ( Stop && Counter == 0u ) {
                        return;
                    }
                    ThreadCounter++;
                    if ( Counter == 0 ) {
                        continue;
                    }
                    Counter--;
                }
            }   //while
        });
    }   //for

    for ( std::size_t i = 0u; i < Tasks; i++ ) {
        MainCounter++;
        Counter++;
        Condition.notify_one();
    }
    while ( Counter != 0u ) {
        std::this_thread::yield();
    }

    Stop = true;
    Condition.notify_all();
    for ( auto& t: v ) {
        t.join();
    }

    std::cout << "ThreadCounter = " << ThreadCounter.load() << std::endl;
    std::cout << "MainCounter = " << MainCounter.load() << std::endl;    

    return 0;
}

In the threading code, I have an additional atomic, ThreadCounter, to keep track of how many times Counter was actually decremented.

ThreadCounter always gets incremented many more times than Condition.notify_one() gets called:

ThreadCounter = 212
MainCounter = 100

How does this happen when I am locking the condition with one lock? As far as I understand, only one thread at a time can access Counter (in addition to the main thread).

Upvotes: 0

Views: 115

Answers (1)

Barry
Barry

Reputation: 303097

Each thread thread waits on a condition that the counter is not zero

That's actually not your condition:

Condition.wait( lock, [&Counter, &Stop]() -> bool {
    //wait while this is false
    return Counter.load() >= 0u || Stop;
        // ^^^^^^^^^^^^^^^^^^^^
} );

Counter is unsigned, so >= 0u is always true. If Counter == 0 then your loop body will increment ThreadCounter potentially lots of times, hence your discrepancy.

You probably meant:

return Counter > 0 || Stop;

(You don't need to call .load() there)

Upvotes: 3

Related Questions