C# Language Lock Statement Anti-Patterns and gotchas
Example
Locking on an stack-allocated / local variable
One of the fallacies while using lock is the usage of local objects as locker in a function. Since these local object instances will differ on each call of the function, lock will not perform as expected.
List<string> stringList = new List<string>();
public void AddToListNotThreadSafe(string something)
{
// DO NOT do this, as each call to this method
// will lock on a different instance of an Object.
// This provides no thread safety, it only degrades performance.
var localLock = new Object();
lock(localLock)
{
stringList.Add(something);
}
}
// Define object that can be used for thread safety in the AddToList method
readonly object classLock = new object();
public void AddToList(List<string> stringList, string something)
{
// USE THE classLock instance field to achieve a
// thread-safe lock before adding to stringList
lock(classLock)
{
stringList.Add(something);
}
}
Assuming that locking restricts access to the synchronizing object itself
If one thread calls: lock(obj) and another thread calls obj.ToString() second thread is not going to be blocked.
object obj = new Object();
public void SomeMethod()
{
lock(obj)
{
//do dangerous stuff
}
}
//Meanwhile on other tread
public void SomeOtherMethod()
{
var objInString = obj.ToString(); //this does not block
}
Expecting subclasses to know when to lock
Sometimes base classes are designed such that their subclasses are required to use a lock when accessing certain protected fields:
public abstract class Base
{
protected readonly object padlock;
protected readonly List<string> list;
public Base()
{
this.padlock = new object();
this.list = new List<string>();
}
public abstract void Do();
}
public class Derived1 : Base
{
public override void Do()
{
lock (this.padlock)
{
this.list.Add("Derived1");
}
}
}
public class Derived2 : Base
{
public override void Do()
{
this.list.Add("Derived2"); // OOPS! I forgot to lock!
}
}
It is much safer to encapsulate locking by using a Template Method:
public abstract class Base
{
private readonly object padlock; // This is now private
protected readonly List<string> list;
public Base()
{
this.padlock = new object();
this.list = new List<string>();
}
public void Do()
{
lock (this.padlock) {
this.DoInternal();
}
}
protected abstract void DoInternal();
}
public class Derived1 : Base
{
protected override void DoInternal()
{
this.list.Add("Derived1"); // Yay! No need to lock
}
}
Locking on a boxed ValueType variable does not synchronize
In the following example, a private variable is implicitly boxed as it's supplied as an object argument to a function, expecting a monitor resource to lock at.
The boxing occurs just prior to calling the IncInSync function, so the boxed instance corresponds to a different heap object each time the function is called.
public int Count { get; private set; }
private readonly int counterLock = 1;
public void Inc()
{
IncInSync(counterLock);
}
private void IncInSync(object monitorResource)
{
lock (monitorResource)
{
Count++;
}
}
Boxing occurs in the Inc function:
BulemicCounter.Inc:
IL_0000: nop
IL_0001: ldarg.0
IL_0002: ldarg.0
IL_0003: ldfld UserQuery+BulemicCounter.counterLock
IL_0008: box System.Int32**
IL_000D: call UserQuery+BulemicCounter.IncInSync
IL_0012: nop
IL_0013: ret
It does not mean that a boxed ValueType can't be used for monitor locking at all:
private readonly object counterLock = 1;
Now boxing occurs in constructor, which is fine for locking:
IL_0001: ldc.i4.1
IL_0002: box System.Int32
IL_0007: stfld UserQuery+BulemicCounter.counterLock
Using locks unnecessarily when a safer alternative exists
A very common pattern is to use a private List or Dictionary in a thread safe class and lock every time it is accessed:
public class Cache
{
private readonly object padlock;
private readonly Dictionary<string, object> values;
public WordStats()
{
this.padlock = new object();
this.values = new Dictionary<string, object>();
}
public void Add(string key, object value)
{
lock (this.padlock)
{
this.values.Add(key, value);
}
}
/* rest of class omitted */
}
If there are multiple methods accessing the values dictionary, the code can get very long and, more importantly, locking all the time obscures its intent. Locking is also very easy to forget and lack of proper locking can cause very hard to find bugs.
By using a ConcurrentDictionary, we can avoid locking completely:
public class Cache
{
private readonly ConcurrentDictionary<string, object> values;
public WordStats()
{
this.values = new ConcurrentDictionary<string, object>();
}
public void Add(string key, object value)
{
this.values.Add(key, value);
}
/* rest of class omitted */
}
Using concurrent collections also improves performance because all of them employ lock-free techniques to some extent.
