Practical Testing: 11 - Moving away from the simplest thing

| 0 Comments
Previously, on Practical Testing: having decided to rework the code from scratch in TDD style we fixed the tick count bug for the second time - this time the solution was cleaner and simpler. At the end of that episode we were left with a failing test. The test was for multiple timers and it failed because our TDD route was taking a 'simplest thing that could possibly work' approach and that design only supported a single timer. Today we'll fix that problem by moving nearer to a real world solution.

Our TDD timer queue isn't currently a queue at all. It only allows a single timer to be set at any one time. That said, it does pass all of the tests that we defined before, apart from the one that requires multiple timers.

Adding support for multiple timers means that we need to switch from using a single instance of timer data to using a list of instances of timer data. When I originally introduced the code I explained how I didn't really like the dependency that the code had on our C++ tools library for the CNodeList class. This time we'll avoid that class and, instead, use a std::list and a std::map to provide the access that we require to our timer data.

The first thing we need to do is hoist the timer data out of the CCallbackTimerQueue class and into an class that we can use in our list of timers. Something like this will do:

struct CCallbackTimerQueue::TimerData
{
   TimerData(
      Timer &timer,
      DWORD timeout,
      UserData userData);
 
   Timer * const pTimer;
   const DWORD timeout;
   const UserData userData;
};

Setting a timer becomes a case of creating an instance of this class and inserting it into our list of timer data instances at the correct point. We solved the 'correct point' problem back when we were fixing the tick count bug the first time around; we'll use the same solution now. If the timer that we're just about to set has wrapped around we'll make sure we insert it after a dummy node in the list that represents the 'wrap point'. We're splitting the list into two parts, both sorted in ascending timeout order and we're making sure that wrapped timers occur after unwrapped timers even though the absolute timeout value is lower. We then return a Handle, just as before, and this handle is simply our TimerData cast to a Handle.

The problem with simply casting our dynamically allocated TimerData structure to a Handle and then returning it to the client is that the client may try and cancel a timer more than once, or may cancel a timer after the timer has already expired; both of these situations would mean that the Handle, if cast back to a TimerData structure, would be a pointer into deallocated memory. Our implementation of CancelTimer() needs to deal with these situations, so it can't simply cast the handle back to a TimerData and assume that the structure is good. Also, there's the issue of removing the timer from the list without needing to scan the list to locate the node we need to remove. Both of these problems can be solved by storing all Handles that we return to clients in a std::map which maps between handle and std::list iterator. CancelTimer() could then look something like this:

bool CCallbackTimerQueue::CancelTimer(
   Handle handle)
{
   bool wasPending = false;
 
   HandleMap::iterator mapIt = m_handleMap.find(handle);
 
   if (mapIt != m_handleMap.end())
   {
      wasPending = true;
 
      TimerQueue::iterator it = mapIt->second;
 
      delete *it;
      m_queue.erase(it);
      m_handleMap.erase(mapIt);
   }
 
   return wasPending;
}

There's a potential issue if a client holds on to a Handle after the timer expires and then tries to use it to cancel the timer; if the memory used by the expired timer has been reused for a new timer and the exact same area of memory was allocated then the handle will map to the wrong timer... Right now we'll assume this is a very unlikely problem and we'll write a test for it and deal with it later.

The changes required for GetNextTimeout() and HandleTimeouts() are relatively simple. GetNextTimeout() now uses the head of the list as the source of its 'next timeout' and HandleTimeouts() now pops entries off of the list and deals with the timeouts until the next timeout is some time in the future.

Now we have a class that passes all of our original tests and is much easier to test due to the fact that we've seperated the real work of the class from the multi-threaded implementation. Next time we'll put the threading back so that our new code can be used in place of our old code. Once that's done we'll write some tests for that unlikely Handle reuse bug.

Code is here. Same rules as before.

Leave a comment