Blog Moved to http://podwysocki.codebetter.com/

Blog Moved to http://podwysocki.codebetter.com/
posts - 277 , comments - 156 , trackbacks - 27

Static Members and Threading Best Practices

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:
 

Print | posted on Wednesday, June 7, 2006 1:23 PM | Filed Under [ .NET C# ]

Feedback

Gravatar

# re: Static Members and Threading Best Practices

Good tip!

I'm still curious what the true difference between lock(this) and lock(_somePrivateField) really is?

In theory, you could build multiple "syncObjects" for locking, thus decreasing the potential for an actual wait, but in the single syncObject scenario whats the real difference, if any?
6/7/2006 2:32 PM | Eric Newton
Gravatar

# re: Static Members and Threading Best Practices

Well, the whole lock(this) pattern is a bad idea when it comes to the singleton pattern. What this does is it locks the current instance of your class.

Here is an example which might cause a deadlock:

public class ExternalClass
{
private static SingletonClass sInstance = SingletonClass.Instance;

public static void ChangeData()
{
lock(sInstance)
{
...
}
}
}

public class SingletonClass
{
private static SingletonClass instance;

public SingletonClass Instance
{
get
{
lock(this)
{
...
}
}
}
}

See, what I have above is during the Instance method, I am locking the instance of the object while I do some checks and other things. From the external class, I'm also locking the instance from the outside which could cause deadlock issues.

Depends on what you want to accomplish when you want to use multiple syncObjects. Is this for each separate static method you have?
6/7/2006 2:45 PM | Matthew Podwysocki
Gravatar

# Interesting Links for Today

Static Members and Threading Best Practises - the author uses a static field to lock the static member access as opposed to locking the class type
6/14/2006 4:54 PM |
Post A Comment
Title:
Name:
Email:
Comment:
Verification:
 

Powered by: