1

I'm trying to create a C++ class that wraps POSIX timer functions so that I have a Timer class that can be used in a flexible manner and pass in user defined data ( which normally can't be done with straight POSIX C timer functions ). So I have the following:

Timer class implementation:

#include <functional>
#include <utility>
#include <sys/time.h>
#include <signal.h>
#include <time.h>
#include <string.h>


template<typename F>
class Timer
{
    public:
        struct sigaction SignalAction;
        struct sigevent signalEvent;
        struct itimerval   timer_ms;
        timer_t timerID;

        Timer(F callback, int milliseconds) : onTimeout(std::move(callback)) 
        {
            timer_ms.it_value.tv_sec    = milliseconds / 1000;
            timer_ms.it_value.tv_usec   = ( milliseconds % 1000 ) / 1000;
            timer_ms.it_interval.tv_sec = milliseconds / 1000;
            timer_ms.it_interval.tv_usec = ( milliseconds % 1000 ) / 1000;

            // Clear the sa_mask
            sigemptyset(&this->SignalAction.sa_mask);
            // set the SA_SIGINFO flag to use the extended signal-handler function
            this->SignalAction.sa_flags = SA_SIGINFO;

            // Define sigaction method
            // This function will be called by the signal
            this->SignalAction.sa_sigaction = Timer::alarmFunction;         

            // Define sigEvent
            // This information will be forwarded to the signal-handler function
            memset(&this->signalEvent, 0, sizeof(this->signalEvent));
            // With the SIGEV_SIGNAL flag we say that there is sigev_value
            this->signalEvent.sigev_notify = SIGEV_SIGNAL;
            // Now it's possible to give a pointer to the object
            this->signalEvent.sigev_value.sival_ptr = (void*) this;
            // Declare this signal as Alarm Signal
            this->signalEvent.sigev_signo = SIGALRM;

            // Install the Timer
            timer_create(CLOCK_REALTIME, &this->signalEvent, &this->timerID);
            sigaction(SIGALRM, &this->SignalAction, NULL);
        }

        void start()
        {
            // start the timer
            //timer_settime(this->timerID, 0, &this->timerSpecs, NULL);
            setitimer(ITIMER_REAL, &timer_ms, NULL);
            return;
        }


        static void alarmFunction(int sigNumb, siginfo_t *si, void *uc)
        {
            // get the pointer out of the siginfo structure and asign it to a new pointer variable
            Timer *ptrTimer = reinterpret_cast<Timer *> (si->si_value.sival_ptr);
            // call the member function
            ptrTimer->onTimeout();
        }


    private:
        F onTimeout;
};


template<typename F>
Timer<F> CreateTimer(int milliseconds, F callback)
{
    return Timer<F>(callback, milliseconds);
}

Note that in the Timer class the template F onTimeout is called in the Timer::alarmFunction() method by using a pointer stored in the siginfo_t structure.

static void alarmFunction(int sigNumb, siginfo_t *si, void *uc)
{
    // get the pointer out of the siginfo structure and asign it to a new pointer variable
    Timer *ptrTimer = reinterpret_cast<Timer *> (si->si_value.sival_ptr);
    // call the member function
    ptrTimer->onTimeout();
}

And my main.cpp:

#include "Timer.h"
#include <stdlib.h>
#include <iostream>

class Generic
{
    private:
        int m_data;
    public:
        Generic( int data ) : m_data( data ) {}
        int getData() { return( m_data); }
};

void HandleTimer()
{
    std::cout << "HandleTimer " << std::endl;
    return;
}

int main(int argc, char *argv[])
{
    Generic obj(42);

    auto timer = CreateTimer(1000, [] { HandleTimer(); });
    timer.start();

    while(1)
    {
        sleep(5);
    }

    return( 0 );
}

In main.cpp you will see that created a silly little class called Generic and instantiated one as obj.

Some questions:

1- How can I pass obj to HandleTimer() in this line:

auto timer = CreateTimer(1000, [] { HandleTimer(); });

so that when the timer is triggered HandleTimer() is called by the Timer class and has access to obj?

2- What might be some better ways to do this?

