Practical Testing: 13 - Missing functionality

| 0 Comments
Previously, on Practical Testing: we added a multi-threaded version of our timer queue that manages timeouts automatically. This time we'll integrate our new timer queue into an application that uses the old version of the code and, along the way, discover some functionality that the original version supports but that the new version currently doesn't.

One of the original reasons that we designed this timer queue was to handle per socket, read timeouts in our IO Completion Port based server framework. I explained why we chose to roll our own timer queue back at the start of the series.

Typical usage of the timer queue for socket timeouts is as follows. The socket server derived class implements our timer callback interface. When a socket connection is established a timer is set and the socket is passed as the "user data". Each time data arrives on the socket we reset the timer. If the timer expires we know our read timed out and we can do whatever is appropriate; send an inactivity warning message, disconnect, etc. If the connection is closed normally then we cancel the timer. Most of the time we're just reseting the timer; in the original code we checked to see if the timer was already set when we called SetTimer() and if it was we 'reset' it. In the new code, to achieve the same functionality we would need to explicitly cancel the timer and then set it again. Whilst this more clearly indicates what's going on, it's a pain because it involves deallocating and reallocating a timer data structure when we should be able to reuse the existing timer data structure if the timer hasn't expired.

Rather than augment the functionality of our new implementation of SetTimer() I've decided that it's better to add a new function, ResetTimer() that does exactly what it says on the tin. ResetTimer() is, not surprisingly, a cross between SetTimer() and CancelTimer(). It looks like this:

bool CCallbackTimerQueue::ResetTimer(
   Handle &handle, 
   Timer &timer,
   const DWORD timeoutMillis,
   const UserData userData)

Note that like CancelTimer() you need to supply the handle to the timer that you want to manipulate and that it returns a bool to signal if the timer was still pending when you reset it or if it had already expired. As with SetTimer() you need to supply the timer callback, timeout and user data; at first this may seem strange, after all, you're resetting the timer so you'd expect the timer callback and user data to stay the same and only the timeout to change. The thing is, if the timer has already expired then ResetTimer() effectively becomes SetTimer() and so it needs all of the information that you'd need to supply when setting a timer for the first time. Since we have the caller supply these data items we actually use them each time, even if we're resetting a timer that hasn't expired. This means that you can change the timer callback destination and the user data each time you reset the timer; whether it's expired or not. The handle that you pass in may remain the same if the timer is reset, but will change if the timer has already expired.

The first cut of this code was a bastardisation of the code used for SetTimer() and CancelTimer(). Not surprisingly there was a lot of duplication and, once the initial code was passing the tests we refactored it to remove the duplication. The result is that we have split SetTimer() and CancelTimer() into chunks that we can call from our new code as well as from the original call sites. CancelTimer() now calls RemoveTimer() to remove the timer data from the timer queue structures, if it exists, and then deletes the timer data. SetTimer() is broken into two pieces, first a call to CreateTimer() which is followed by a call to InsertTimer(), to place the timer data structure into the timer queue. ResetTimer() starts by calling RemoveTimer(), but if the timer exists and is returned it updates the existing timer data structure rather than deleting it. If the timer had already expired then no timer data is returned and instead CreateTimer() is called. Finally the new, or updated, timer data is added to the queue by calling InsertTimer().

Since ResetTimer() can be called to set a timer if it doesn't currently exist it makes sense to allocate a value to an "invalid" timer handle. This allows us to initialise new timer handles to the invalid value and then safely use this invalid handle in a call to ResetTimer() to set the timer for the first time. Now that we have this invalid value we can have CancelTimer() set the handle that it is passed to the invalid value to help protect client code from inadvertent handle reuse.

The timer data structure used to be a simple collection of three data items, it was a classic struct, all public data and no functionality. In our refactored code the timer data structure has picked up some functionality that is now needed in multiple places, and has become a fully fledged class. This change just seemed appropriate during the refactoring.

We add support for ResetTimer() to the threaded timer in the expected way...

With the addition of ResetTimer() we now have a class that is a drop in replacement for our original code. It's considerably simpler than the original code and doesn't exhibit the 49.7 day tick count roll over bug. However, there's one more thing to do before we can say that we're finished.

When we started to integrate this new code into our server we found that we simply had to change some class names and function signatures, moving from our original callback timer to the new threaded version. Unfortunately this meant that the server was coupled to our threaded implementation. This would make the server harder to test - it's hard to mock a concrete class. It also meant that the server code was coupled to a decision about how we run our timer queue. If that decision changed in the future then we'd need to adjust code that had no business being coupled to that decision... Sounds like we need an interface...

class IQueueTimers
{
   public :
 
      typedef ULONG_PTR UserData;
 
      class Timer;
 
      typedef ULONG_PTR Handle;
 
      enum
      {
         InvalidHandleValue = 0
      };
 
      class TimerData;
 
      virtual Handle SetTimer(
         Timer &timer,
         const DWORD timeoutMillis,
         const UserData userData) = 0;
 
      virtual bool ResetTimer(
         Handle &handle, 
         Timer &timer,
         const DWORD timeoutMillis,
         const UserData userData) = 0;
 
      virtual bool CancelTimer(
         Handle &handle) = 0;
 
   protected :
 
      ~IQueueTimers() {}
};

The interface can, conveniently, deal with all of the 'shared' nested types. This simplifies the design of the threaded version of the code as we no longer need to inherit from the timer queue to gain easy access to its nested types. So, rather than using private inheritance (which only worked on VS 6 anyway) we now, correctly, contain an instance of the original timer queue and delegate calls to it via our thread safe wrappers.

Now that we have an interface that provides the functionality that clients of the timer queue require we can decouple the client code from the concrete instances of the queue classes. The clients now interact via the IQueueTimers interface and aren't coupled to any particular implementation; this means we can mock out the timer queue for testing components that use it, if required.

We've come a long way since we first started trying to add tests to our existing timer queue. In the final instalment of the series we'll take a look at what we've achieved and see how TDD has helped us get to where we are now.

Code is here. Same rules as before.

Leave a comment