After reviewing some code examples out there, I decided it was time to look into static members and threading best practices. I see a lot of the same mistakes being made over and over. We can think of many times when we create static methods and properties that sometimes we forget that problems can arise.
Let's look at a code example:
private static int currentNumber = 0;
public static int NextNumber
{
get
{
int newNumber = currentNumber;
newNumber += 1;
// Do more stuff here
currentNumber = newNumber;
return currentNumber;
} // get - NextNumber
} // property - NextNumber
What's wrong with the above code? Well, we are not protecting access to the currentNumber member variable which could lead to a race condition. A race condition is a bug that occurs when the outcome of an application depends on which of two or more threads reaches a particular block of code first. Running this application many times produces different results, and the results cannot be predicted.
This kind of reminds of a bad joke I heard:
Knock-knock
New Incoming Thread
New Incoming Tr---
Knock-Knock
So, let's try rewriting it while using the
lock statement to lock our resource so that only one person can modify it at a time:
private static int currentNumber = 0;
public static int NextNumber
{
get
{
lock(typeof(CounterClass))
{
int newNumber = currentNumber;
newNumber += 1;
// Do more stuff here
currentNumber = newNumber;
return currentNumber;
} // lock - typeof(CounterClass)
} // get - NextNumber
} // property - NextNumber
Did we solve the race condition issue? It is true that we solved the race condition, but we just introduced another problem of thread deadlocking.
For a given type, there is only one instance of System.Type per AppDomain. Putting a lock on a System.Type instance takes a lock that affects the entire process, not just the AppDomain. If one AppDomain takes a lock on a Type object then that thread abruptly aborts, it will not release the lock. This lock then may cause other application domains to deadlock. Sound bad?
What about lock(this)? It is ok to take a lock on an individual object that is publicly accessible. However, if the object follows a singleton pattern that might cause an entire subsystem to deadlock. If other code in your application, external to current object, puts a lock on the object, deadlocks could occur.
Now let's go ahead and fix this code:
private static object syncRoot = new object();
private static int currentNumber = 0;
public static int NextNumber
{
get
{
lock(syncRoot)
{
int newNumber = currentNumber;
newNumber += 1;
// Do more stuff here
currentNumber = newNumber;
return currentNumber;
} // lock - syncRoot
} // get - NextNumber
} // property - NextNumber
What we have done is created an external object to lock while we are modifying the underlying data. Let's take another look at an example of the Singleton pattern.
private static object internalSyncRoot;
private static object InternalSyncRoot
{
get
{
if (internalSyncRoot == null)
{
object obj = new object();
Interlocked.CompareExchange(ref internalSyncRoot, obj, null);
} // if - internalSyncRoot
return internalSyncRoot;
} // get - InternalSyncRoot
} // property - InternalSyncRoot
Now that we have the is the SyncRoot of our Singleton class. We used the Interlocked class which provides atomic operations for variables that are shared by multiple threads. The CompareExchange method compares objects and if they are different, it replaces the first parameter if the second is different.
We need to now create the Singleton pattern Instance property. Below is the code for that:
private static CounterClass instance;
public static CounterClass Instance
{
get
{
if (instance == null)
{
lock(InternalSyncRoot)
{
if (instance == null)
{
CounterClass counter = new CounterClass();
// Set other properties
instance = counter;
} // if - instance
} // lock - InternalSyncRoot
} // if - instance
return instance;
} // get - Instance
} // property - Instance
What I did above was the double lock check to check whether the instance had been already created, then it locks our Internal SyncRoot and checks again before creating the instance and setting it.
Conclusion
Overall, we should be careful about our static data. We should also determine whether it makes to publish the data as static or make it instance based. The .NET Framework is not thread-safe by default, as most applications operate on a single thread, so you will not run into many of these problems.
Also keep in mind the following points:
* Make static data thread-safe by default
* Do not make instance data thread-safe by default
* Avoid lock(typeof(MyClass))
* The use of lock(this) should be kept to a minimum on a public type
Here is more reading on the subject:
