Practical Testing: 10 - Fixing the tick count wrap bug, again

| 0 Comments
Previously, on Practical Testing: Having bolted on some tests to an existing piece of code we're now doing some "agressive refactoring" ;) and rewriting the code from scratch using the testing ideas we developed earlier. The whole point of this exercise was to fix a known bug, we did that in the existing code here, now we have a test that forces us to address the issue in the new code.

Continuing in our test driven, simplest thing that could possibly work, redevelopment of the callback timer we're back at the point where we need to make the code work when GetTickCount() rolls over from a large number to 0 once ever 49.7 days. The solution that I came up with this time feels better than my last effort. The simplicity of the surrounding code seems to make it easier to work out how to solve the problem at hand.

Where before we simply tested to see if the next timeout was 'in the past' by being less than 'now' with code like this:

      const DWORD now = m_tickProvider.GetTickCount();
 
      if (now > m_nextTimeout)
      {
         timeout = 0;
      }
      else
      {
         timeout = m_nextTimeout - now;
      }

To be able to handle the fact that now may legitimately be less than a timeout and that timeout has still expired I decided to limit the range of allowable timeouts values so that we have an easy to detect range of invalid timeouts that must have already expired. Given that the scope of our tick count is 0 to 49.7 days, in milliseconds, I dont feel too bad about limiting valid timeouts to, say, 3/4 of that as it would still allow us to set timeouts up to 37 days in the future. The advantages of limiting the range are that we will have a set of values that cannot be in the future and therefore must be in the past. They can't be in the future because we don't allow timeouts larger than our limit to be set. With this in mind we change the code above to this:

      const DWORD now = m_tickProvider.GetTickCount();
 
      timeout = m_nextTimeout - now;
 
      if (timeout > s_timeoutMax)
      {
         timeout = 0;
      }

When our tick count wraps and 'now' becomes less than our next absolute timeout the value of timeout gets big and if it's bigger than the maximum timeout that we're allowed to set then we know the timer has expired.

Obviously chosing the value of our maximum is possibly a tricky thing to do. In practice, for the kind of timeouts I use the class for, I could probably set a maximum of an hour. 3/4 of the actual maximum tick count seems a reasonably 'generic' value that will work for most users, and we can allow the max value to be passed in to the constructor if a user knows better.

Because we've added a restriction the values that can be used in SetTimer() we need to enforce that limit in the code. Some would argue that we should use an assertion for this, but they're wrong ;), we'll use an exception.

CCallbackTimerQueue::Handle CCallbackTimerQueue::SetTimer(
   Timer &timer,
   const DWORD timeoutMillis,
   const UserData userData)
{
   if (timeoutMillis > m_maxTimeout)
   {
      throw CException(_T("CCallbackTimerQueue::SetTimer()"), _T("Timeout value is too large, max = ") + ToString(m_maxTimeout));
   }
 
   const DWORD now = m_tickProvider.GetTickCount();
...

We'll add some tests for this new restriction; I can't remember if I added them before I added the code, or after I added the code; it doesn't matter, it's just typing. Once that's working we'll add back the last of the original tests, the test that will make us move away from our 'simplest possible solution' and into the real world; the test for multiple concurrent timers. As expected, our current implementation fails this test, we'll fix that next time. Code is here. Same rules as before.

Leave a comment