Anti-Patterns and gotchas C# with 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 stringList = new List(); 
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 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 list; 
public Base() 
{ 
this.padlock = new object(); 
this.list = new List(); 
} 
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 list; 
public Base() 
{ 
this.padlock = new object(); 
 

this.list = new List(); 
} 
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 values; 
public WordStats() 
{ 
this.padlock = new object(); 
this.values = new Dictionary(); 
} 
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 values; 
public WordStats() 
{ 
this.values = new ConcurrentDictionary(); 
} 
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.