Practical Testing: 26 - More functionality, more refactoring and a new bug

Previously on “Practical Testing”… To deal with some specific usage scenarios of a piece of general purpose code I’m in the process of implementing a timer wheel that matches the interface to the timer queue that I previously developed in this series of articles. The timer wheel trades space for speed and so far the development has gone well as I’ve been able to use the tests that I had already developed for the previous implementations to guide the new development.

By the end of last time we’d got to the point where we had four functions left to implement…

Today we’ll deal with the second style of SetTimer() call. The timers can be set in two ways, the first is useful if you need to repeatedly set and reset a timer, you create a timer handle by calling CreateTimer() and you can then call SetTimer() and CancelTimer() as often as you like. Finally you call DestroyTimer() when you’re done with your timer. The second style is for when you simply want to ‘fire and forget’ a timer. You simply call the second variation of SetTimer() and this creates a timer for you, sets it and destroys it once the timer has timed out or when the timer system is destroyed. This second style of timer makes it easy for the caller and slightly more complex for the timer system since there are now timers that need to be cleaned up once they expire. However, we need to deal with this kind of thing anyway as a timer could be destroyed during timeout processing, or in the gap between a begin/handle/end sequence of timer handling.

Whilst adjusting the tests to make sure they took into account the timer wheel’s traits and generally worked correctly with the new implementation I decided that although I like my tests rigid (some would say brittle, or fragile), I’d gone slightly too far with the logging coming out of the tick count providers. These were reporting the value of the tick count that was being provided as well as the fact that the call was made. Now in some of my tests this is useful but here the test was clearly setting the value so logging it was of no use and made the tests more complex in the presence of variable timer granularities. Adjusting the mocks and the tests makes them a bit cleaner.

The tests also needed adjusting now that all of the code can be built with monitoring enabled. The traits work pretty well for this and I’m happy with the results.

There’s still quite a bit of duplicate code in the tests and the code to create and set a timer is one piece that’s easy to slim down by using the helper function that is used by some but not all of the tests. Only the tests that are actually for SetTimer() need to do it long hand to make it clear what we’re actually testing.

Since we now have timers that can delete themselves and since I recently added to the timer monitoring interface to allow us to ensure that all timers were always cleaned up it seems about the right time to add monitoring interface support to the timer wheel.

The resulting changes to implement the second SetTimer() overload are as follows, note that I’ve adjusted CreateTimer() so that the common code that I need to call when I create a timer in SetTimer() isn’t duplicated. The timer data constructor that we’re using there sets the timer up appropriately for single use mode.

CCallbackTimerWheel::Handle CCallbackTimerWheel::CreateTimer()
{
   TimerData *pData = new TimerData();
  
   return OnTimerCreated(pData);
}

CCallbackTimerWheel::Handle CCallbackTimerWheel::OnTimerCreated(
   TimerData *pData)
{
   m_handles.insert(pData);
  
#if (JETBYTE_PERF_TIMER_WHEEL_MONITORING_DISABLED == 0)
  
   m_monitor.OnTimerCreated();
  
#endif
  
   return reinterpret_cast<Handle>(pData);
}

bool CCallbackTimerWheel::SetTimer(
   const Handle &handle,
   Timer &timer,
   const Milliseconds timeout,
   const UserData userData)
{
   if (timeout > m_maximumTimeout)
   {
      throw CException(
         _T("CCallbackTimerWheel::SetTimer()"), 
         _T("Timeout is too long. Max is: ") + ToString(m_maximumTimeout) + _T(" tried to set: ") + ToString(timeout));
   }
  
   TimerData &data = ValidateHandle(handle);
  
   const bool wasPending = data.CancelTimer();
  
   data.UpdateData(timer, userData);
  
   InsertTimer(timeout, data, wasPending);
  
#if (JETBYTE_PERF_TIMER_WHEEL_MONITORING_DISABLED == 0)
  
   m_monitor.OnTimerSet(wasPending);
  
#endif
  
   return wasPending;
}
  
void CCallbackTimerWheel::SetTimer(
   IQueueTimers::Timer &timer,
   const Milliseconds timeout,
   const IQueueTimers::UserData userData)
{
   if (timeout > m_maximumTimeout)
   {
      throw CException(
         _T("CCallbackTimerWheel::SetTimer()"), 
         _T("Timeout is too long. Max is: ") + ToString(m_maximumTimeout) + _T(" tried to set: ") + ToString(timeout));
   }
  
   TimerData *pData = new TimerData(timer, userData);
  
   OnTimerCreated(pData);
  
   InsertTimer(timeout, *pData);
  
#if (JETBYTE_PERF_TIMER_WHEEL_MONITORING_DISABLED == 0)
  
   m_monitor.OnOneOffTimerSet();
  
#endif
}

So that we can delete these “one shot” timers when the timer wheel is destroyed we need to keep the handle in the handle map, this does, however, open a hole in our handle validation code as someone could pass in a random invalid handle value that matches one of the “one shot” timers and convince the timer wheel that the handle is valid. To prevent this we now also check that the handle isn’t scheduled to be deleted after the timer expires, the only handles in the handle map that will be set like this are the “one shot” timers and these, by definition, don’t have a valid handle that you can manipulate.

CCallbackTimerWheel::TimerData &CCallbackTimerWheel::ValidateHandle(
   const Handle &handle) const
{
   TimerData *pData = reinterpret_cast<TimerData *>(handle);
  
   Handles::const_iterator it = m_handles.find(pData);
  
   if (it == m_handles.end())
   {
      throw CException(
         _T("CCallbackTimerWheel::ValidateHandle()"), 
         _T("Invalid timer handle: ") + ToString(handle));
   }
  
   if (pData->DeleteAfterTimeout())
   {
      throw CException(
         _T("CCallbackTimerWheel::ValidateHandle()"), 
         _T("Invalid timer handle: ") + ToString(handle));
   }
  
   return *pData;
}

With all of that done and with some more tests passing I’m left with just the BeginTimeoutHandling(), HandleTimeout(), EndTimeoutHandling() code to implement. Unfortunately there’s a bug in our timer setting code for the timer wheel and the existing tests don’t catch it.

Let’s assume that we’re in the situation shown above and we set a timer. The timer wheel has its current time set to 35ms before the actual time because timeouts haven’t been handled yet. At present if we set a timer for 10ms the timer will be set at the point marked as 30 on the diagram above rather than at the point marked 65. The existing tests don’t show this problem as they all set timers for a ’now’ that is the same as the time the wheel was created or just after timeouts have been handled; which means that current always equals now in most of the tests that set timers. A new test that sets a timer with now != current clearly shows the problem. I’ll leave this broken test to show me the way for next time.

The code can be found here and the previous rules apply.