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
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:
globalState variable and the notify function, the mutex, the condition variable and possibly many others).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.