Right way to concurrently process std::stack

Question

This might be a basic question in terms of multithreaded programming but I really want to achieve the following without any concurrent data structure. Consider the code:

class A
{
    std::stack<int> s;
public:
    A()
    {
        s.push(7); s.push(6); s.push(5); s.push(4); s.push(3); s.push(2); s.push(1);
    }
    void process(int tid)
    {
        while (!s.empty())
        {
            std::unique_lock<std::mutex> lck(m);
            std::cout << tid << " --> " << s.top() << '\n';
            cv.wait(lck);
            s.pop();
            cv.notify_all();
            lck.unlock();
        }
    }
    std::mutex m;
    std::condition_variable cv;
};

int main()
{   
    A a;
    std::thread t1(&A::process, &a, 1);
    std::thread t2(&A::process, &a, 2);    
    t1.join();
    t2.join();
}

I want for each thread to print the top of the stack and pop it out so that the output is looking like this:

1 --> 1
2 --> 2
1 --> 3
2 --> 4
...

So only 1 thread should enter the while body and execute it only one iteration.

But instead it always outputs:

1 --> 1
2 --> 1

then it waits infinitely

How can I do this ?

What's wrong with the current solution ?

I don't know if this is your only problem, but you should never, ever wait on a condition variable without a loop or a lambda. Ah, you also seem to think condition variables are some kind of semaphore? Or somesuch. — Yakk - Adam Nevraumont, Nov 23 '17 at 20:45
@Yakk I rather was thinking to use conditional variable to provide some synchronization so that only 1 iteration of the loop is executed by thread — ampawd, Nov 23 '17 at 20:50

Yakk - Adam Nevraumont · Accepted Answer · 2017-11-23T21:03:31.437

Never, ever do a wait on a condition variable without testing for spurious wakeups. The easiest way is to use the lambda verson.

condition_variables are not semaphores, they are lower level than that.

class A
{
public:
  A()
  {
    s.push(7); s.push(6); s.push(5); s.push(4); s.push(3); s.push(2); s.push(1);
  }
  void process(int tid)
  {
    while (true)
    {
      std::unique_lock<std::mutex> lck(m);
      cv.wait(lck, [&]{ return std::this_thread::get_id() != last || s.empty(); });
      // must only read within lock:
      if (s.empty()) {
        last = std::thread::id{}; // thread ids can be reused
        break;
      }
      last = std::this_thread::get_id();
      std::cout << tid << " --> " << s.top() << '\n';
      s.pop();
      cv.notify_one();
    }
  }
  std::mutex m;
  std::condition_variable cv;
  std::thread::id last{};
  std::stack<int> s;
};

yeah I'm understanding, but it does not work the right way anyway. Throws exception trying to pop from empty container ... — ampawd, Nov 23 '17 at 20:58

Right way to concurrently process std::stack

1 Answers1