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

When you call DeleteTimerQueueTimer make sure you pass INVALID_HANDLE_VALUE for the completion event. This will block your destructor till all pending callbacks are completed or cancelled.

e.g.

virtual ~Timer()
   {
       ::DeleteTimerQueueTimer( timerQueue, handle, INVALID_HANDLE_VALUE );
   }

This implies your code will block until all the timer callbacks are completed or cancelled. You can then proceed with destruction as usual. One thing to note though - you cannot call the deletetimerqueuetimer from the same timer callback or else you will deadlock.

I believe this alone should be sufficient to prevent the race condition you are experiencing.