Lock This! (not)

Having witnessed numerous demonstrations of all manner of concurrency gotchas at the recent DevWeek, I thought I would focus on one particular construct that I see quite often, and admittedly have been responsible for introducing in the past.

Joe Duffy, in his otherwise excellent (and extremely thick) book "Concurrent Programming on Windows", advises us to "Use the C# lock and VB SyncLock for all synchronized regions. Following this guidance ensures that locks will be released even in the face of asynchronous thread aborts, leading to fewer deadlocks (statistically speaking)."

There is a problem here, and the problem is that the lock keyword in C# is in league with Satan. It is as evil as the dreaded "On Error Resume Next" in VB. I cannot think of a single case where its use is justified (so if you can, let me know!). Let’s see why. As always the devil is in the detail.

Let’s look at what the lock keyword actually does. Look at the decompiled MSIL (using Reflector):

L_0009: call void [mscorlib]System.Threading.Monitor::Enter(object)
// synchronised code goes in here...

L_0052: leave.s L_005d
L_0054: ldloc.s CS$2$0001
L_0056: call void [mscorlib]System.Threading.Monitor::Exit(object)
L_005b: nop
L_005c: endfinally

So... when you use the lock keyword, or its even more evil mutant twin, [MethodImpl(MethodImplOptions.Synchronized)] - the true dark lord of attributes - what you are actually doing at an IL level is the equivalent of...

Monitor.Enter(syncLock);
try
{
//do something
}
finally
{
Monitor.Exit(syncLock);
}

Would you write code like this yourself? Would you?

Well not really... Calling Monitor.Enter() with no timeout value means the thread will block until the lock becomes available. Why is this bad? Well... blocking threads are the archenemy of concurrency!

As an example, if you are using the built in threadpool, when a thread becomes blocked the threadpool will start a new thread in anticipation of any new work items that come in. Now, what happens if your synchronised code block calls some resource on the network with a 5 minute timeout? "Kaboom!" is the technical term for it. So really whilst appearing as if it's being defensive, its lack of a timeout naturally leads to unreliable code. So much for using lock to help us wrangle threads.

So... if you really really really must block a thread (and you often don't where you think you do)... Please! For the sake of the children... use something more like this...

if (Monitor.TryEnter(syncLock, 1000)
{
try
{
//do something here
}
finally
{
Monitor.Exit(syncLock);
}
}

There's always time for a Timeout :-)