I've already created a much simpler Timer class that just takes a frequency and static function as parameters so that when the timer expires the static function is called. This is nice, but the static function doesn't have access to any class scope and has no access to user defined data without resorting to global data.

EDIT: I've tried the following:

void HandleTimer( Generic *obj )
{
    std::cout << "HandleTimer " << std::endl;
    return;
}



int main(int argc, char *argv[])
{
    Generic obj(42);

    auto timer = CreateTimer(1000, [&obj] { HandleTimer(&obj); });
    timer.start();

    while(1)
    {
        sleep(5);
    }

    return( 0 );
}

But this causes a segfault. gdb session below:

(gdb) file blink
Reading symbols from /home/jrn/build_root/src/svn/arm/arm-gpio/Timer/blink...done.
(gdb) run
Starting program: /home/jrn/build_root/src/svn/arm/arm-gpio/Timer/blink 
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Program received signal SIGSEGV, Segmentation fault.
0x000000000040298a in __lambda0::operator() (__closure=0x100) at main.cpp:28
28      auto timer = CreateTimer(1000, [&obj] { HandleTimer(&obj); });
(gdb) 
Chimera
  • 5,884
  • 7
  • 49
  • 81
  • Is there some reason not to simply use the C++11 `std::chrono` features instead? – Edward Jun 19 '14 at 00:56
  • @Edward Yes, I don't want to do polling... Can std::chrono call a callback with access to the class data when a certain time has passed? – Chimera Jun 19 '14 at 01:03
  • Nitpick: drop `reinterpret_cast`, you don't need it, and you don't want it (it won't matter for the correct use above, but it is more error prone; learn to avoid it and it will raise more suspicions when it is used) – David Rodríguez - dribeas Jun 19 '14 at 03:47
  • 1
    Related: [C++11 way to create a timer that can be “stopped” if needed](http://stackoverflow.com/questions/24171395/c11-way-to-create-a-timer-that-can-be-stopped-if-needed). Specifically, take a look at [this sample implementation](http://coliru.stacked-crooked.com/a/8804fee86d99a1e5). – Casey Jun 19 '14 at 16:08

3 Answers3

3

Your code has undefined behavior in at least 2 different places.

First, your CreateTimer function returns the Timer object by value. This means that it's making a copy of the timer object, and because you're passing the this pointer to the sigevent, after the copy, the this pointer of the timer object in main() will be different. So when the callback fires, you'll be attempting to invoke onTimeout on some object that was destroyed.

You will probably get away with this most of the time because due to copy elision no copy will be made. However, you can verify the buggy behavior yourself by compiling with -fno-elide-constructors after you fix the other bug.

You can work around this by not using CreateTimer(), instead add the following lines to main(). I'm not sure of a better way to fix this. This also demonstrates how to pass obj by value to the lambda.

auto l = [obj]() { HandleTimer(obj); };
Timer<decltype(l)> timer(l, 1000);
timer.start();

Also consider making your class non-copyable/assignable

Timer(Timer const&) = delete;
Timer& operator=(Timer const&) = delete;

The second problem is also with the this pointer, but I don't know enough about the timers you're using to figure out what you're doing wrong. Add these debug statements to the code:

// Now it's possible to give a pointer to the object
this->signalEvent.sigev_value.sival_ptr = static_cast<void *>(this);
std::cout << "this: " << this << std::endl;

and

Timer *ptrTimer = static_cast<Timer *> (si->si_value.sival_ptr);
std::cout << "sival_ptr: " << si->si_value.sival_ptr << std::endl;
// call the member function
// ptrTimer->onTimeout();

You'll see that the pointer value printed by the two statements are different. Looking at some code you had commented out, and this answer, I made the following changes:

Add a data member and initialize it in the constructor

struct itimerspec  timerSpecs;

...

timerSpecs.it_value.tv_sec    = milliseconds / 1000;
timerSpecs.it_value.tv_nsec   = ( milliseconds % 1000 ) / 1000;  // this needs fixing
timerSpecs.it_interval.tv_sec = milliseconds / 1000;
timerSpecs.it_interval.tv_nsec = ( milliseconds % 1000 ) / 1000; // this needs fixing

...

void start()
{
    // start the timer
    timer_settime(this->timerID, 0, &this->timerSpecs, NULL);
    // setitimer(ITIMER_REAL, &timer_ms, NULL);
    return;
}

Also modified HandleTimer slightly

void HandleTimer(Generic g)
{
    std::cout << "HandleTimer " << g.getData() << std::endl;
    return;
}

Now your code runs and prints

HandleTimer 42
Praetorian
  • 106,671
  • 19
  • 240
  • 328
  • Thank you so much. I will test this out and make it the approved answer shortly. – Chimera Jun 19 '14 at 02:32
  • Using `auto` and `decltype` is the easiest way, but one can also use `std::function` as a declared type of the lambda. Also, as a minor nit, `Generic::getData()` should be `const`. – Edward Jun 19 '14 at 10:55
  • @Edward Agreed that `getData()` should be `const`, you should tell the OP that :) That's the reason why I made `HandleTimer` take `Generic` by value (didn't want to make the lambda `mutable`). As for `std::function`, sure that's an option, but why use it when it can be avoided? – Praetorian Jun 19 '14 at 17:14
  • @Praetorian: agree on all counts. The comments were directed at enlightening others who might read this as to options which might be used in similar, but not identical situations. – Edward Jun 19 '14 at 17:26
