C++ 11: Calling a C++ function periodically

Question

I have put together a simple c++ timer class that is supposed to call a given function periodically from various examples on SO as follows:

#include <functional>
#include <chrono>
#include <future>
#include <cstdio>

class CallBackTimer
{
public:
    CallBackTimer()
    :_execute(false)
    {}

    void start(int interval, std::function<void(void)> func)
    {
        _execute = true;
        std::thread([&]()
        {
            while (_execute) {
                func();                   
                std::this_thread::sleep_for(
                std::chrono::milliseconds(interval));
            }
        }).detach();
    }

    void stop()
    {
        _execute = false;
    }

private:
    bool            _execute;
};

Now I want to call this from a C++ class as followsL

class Processor()
{
    void init()
    {
         timer.start(25, std::bind(&Processor::process, this));
    }

    void process()
    {
        std::cout << "Called" << std::endl;
    }
};

However, this calls with the error

terminate called after throwing an instance of 'std::bad_function_call'
what():  bad_function_call

Do you actually wait for the thread to finish at some point? You completely detach it, so are you sure your main thread simply hasn't destroyed the relevant `Processor` object yet? — KillianDS, May 24 '15 at 16:35
@KillianDS that's most likely what's going on. Luca, you should post a [MCVE](http://stackoverflow.com/help/mcve). What happens if you `.join()` the thread? — vsoftco, May 24 '15 at 16:36
Side notes: This doesn't execute the function every 25ms, but only waits the given interval between the function calls. That's not exactly the same. Only if the function is a no-op. Secondly, You shouldn't just detach a thread. You are responsible for making it stop before the main thread stops, so better make it join in the destructor of your CallBackTimer. — stefan, May 24 '15 at 16:37
Ok, I must have misunderstood those posts I used to put this example together. For sure, the processor object is alive. — Luca, May 24 '15 at 16:38
It seems it was the detach. So when I use join() it works fine but can someone comment why that was an issue. It would crash immediately before. I thought the detach would only be called when the thread is done. — Luca, May 24 '15 at 16:49
You may have a look at: http://codereview.stackexchange.com/questions/47347/follow-up-timer-utilizing-stdfuture — , May 24 '15 at 16:56
Here's how I would (roughly) solve this task: http://coliru.stacked-crooked.com/a/782ae1d6780d9573 There are still some open design questions (such as does it need to wait the given interval or does it need to execute at that exact interval (no matter how long the function takes)). What's additional to Mikael's answer: Make `_execute` an atomic variable. Most likely, this error would not blow up in a million runs, but technically, it's allowed to blow up every time. Also, I've added a small wrapper to manage the thread automatically. — stefan, May 24 '15 at 16:58

Mikael Persson · Accepted Answer · 2015-05-24T17:19:21.843

The problem in your code is that your lambda expression inside your "start" function captures the local variables by reference, using the [&] syntax. This means that the lambda captures the interval and func variables by reference, which are both local variables to the start() function, and thus, they disappear after returning from that function. But, after returning from that function, the lambda is still alive inside the detached thread. That's when you get the "bad-function-call" exception because it tries to call func by reference to an object that no longer exists.

What you need to do is capture the local variables by value, with the [=] syntax on the lambda, as so:

void start(int interval, std::function<void(void)> func)
{
    _execute = true;
    std::thread([=]()
    {
        while (_execute) {
            func();                   
            std::this_thread::sleep_for(
            std::chrono::milliseconds(interval));
        }
    }).detach();
}

This works when I try it.

Or, you could also list out the values you want to capture more explicitly (which I generally recommend for lambdas):

void start(int interval, std::function<void(void)> func)
{
    _execute = true;
    std::thread([this, interval, func]()
    {
        while (_execute) {
            func();                   
            std::this_thread::sleep_for(
            std::chrono::milliseconds(interval));
        }
    }).detach();
}

EDIT

As others have pointed out, the use of a detached thread is not a great solution because you could easily forget to stop the thread and you have no way to check if it's already running. Also, you should probably make the _execute flag atomic, just to be sure it doesn't get optimized out and that the reads / writes are thread-safe. You could do this instead:

class CallBackTimer
{
public:
    CallBackTimer()
    :_execute(false)
    {}

    ~CallBackTimer() {
        if( _execute.load(std::memory_order_acquire) ) {
            stop();
        };
    }

    void stop()
    {
        _execute.store(false, std::memory_order_release);
        if( _thd.joinable() )
            _thd.join();
    }

    void start(int interval, std::function<void(void)> func)
    {
        if( _execute.load(std::memory_order_acquire) ) {
            stop();
        };
        _execute.store(true, std::memory_order_release);
        _thd = std::thread([this, interval, func]()
        {
            while (_execute.load(std::memory_order_acquire)) {
                func();                   
                std::this_thread::sleep_for(
                std::chrono::milliseconds(interval));
            }
        });
    }

    bool is_running() const noexcept {
        return ( _execute.load(std::memory_order_acquire) && 
                 _thd.joinable() );
    }

private:
    std::atomic<bool> _execute;
    std::thread _thd;
};

Thanks for a great answer. Is it safe to use detach() as in your example. Some of the comments mentioned using join() — Luca, May 24 '15 at 16:57
@Luca I would agree with the others that it's not great to created a detached thread like you do. What you should do is make the thread object a data member of the `CallBackTimer` class, and then do a `_execute = false;` followed by a `thd.join()` in the destructor of your `CallBackTimer` class. Also, your `_execute` flag should be either `volatile` or an `std::atomic`, to make sure that your while loop is not going to be optimized to a `while(true)` loop. I'll make an edit to show that. — Mikael Persson, May 24 '15 at 17:04
@MikaelPersson thank you for the answer, however I am getting a sigart -6 due to the join method isn't called properly. any ideas? — Dillon, Sep 05 '18 at 23:13
1. because func need time, so using sleep_until should be better. 2. because func may overtime, so use another thread to execute func should be better. — andrewchan2022, Mar 23 '20 at 03:57
In the `start` function. the line `if( _execute.load(std::memory_order_acquire) ) {`. Shouldn't it be like `if(false == _execute.load(std::memory_order_acquire) ) {`? — Mubin Icyer, Aug 27 '20 at 16:02
@MikaelPersson Use `std::this_thread::sleep_until()` instead of `std::this_thread::sleep_for()` will account for the time cost of `func` thus a higher precision. — heLomaN, Sep 29 '20 at 11:15
How to implement an optional thread name as an input to start? void start(int interval, std::function func, string threadName) — jacknad, Aug 11 '23 at 13:42

C++ 11: Calling a C++ function periodically

1 Answers1

Linked

Related