Test Resistant Code #5–Threading

I want to wrap up my test-resistant code series with one final type of code that proves hard to test, and that is multi-threaded code. We’ll just consider two scenarios, one that proves easy to test, another that proves very complex.

Separate out thread creation

A common mistake is to include the code that creates a thread (or queues a background worker) in the same class that contains the code for the actual work to be performed by the thread. This is a violation of “separation of concerns” In the following trivial example, the function we really need to unit test, DoStuff, is private, and the public interface is not helpful for unit testing.

public void BeginDoStuff()
{
    ThreadPool.QueueUserWorkItem((o) => DoStuff("hello world"));
}

private void DoStuff(string message)
{
    Console.WriteLine(message);
}

Fixing this is not hard. We separate the concerns by making the DoStuff method a public member of a different class, leaving the original class simply to manage the asynchronous calling and reporting of results (which you may find can be refactored into a more generic threading helper class).

Locks and race conditions

But what about locking? Consider a very simple circular buffer class I wrote for NAudio. In the normal use case, one thread writes bytes to it while another reads from it. Here’s the current code for the Read and Write methods (which I’m sure could be refactored down to something much shorter):

/// <summary>
/// Write data to the buffer
/// </summary>
/// <param name="data">Data to write</param>
/// <param name="offset">Offset into data</param>
/// <param name="count">Number of bytes to write</param>
/// <returns>number of bytes written</returns>
public int Write(byte[] data, int offset, int count)
{
    lock (lockObject)
    {
        int bytesWritten = 0;
        if (count > buffer.Length - this.byteCount)
        {
            count = buffer.Length - this.byteCount;
        }
        // write to end
        int writeToEnd = Math.Min(buffer.Length - writePosition, count);
        Array.Copy(data, offset, buffer, writePosition, writeToEnd);
        writePosition += writeToEnd;
        writePosition %= buffer.Length;
        bytesWritten += writeToEnd;
        if (bytesWritten < count)
        {
            // must have wrapped round. Write to start
            Array.Copy(data, offset + bytesWritten, buffer, writePosition, count - bytesWritten);
            writePosition += (count - bytesWritten);
            bytesWritten = count;
        }
        this.byteCount += bytesWritten;
        return bytesWritten;
    }
}

/// <summary>
/// Read from the buffer
/// </summary>
/// <param name="data">Buffer to read into</param>
/// <param name="offset">Offset into read buffer</param>
/// <param name="count">Bytes to read</param>
/// <returns>Number of bytes actually read</returns>
public int Read(byte[] data, int offset, int count)
{
    lock (lockObject)
    {
        if (count > byteCount)
        {
            count = byteCount;
        }
        int bytesRead = 0;
        int readToEnd = Math.Min(buffer.Length - readPosition, count);
        Array.Copy(buffer, readPosition, data, offset, readToEnd);
        bytesRead += readToEnd;
        readPosition += readToEnd;
        readPosition %= buffer.Length;

        if (bytesRead < count)
        {
            // must have wrapped round. Read from start
            Array.Copy(buffer, readPosition, data, offset + bytesRead, count - bytesRead);
            readPosition += (count - bytesRead);
            bytesRead = count;
        }

        byteCount -= bytesRead;
        return bytesRead;
    }
}

The fact that I take a lock for the entirety of both methods makes me confident that the internal state will not get corrupted by one thread calling Write while another calls Read; the threads simply have to take it in turns. But what if I had a clever idea for optimising this code that only involved me locking for part of the time. Maybe I want to do the Array.Copy’s outside the lock since they potentially take the longest. How could I write a unit test that ensured my code remained thread-safe?

Short of firing up two threads reading and writing with random sleep times inserted here and there, I’m not sure I know how best to prove the correctness of this type of code. Locking issues and race conditions can be some of the hardest to track down bugs. I once spent a couple of weeks locating a bug that only manifest itself on a dual processor system (back in the days when those were few and far between). The code had been thoroughly reviewed by all the top developers at the company and yet no one saw the problem.

Here’s another example, based on some code I saw in a product I worked on. A method kicks off two threads to do some long-running tasks and attempts to fire a finished event when both have completed. We want to ensure that the SetupFinished event always fires, and only fires once. You might be able to spot a race condition by examining the code, but how would we write a unit test to prove we had fixed it?

private volatile bool eventHasBeenRaised;

public void Init()
{    
    ThreadPool.QueueUserWorkItem((o) => Setup1());
    ThreadPool.QueueUserWorkItem((o) => Setup2());    
}

private void Setup1()
{
    Thread.Sleep(500);
    RaiseSetupFinishedEvent();
}

private void Setup2()
{
    Thread.Sleep(500);
    RaiseSetupFinishedEvent();
}

private void RaiseSetupFinishedEvent()
{
    if (!eventHasBeenRaised)
    {
        eventHasBeenRaised = true;
        SetupFinished(this, EventArgs.Empty);
    }
}

The only tool for .NET I have heard of that might begin to address this shortcoming is Microsoft CHESS. It seems a very promising tool although it seems to have stalled somewhat – the only integration is with VS2008; VS2010 is not supported. I’d love to hear of other tools or clever techniques for unit testing multi-threaded code effectively. I haven’t delved too much into the C# 5 async stuff yet, but I’d be interested to know how well it plays with unit tests.