I've written a class and many unit test, but I did not make it thread safe. Now, I want to make the class thread safe, but to prove it and use TDD, I want to write some failing unit tests before I start refactoring.
Any good way to do this?
My first thought is just create a couple threads and make them all use the class in an unsafe way. Do this enough times with enough threads and I'm bound to see it break.
There are two products that can help you there:
Both check for deadlocks in your code (via unit test) and I think Chess checks for race conditions as well.
Using both tools is easy - you write a simple unit test and the run your code several times and check if deadlocks/race conditions is possible in your code.
Edit:
Google has released a tool that checks for race condition in runtime (not during tests) that called thread-race-test.
it won't find all of the race conditions because it only analyse the current run and not all of the possible scenarios like the tool above but it might help you find the race condition once it happens.
The problem is that most of the multi-threading issues, like race conditions, are non-deterministic by their nature. They can depend on hardware behavior which you can't possibly emulate or trigger.
That means, even if you make tests with multiple threads, they will not be consistently failing if you have a defect in your code.
testNG or Junit with springframeworks test module (or other extension) has basic support for concurrency testing.
This link might interest you
you'll have to construct a test case for each concurrency scenario of concern; this may require replacing efficient operations with slower equivalents (or mocks) and running multiple tests in loops, to increase the chance of contentions
without specific test cases, it is difficult to propose specific tests
some potentially useful reference material:
I have seen people try to test this with standard unittests as you yourself propose. The tests are slow, and have so far failed to identify a single of the concurrency problems our company struggles with.
After many failures, and despite my love for unittests, I have come to accept that errors in concurrency is not one of unittests strengths. I usually encourage analysis and review in favour of unittests for classes where concurrency is a subject. With total overview of the system it is in many cases possible to prove/falsify claims of thread safety.
Anyhow I would love for someone to give me something that might point to the opposite, so I watch this question closely.
When I recently had to address the same problem I thought of it this way; First of all your existing class has one responsibility and that is to provide some functionality. It is not the objects responsibility to be thread safe. If it needs to be thread safe some other object should be used to provide this functionality. But if some other object is providing the thread safe-ness it cannot be optional because then you cannot prove your code is thread safe. So this is how I handle it:
// This interface is optional, but is probably a good idea.
public interface ImportantFacade
{
void ImportantMethodThatMustBeThreadSafe();
}
// This class provides the thread safe-ness (see usage below).
public class ImportantTransaction : IDisposable
{
public ImportantFacade Facade { get; private set; }
private readonly Lock _lock;
public ImportantTransaction(ImportantFacade facade, Lock aLock)
{
Facade = facade;
_lock = aLock;
_lock.Lock();
}
public void Dispose()
{
_lock.Unlock();
}
}
// I create a lock interface to be able to fake locks in my tests.
public interface Lock
{
void Lock();
void Unlock();
}
// This is the implementation I want in my production code for Lock.
public class LockWithMutex : Lock
{
private Mutex _mutex;
public LockWithMutex()
{
_mutex = new Mutex(false);
}
public void Lock()
{
_mutex.WaitOne();
}
public void Unlock()
{
_mutex.ReleaseMutex();
}
}
// This is the transaction provider. This one should replace all your
// instances of ImportantImplementation in your code today.
public class ImportantProvider<T> where T:Lock,new()
{
private ImportantFacade _facade;
private Lock _lock;
public ImportantProvider(ImportantFacade facade)
{
_facade = facade;
_lock = new T();
}
public ImportantTransaction CreateTransaction()
{
return new ImportantTransaction(_facade, _lock);
}
}
// This is your old class.
internal class ImportantImplementation : ImportantFacade
{
public void ImportantMethodThatMustBeThreadSafe()
{
// Do things
}
}
The use of generics makes it possible to use a fake lock in your tests to verify that the lock is always taken when a transaction is created and not released until transaction is disposed. Now you can also verify that the lock is taken when your important method is called. Usage in production code should look something like this:
// Make sure this is the only way to create ImportantImplementation.
// Consider making ImportantImplementation an internal class of the provider.
ImportantProvider<LockWithMutex> provider =
new ImportantProvider<LockWithMutex>(new ImportantImplementation());
// Create a transaction that will be disposed when no longer used.
using (ImportantTransaction transaction = provider.CreateTransaction())
{
// Access your object thread safe.
transaction.Facade.ImportantMethodThatMustBeThreadSafe();
}
By making sure the ImportantImplementation cannot be created by somebody else (by for example create it in the provider and make it a private class) you kan now prove your class is thread safe since it cannot be accessed without a transaction and the transaction always takes the lock when created and releases it when disposed.
Make sure the transaction is disposed correctly can be harder and if not you might see weird behaviour in your application. You can use tools as Microsoft Chess (as suggested in another anser) to look for things like that. Or you can have your provider implement the facade and make it implement it like this:
public void ImportantMethodThatMustBeThreadSafe()
{
using (ImportantTransaction transaction = CreateTransaction())
{
transaction.Facade.ImportantMethodThatMustBeThreadSafe();
}
}
Even though this is the implementation I hope you can figure out the tests to verify these classes as needed.
Note that Dror's answer does not explicitly say this, but at least Chess (and probably Racer) work by running a set of threads through all their possible interleavings to get repreoducible errors. They do not just run the threads for a while hoping that if there is an error it will happen by coincidence.
Chess for example will run through all interleavings and then give you a tag string that represents the interleaving that a deadlock was found on so that you can attribute your tests with the specific interleavings that are interesting from a deadlocking perspective.
I do not know the exact internal workings of this tool, and how it maps these tag strings back to code that you may be changing to fix a deadlock, but there you have it... I am actually really looking forward to this tool (and Pex) becoming part of the VS IDE.