Tricky situation with race condition

Question

I have this race condition with an audio playback class, where every time I start playback I set keepPlaying as true, and false when I stop.

The problem happens when I stop() immediately after I start, and the keepPlaying flag is set to false, then reset to true again.

I could put a delay in stop(), but I don't think that's a very good solution. Should I use conditional variable to make stop() wait until keepPlaying is true?

How would you normally solve this problem?

#include <iostream>
#include <thread>
using namespace std;


class AudioPlayer

{
    bool keepRunning;
    thread thread_play;

    public: 
    AudioPlayer(){ keepRunning = false; }
    ~AudioPlayer(){ stop(); }

    void play()
    {
        stop();
        // keepRunning = true; // A: this works OK
        thread_play = thread(&AudioPlayer::_play, this);
    }
    void stop()
    {
        keepRunning = false;
        if (thread_play.joinable()) thread_play.join();
    }
    void _play()
    {
        cout << "Playing: started\n";
        keepRunning = true; // B: this causes problem
        while(keepRunning)
        {
            this_thread::sleep_for(chrono::milliseconds(100)); 
        }
        cout << "Playing: stopped\n";
    }
};


int main()
{
    AudioPlayer ap;

    ap.play();
    ap.play();
    ap.play();

    return 0;
}

Output:

$ ./test
Playing: started
(pause indefinitely...)

Answer 1

Here is my suggestion, combining many comments from below as well:

1) Briefly synchronized the keepRunning flag with a mutex so that it cannot be modified while a previous thread is still changing state.

2) Changed the flag to atomic_bool, as it is also modified while the mutex is not used.

class AudioPlayer
{
    thread thread_play;

public:
    AudioPlayer(){ }
    ~AudioPlayer()
    {
        keepRunning = false;
        thread_play.join();

    }

    void play()
    {
        unique_lock<mutex> l(_mutex);
        keepRunning = false;
        if ( thread_play.joinable() )
            thread_play.join();
        keepRunning = true;
        thread_play = thread(&AudioPlayer::_play, this);
    }
    void stop()
    {
       unique_lock<mutex> l(_mutex);
       keepRunning = false;
    }
private:
    void _play()
    {
        cout << "Playing: started\n";
        while ( keepRunning == true )
        {
            this_thread::sleep_for(chrono::milliseconds(10));
        }
        cout << "Playing: stopped\n";
    }

    atomic_bool keepRunning { false };
    std::mutex _mutex;
};




int main()
{
    AudioPlayer ap;
    ap.play();
    ap.play();
    ap.play();
    this_thread::sleep_for(chrono::milliseconds(100));
    ap.stop();
    return 0;
}

Answer 2

To answer the question directly.

Setting keepPlaying=true at point A is synchronous in the main thread but setting it at point B it is asynchronous to the main thread.

Being asynchronous the call to ap.stop() in the main thread (and the one in the destructor) might take place before point B is reached (by the asynchronous thread) so the last thread runs forever.

You should also make keepRunning atomic that will make sure that the value is communicated between the threads correctly. There's no guarantee of when or if the sub-thread will 'see' the value set by the main thread without some synchronization. You could also use a std::mutex.

Other answers don't like .join() in stop(). I would say that's a design decision. You certainly need to make sure the thread has stopped before leaving main()(*) but that could take place in the destructor (as other answers suggest).

As a final note the more conventional design wouldn't keep re-creating the 'play' thread but would wake/sleep a single thread. There's an overhead of creating a thread and the 'classic' model treats this as a producer/consumer pattern.

#include <iostream>
#include <thread>
#include <atomic>

class AudioPlayer

{
    std::atomic<bool> keepRunning;
    std::thread thread_play;

    public: 
    AudioPlayer():keepRunning(false){ 
    }
    ~AudioPlayer(){ stop(); }

    void play()
    {
        stop();
        keepRunning = true; // A: this works OK
        thread_play = std::thread(&AudioPlayer::_play, this);
    }

    void stop()
    {
        keepRunning=false;
        if (thread_play.joinable()){
            thread_play.join(); 
        } 
    }
    void _play()
    {
        std::cout<<"Playing: started\n";
        while(keepRunning)
        {
            std::this_thread::sleep_for(std::chrono::milliseconds(100)); 
        }
        std::cout<<"Playing: stopped\n";
    }
};


int main()
{
    AudioPlayer ap;

    ap.play();
    ap.play();
    ap.play();
    ap.stop();
    return 0;
}

(*) You can also detach() but that's not recommended.