template<class Function, class... Args, class Rep, class Period>
void Timer::setInterval(const std::chrono::duration<Rep, Period>& sleep_duration, Function&& f, Args&&... args) {
this->clear = false;
std::thread t([this, f=std::move(f), sleep_duration, args = std::make_tuple(std::forward<Args>(args)...))]() {
while(true) {
if (this->clear) return;
std::this_thread::sleep_for(sleep_duration);
if(this->clear) return;
std::apply(f, args);
}
});
t.detach();
}
That should do correct capturing. Note that as the timer goes off more than once, you don't want to further-forward data into the f.
But that still doesn't work because you didn't capture this.
Really, you should do a proper cv/mutex and not leak threads.
class Timer {
std::condition_variable cv;
mutable std::mutex m;
bool clear = false;
std::thread active_thread;
void dispose_thread() {
if (active_thread.joinable()) {
stop();
active_thread.join();
}
clear = false; // safe, because all threads are dead
}
public:
template<class Function, class... Args, class Rep, class Period>
[[maybe_unused]] void setInterval(const std::chrono::duration<Rep, Period>& sleep_duration, Function&& f, Args&&... args);
auto lock() const {
return std::unique_lock( m );
}
void stop() {
auto l = lock();
clear = true;
cv.notify_all();
}
~Timer() { dispose_thread(); }
};
template<class Function, class... Args, class Rep, class Period>
void Timer::setInterval(const std::chrono::duration<Rep, Period>& sleep_duration, Function&& f, Args&&... args) {
dispose_thread();
// capture everything explicitly; when messing with threads,
// best to understand state
active_thread = std::thread(
[this, f = std::move(f), sleep_duration, args = std::make_tuple(std::forward<Args>(args)...)]()
{
while(true) {
{
auto l = lock();
// return value here isn't useful, but the function is:
(void)cv.wait_for(l, sleep_duration, [&]{ return clear; });
if (clear)
return;
}
std::apply(f, args);
}
}
);
}
and .stop() and .join() in the destructor.
Live example.
This prevents you from leaking threads and outliving your timer object and doesn't make you wait for a timer interval to set a new interval and the like.
You should also set up sleep_until time points, as if you want to do something every second and the f function takes 0.1 seconds, this will actually repeat every 1.1 seconds.
If you want to maintain multiple worker threads in one Timer object, I'd switch to using std::futures in a vector, and maybe sweeping them to clean up ones that are finished at various intervals.
The idea of waiting for existing tasks to cleanup before starting a new tasks has value to me. If you don't like that, you could have the threads report back that they are ready to clean up, and clean them up lazily later. But this also requires multiple clear variables, so the threads that are delayed don't get the wrong value when the next thread is ready to go.
