Implementing Binary Semaphore using atomic<int>-CodePudding

I have written this code to demonstrate Binary Semaphore using only atomic .

1 thread producer will push 100 elements in the queue initially.

later threads 2 and 3 which is the consumer will run in parallel to consume this queue.

The issue is: I can see the same data/element print by both the threads

BinarySemaphore.cpp

std::queue<int> buffer;
int s_data = 1;


struct Semaphore
{
    Semaphore():s_(1)
    {

    }
    void wait()
    {
        while( s_.load() ==  0);    //you will keep waiting here until s_ becomes 1
        s_.fetch_sub(1);
    }

    void signal()
    {
        s_.fetch_add(1);
    }

    private :
    std::atomic<int> s_ ;

};

Semaphore s; 


void producer()
{
    
    while(s_data <= 100)
    {
        s.wait();

        // critical section starts
        {
            
            std::ostringstream oss;
            oss << "Consumer pushing data " << s_data <<endl;

            cout << oss.str();
            
            buffer.push(s_data  );
        }
        // critical section ends

        s.signal();
    }
}

void consumer()
{
    while (1)
    {
        s.wait();
        // critical section starts
        if (!buffer.empty())
        {
            int top = buffer.front();
            buffer.pop();

            std::ostringstream oss;
            oss << "consumer  thread id= " << this_thread::get_id() <<  " reading data = " << top  << endl;
            cout << oss.str();
        }
        // critical section ends
        s.signal();
    }
}




int main()
{
    Semaphore s;

    std::thread prod(producer);

    prod.join();

    std::thread cons1(consumer);
    std::thread cons2(consumer);


    cons1.join();
    cons2.join();

}

CodePudding user response：

You have a "gap" in wait:

void wait()
    {
        while( s_.load() ==  0);    //you will keep waiting here until s_ becomes 1
        s_.fetch_sub(1);
    }

load() and fetch_sub are atomic by themselves but between the while... load() and fetch_sub() there is a gap. Maybe you should use "exchange" (and evaluate the result): https://en.cppreference.com/w/cpp/atomic/atomic/exchange or even better use compare_exchange: https://en.cppreference.com/w/cpp/atomic/atomic/compare_exchange

CodePudding user response：

If you need to do more then one action on atomic you need check consistency if data was not changed. Other wise you will have a "gap" as point out in other answer.

There is a compare_exchange which should be used for that:

    void wait()
    {
        auto oldValue = s_.load();
        while (oldValue == 0 || !compare_exchange_strong(oldValue, oldValue - 1))
           oldValue = s_.load();
    }

Now if oldValue is out of date oldValue will be updated and new check will be performed (new iteration of loop) and in next iteration condition will be checked again.