Sync more threads with atomic variable

Question

I'm right now working on a school projection and i have some problems with sync'ing 3 threads (2 threads + mian thread). The description says that i have to print 100x "ping" then 100x"pong" and 100x"\n", BUT in this order :

PingPong\n and so on...

when i start my code like I have it right now it just prints 100xPing then 100xPong and then 100x\n out, and I cant understand why :(

The Point which i cant understnad is , the while should stop when i set counter to 1 and it should open cond.wait(); after that it shoudl move to pong() and so on...

Here is the code:

  #include <iostream>
#include <string>
#include <thread>
#include <mutex>
#include <atomic>
using namespace std;
mutex m; // Mutex
         // Bedingungsvariable
condition_variable cond;
atomic<int> counter = 0;
bool done= true;

void ping() {
    unique_lock <mutex > lock{ m }; // m sperren

        while (counter != 0) {
            cond.wait(lock); //sperren und dann wieder freigeben
        }

    while (counter == 0) {

        for (int i = 0; i < 100; i++) {
            cout << "Ping"; 
            counter = 1;
            cond.notify_one();

        }   

    }
}
void pong() {
    unique_lock <mutex > lock{ m }; // m sperren
    while (counter != 1) {
        cond.wait(lock);
    }
    while (counter == 1) {

        for (int i = 0; i < 100; i++) {
            cout << "Pong";
            counter = 2;
            cond.notify_one();          

        }

    }

}


int main() {
    thread t1(pong);
    thread t(ping); // Zweiten Thread starten
    unique_lock <mutex > lock{ m }; // m sperren
    while (counter != 2) cond.wait(lock);
    while (counter == 2) {

        for (int i = 0; i < 100; i++) {
            cout << "\n";
            counter = 0;
            cond.notify_one();

        }       
    }
    lock.unlock(); // Mutex freigeben
    t.join();
    t1.join();
    system("PAUSE");
    return EXIT_SUCCESS;
}

Answer 1

I took the time to correct and provide feedback. Please take your time to learn and understand. I added comments in the code also.

• I removed the line #include <string>, because no strings are used in this program.
• I removed the line bool done = true;. I don't think you need it.
• You have to #include <condition_variable> if you want to use conditional variables (Bedingungsvariablen).
• atomic<int> counter = 0; gives me an error (g++ 4.8.4, Ubuntu, C++11 flag on). Instead I replaced that line by atomic<int> counter{0};.
• In regards to thread synchronisation I put comments in the code. Please check it out. The cond.wait(*lock*, *lambda*); might look strange, but it does the same as the while(*condition*) cond.wait(*lock*); of yours.
• notify_one() notifies only one thread, but you have no control over which one will be awaken. Therefore (since you have more than two threads) I use notify_all().
• I removed the line system("PAUSE"). Not needed.

Summary (of sync mechanism):

Basically each counter value (0, 1, 2) corresponds to a thread (you got that idea right I think).
However you skip the while loops, because you want to loop/process only a hundred times; you just need to loop a hundred times.
So the synchronisation is best put into the respective for loops and all follow the same pattern:

1. Get a lock, release the mutex and wait until condition is true. Meanwhile the thread is blocked.
2. Process the data and set the counter (condition for another thread).
3. Unlock the mutex and notify all waiting threads (so that the next thread whose condition is met can be woken up).

And so the threads can execute in turn.

---

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

using namespace std;

mutex m;                 // Mutex
condition_variable cond; // Bedingungsvariable
atomic<int> counter{0};

void ping() {

    for (int i = 0; i < 100; i++) {

        //cout << "Ping: m sperren und warten" << endl;
        // m sperren und ...
        unique_lock <mutex > lock{m};
        //... dann wieder freigeben
        //sobald counter == 0 gilt
        cond.wait(lock, [] {
            return counter == 0;
        });

        //Datenverarbeitung und ...
        //... counter setzen für nächsten Thread
        cout << "Ping";
        counter = 1;

        //cout << "Ping: m freigeben und benachrichtigen" << endl;
        //m freigeben und
        //andere Threads benachrichtigen
        lock.unlock();
        cond.notify_all();

    }
}

void pong() {

    for (int i = 0; i < 100; i++) {

        //cout << "Pong: m sperren und warten" << endl;
        //m sperren und ...
        unique_lock <mutex > lock{m};
        //... dann wieder freigeben
        //sobald counter == 1 gilt
        cond.wait(lock, [] {
            return counter == 1;
        });

        //Datenverarbeitung und ...
        //... counter setzen für nächsten Thread
        cout << "Pong";
        counter = 2;

        //cout << "Pong: m freigeben und benachrichtigen" << endl;
        //m freigeben und
        //andere Threads benachrichtigen
        lock.unlock();
        cond.notify_all();
    }

}

int main() {

    thread t(ping); // ping Thread starten
    thread t1(pong); // pong Thread starten

    for (int i = 0; i < 100; i++) {

        //cout << "\\n: m sperren und warten" << endl;
        // m sperren und ...
        unique_lock <mutex > lock{m};
        //... dann wieder freigeben
        //sobald counter == 2 gilt
        cond.wait(lock, [] {
            return counter == 2;
        });

        //Datenverarbeitung und ...
        //... counter setzen für nächsten Thread
        cout << endl;
        counter = 0;

        //cout << "\\n: m freigeben und benachrichtigen" << endl;
        //m freigeben und
        //andere Threads benachrichtigen
        lock.unlock();
        cond.notify_all();

    }

    t.join();
    t1.join();

    return EXIT_SUCCESS;
}

---

Note 1:

This does not work:

atomic<int> counter = 0;

This works:

atomic<int> counter(0);

or

atomic<int> counter{0};

or

atomic<int> counter;
...
counter = 0;