2

You can pass obj into the lambda by reference like this:

auto timer = CreateTimer(1000, [&obj] { HandleTimer(obj); });

Note that this creates a reference to the object on the stack, which is dangerous with asynchronous callbacks like a timer where the timer might callback to the lambda after the object leaves scope.

I regularly use std::shared_ptr<> for this reason:

#include <memory>

// ...

std::shared_ptr<Generic> obj(new Generic(42));

auto timer = CreateTimer(1000, [obj] { HandleTimer(obj); });

The lambda (and the timer) will hold a reference to obj for the lifetime of timer.

Consider std::function instead of your own template parameter for the callback.

#include <functional>

// ...
Timer(std::function<void()> callback, int milliseconds): onCallback(callback), // etc
dmercredi
  • 2,112
  • 11
  • 10
  • 2
    Using `std::shared_ptr` for this is the wrong use case. `std::shared_ptr` is for shared ownership. – Rapptz Jun 19 '14 at 01:05
1

As I mentioned in the comments, I think this could be much simpler using std::chrono and std::thread. Specifically, you could use this:

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

// Generic class and HandleTimer code goes here

int main()
{
    Generic obj(42);
    std::thread([&obj]() {
        while(1) {
            std::this_thread::sleep_for(std::chrono::milliseconds(1000));
            HandleTimer(obj);
        }
    }).detach();

    while(1)
    {
        ++obj;
        std::this_thread::sleep_for(std::chrono::seconds(5));
    }
    return 0;
}

Note that the lambda in the std::thread captures obj by reference, so you'll have to take care that it does not outlive the object that's passed, but this code handles this case correctly.

The std::thread will call HandleTimer(obj) (or whatever function you wish) every 1000 milliseconds.

I've modified your Generic object to add an increment operator and made getData() const:

Generic &operator++() { ++m_data; return *this; }
int getData() const { return m_data; }

I also changed your HandleTimer routine like so:

void HandleTimer(const Generic &obj)
{
    std::cout << "HandleTimer " << obj.getData() << std::endl;
}

Within the main loop, it's called every 5 seconds. The result is that HandleTimer will be called every second, and the object to which it points is incremented every 5 seconds. When I run the program I get this output, with one line printed per second.

HandleTimer 43
HandleTimer 43
HandleTimer 43
HandleTimer 43
HandleTimer 44
HandleTimer 44
HandleTimer 44
HandleTimer 44
HandleTimer 44
HandleTimer 45
HandleTimer 45
HandleTimer 45
HandleTimer 45
... 

Note that there really ought to be a mutex guarding access to std::cout if you don't care to have the output possibly scrambled if other threads also print, but I'll leave that to you.

This solution has the benefit that it's portable, without relying on the arcane details of Posix timers which, as you've seen, are not trivial to get right.

Edward
  • 6,964
  • 2
  • 29
  • 55