C++ Timer - Start & Stop works - Restart doesn't

Question

I am having a trouble exiting a thread using a Restart function. When calling Stop it exits the thread, but Restart which calls Stop and then Start right after - doesn't exit the thread -> calls Start and creates a new thread.

Thanks. Any help would be really helpful and appreciated.

Dummy code to show the problem:

#include <iostream>
#include <thread>
#include <condition_variable>
#include <chrono>

using namespace std;

bool running = false;

unsigned int interval = 5000;

condition_variable cv_work;
mutex mu_cv_work;

void Start()
{
    unique_lock<std::mutex> lock(mu_cv_work);
    running = true;
    lock.unlock();
    thread([]{
        cout << "new thread" << '\n';
        while (running)
        {
            cout << "work..." << '\n';
            unique_lock<std::mutex> lock(mu_cv_work);
            cout << "sleep" << '\n';
            if (cv_work.wait_for(lock, chrono::milliseconds(interval), []{return running == false;}))
            {
                cout << "exit thread" << '\n';
                return;
            }
            cout << "done sleeping" << '\n';
        }
    }).detach();
}

void Stop()
{
    unique_lock<std::mutex> lock(mu_cv_work);
    running = false;
    lock.unlock();
    cv_work.notify_one();
}

void Restart()
{
    Stop();
    Start();
}

int main()
{
    Start();
    cout << "press to Stop" << '\n';
    cin.get();
    Stop();                             // Stop actually exits the Thread
    cout << "press to Start" << '\n';
    cin.get();
    Start();
    cout << "press to Restart" << '\n';
    cin.get();
    Restart();                         // Stop doesn't exit the Thread (Restart calls Stop() and Start())

    return 0;
}

Output:

press to Stop
new thread
work...
sleep
                      // KEY PRESS
exit thread
press to Start
                      // KEY PRESS
new thread
work...
sleep
press to Restart
                      // KEY PRESS
new thread
work...
sleep
done sleeping
work...
sleep

Expected output:

press to Stop
new thread
work...
sleep
                      // KEY PRESS
exit thread    
press to Start
                      // KEY PRESS
new thread
work...
sleep    
press to Restart
                      // KEY PRESS    
exit thread             // THIS LINE
new thread
work...
sleep
done sleeping
work...
sleep

Answer 1

You designed your threads this way – stop terminates the thread, and at this point it is gone – start creates a new one and cannot do anything else as the other one is lost.

Worse: those two threads (new and old one) might overlap in execution for some time.

You already have included a loop within your thread – now make sure that you do not exit this loop if you are going to restart your thread. You currently included a return within your code – this makes the running the test for running in your while loop obsolete anyway. We now can change a bit, reusing the running variable for another purpose; additionally we might use a three-fold state, possibly defined in an enum:

enum ThreadState // ThreadCommand?
{
    Active,  // or Running, or Run, if named ...Command
             // choose whichever names appear most suitable to you...
    Restart,
    Exit,    // or Stop
};

Now you'll set the state to Active on starting a thread, on stopping you'd set the state to Exit and notify the thread as is while on re-start you'd set the state appropriately, again notifying the thread. I personally would use an intermediate function for:

void stop()
{
    notify(Exit);
}
void restart()
{
    notify(Restart);
}
void notify(ThreadState state)
{
    // set the global state/command
    // notify the thread as done now
}

Now you'll consider the state in your loop:

[]()
{
    // endless loop, you return anyway...
    for(;;)
    {
        // working code as now
        // checking the notification as now, however the lambda now
        // checks the global state:
        if(wait_for(..., []() { return globalState != Active; })
        {
            if(globalState == Restart)
            {
                std::cout << "thread restarting" << std::endl;
                continue; // the loop!
            }
            else
            {
                std::cout << "thread exiting" << std::endl;
                return;
            }
        }
    }
}

Be aware, though, that you should re-initialise the state of local variables as well (it's a restart, isn't it?) – at least as far as these don't need to be intentionally persisted over multiple thread starts. A pretty simple approach to achieve correct re-initialisation might be packing the variables into a double loop:

[]()
{
    // all variables of which the states should be persisted over multiple
    // starts – well, RE-starts only for now!!!
    // for all starts, you currently rely on global variables

    for(;;)
    {
        // all variables that should get initialised with every restart
        // they get destroyed and newly constructed with every loop run

        for(;;) // another nested loop...
        {
            // ...
            if(wait_for(/* as above*/))
            {
                if(globalState == Restart)
                {
                    break; // the INNER loop!
                           // we'd then continue the outer one
                           // achieving the re-initalisation
                }
                else
                {
                    return; // still terminating the thread
                }
            }
        }
    }
}

I still recommend to pack all this code into its own class. This comes with several advantages:

• You can avoid global variables entirely.
• You can prevent access to variables and functions that should not be accessed directly by the user (e.g. the globalState variable and the notify function, the mutex, the condition variable and possibly many others).
• You can start more than one thread in parallel, each thread now has its own set of variables:

class TaskRunner // (or whatever name suites you...)
{
public:
    // all of these: containing the code as above, but accessing member variables!
    start(); // the lambda would need to capture this now: [this]() { ... }!
    stop();
    restart();
private:
    enum ThreadCommand { /* ... */ }; // not of interest outside the class
    ThreadCommand m_command;
    // mutex, condition variable and whatever else is not of interest outside

    // I'd store the thread in a local variable as well!
    // the advantage of, is that you can check the tread
    // with `joinable`, though this means that you
    // either need to join the thread as well OR delay
    // detaching until calling stop
    std::thread m_thread;

    notify(ThreadCommand command)
    {
        m_command = command;
        // notify the thread runner
    }

    // I'd prefer an ordinary member function over the lambda:
    void run()
    {
        // with the double loop as above, though there might not be variables
        // any more persisted over re-starts only; you might instead entirely
        // rely on member variables...
    }
}

void ThreadRunner::start()
{
    if(m_thread.joinable())
    {
        // appropriate error handling...
    }
    else
    {
        // with separate run function instead of lambda:
        m_thread = std::thread(&ThreadRunner::run, this);
    }
}

void ThreadRunner::stop()
{
    // stopping thread as above

    if(m_thread.joinable())
    {
        m_thread.join(); // waits for thread completion, assuring some
                         // potential cleanup work correctly to be done
                         // prevents, too, two threads overlap provided
                         // subsequent `start` is not called from yet another
                         // thread (you might store thread id of last successful
                         // caller and check it to prevent such asituation)

        // alternatively you might detach, with all its disadvantages
        // but that's far less recommendable
    }
}

You should call stop from within the destructor as well to make sure a thread is correctly stopped if the ThreadRunner instance runs out of scope without stop having been explicitly called:

ThreadRunner::~ThreadRunner() // declare in class as well or define directly there
{
    stop();
}

Note: Any code above is entirely untested; if you find a bug, please fix yourself.