CreateTimerQueueTimer callback and race condition

Question

I'm using timer queues in my application, and pass a pointer to one of my own C++ Timer objects as the 'parameter' to the callback (in CreateTimerQueueTimer). I then call a virtual method on the object in the callback.

The destructor of the Timer object will make sure to cancel the timer using DeleteTimerQueueTimer().

static void callback( PVOID param, BOOLEAN timerOrWaitFired )
{
    Timer* timer = reinterpret_cast< Timer* >( param );
    timer->TimedOut();
}

class Timer
{
public:
   Timer();

   virtual ~Timer()
   {
       ::DeleteTimerQueueTimer( handle );
   }

   void Start( double period )
   {
      ::CreateTimerQueueTimer( &handle, ..., &callback, this, ... );
   }

   virtual void TimedOut() = 0;

   ...
};

However, there is a subtle race condition that if the callback has already been called, but the timer object is destroyed before the call to TimedOut(), the app crashes because the callback calls the virtual method on a non-existent object. Or even worse, while it's being deleted.

I do have mutexes in place to control multi-threaded calls, but I still get the problem.

Is using an object pointer as the callback parameter really a good idea? With no guarantees of synchronisation between the threads, it just smells bad to me.

Is there a better solution? What do other people do?

One thing that occurs is to keep a set of pointers to every single Timer instance (add in constructor, remove in destructor). But I don't think this would work because if Timer is derived from, we'd only remove the pointer from the set in the base class destructor; the damage is already done if we've started destroying the derived object.

Cheers.

Answer 1

The concept of using the object pointer as callback function parameter is not bad by itself. However, you obviously need to start the destruction after the last callback has exited.

So, I wouldn't make Timer abstract and derive from it at all. I would use another abstract class TimerImpl and make the Timer class use a TimerImpl instance:

class Timer
{
  TimerInstance* impl;
  void TimeOut() { impl->TimeOut(); }
public:
  ~Timer() {
    ... make sure the timer has ended and wont fire again after this line...
    delete impl;
  }
}

struct TimerImpl
{
  virtual void TimeOut()=0;
  virtual ~TimerImpl();
}

This way, you can ensure the destruction won't begin till after you say.

The second thing is, you have to wait for the last timer event to burn out. According to MSDN doc, you can do it by calling

DeleteTimerQueueTimer(TimerQueue, Timer, INVALID_HANDLE_VALUE)

Answer 2

You almost certainly can't do this with an inheritance model. The principal problem is that by the time the base class constructor has been entered, the derived object is already invalid but the timer might fire and nothing stops it attempting the virtual function call which will now result in undefined behaviour.

I think the way to do it is a wrapper like this. The point is to ensure that there is no race condition with trying to dispatch the 'timed out' event.

This implementation still has one flaw. There is a chance the a timer event is waiting when the timer object starts to be deleted. It is possible for the destructor to release the mutex and then destroy the mutex while the timer thread is waiting on the mutex. We've prevented the race in the dispatch of the 'timed out' event, but the behaviour of a thread waiting on a mutex that is destroyed depends on the implementation of mutex.

static void callback( PVOID param, BOOLEAN timerOrWaitFired );

class TimerWrapper
{
    public:

        /* Take reference to std::auto_ptr to ensure ownership transfer is explicit */
        TimerWrapper( std::auto_ptr<Timer>& timer ) : timer_(timer)
        {
            ::CreateTimerQueueTimer( &htimer_, ..., callback, this, ... );
        }

        void TimedOut()
        {
            ScopedGuard guard( mutex_ );
            if( timer_.get() )
                timer_->TimedOut();
        }

        ~TimerWrapper()
        {
            ::DeleteTimerQueueTimer( htimer_, ... );
            ScopedGuard guard( mutex_ );
            timer_.reset();
        }

    private:

        Mutex mutex_;
        std::auto_ptr<Timer> timer_;
        HANDLE htimer_;
};

static void callback( PVOID param, BOOLEAN timerOrWaitFired )
{
    TimerWrapper* timer = reinterpret_cast< TimerWrapper* >( param );
    timer->TimedOut();
}