Synchronisation shock… you do not need Lock

|

When you write code that must work in a concurrent environment you may have to synchronise access a resource. It is very common to use a semaphore for this need; .NET is no different, and provides the Monitor class specifically for this purpose. You use the Monitor class like so:

private static object SyncObject = new object();
...
Monitor.Enter(SyncObject);
try
{
// thread sensitive code
}
finally
{
Monitor.Exit(SyncObject);
}

This code enters in to a critical section of code by using the Monitor.Enter() method, passing in a static sync object on which all threads can block; creating a mechanism to synchronise access to a thread sensitive resource. Then, after the code has completed its work with the resource, in the finally block, it exits the critical section with a call to Monitor.Exit() handing in the same sync lock object.

This was considered a common scenario by the C# designer, so common in fact a special keyword for exists to facilitate this pattern of Monitor usage; the keyword is called lock, and the following code that uses the lock keyword is semantically the same as the code shown above:

private static object SyncObject = new object();
...
lock(SyncObject)
{
// thread sensitive code
}

It is worth mentioning that the lock keyword does not actually do anything special; it simply emits the same code shown in the first code snippet in this post, by wrapping the use of the Monitor class in a try/finally guaranteeing that you exit the Monitor on exception.

While I would find it difficult to argue that this code is not simpler and cleaner than direct Monitor usage, I do assert that this is actually very rare scenario, and almost certainly not common enough to warrant a keyword all of its own! This may sound a little controversial but let me explain.

Consider for a moment that the vast majority of code that needs to be synchronised needs to also be atomic; therefore, if an exception is thrown during the execution of the code within the critical section you would not want your application to hobble on using potentially corrupt or broken data by blindly exiting the critical section. This would almost certainly put your application is an unstable state, creating intermittent and really tough to find and debug errors.

Now consider the scenario where you do not leave the critical section on error and simply report the exception and let the application hang the thread. Now you are in a situation that’s much easier to debug, as you’ll most likely be very near the call site where the error occurred, and your application is then not allowed to continue in an inconsistent state, which is a very good thing in my book.

Here’s a little sample code, showing what I think to be a more common scenario than that promoted by the use of the lock keyword:

private static object SyncObject = new object();
...
Monitor.Enter(SyncObject);
// thread sensitive code
Monitor.Exit(SyncObject);

Note this time there is no try/finally, so the code will stop on any unhandled exception (good thing). Obviously you can still wrap code of this nature in a try/catch, but then the normal rules of engagement come into play with regards to exception handling; it may even be appropriate to exit the critical section in the exception handler, and in that exception case you should probably use the lock keyword. However, I think that situation is rare; about as rare as actually handling a known exception, which of course you should only do when you can actually do something about it.

As usual if you have a comments, questions, flames, enhancements I would love to hear from you; but in the meantime happy multithreading, the future is concurrent and massively multithreaded, so think deeply and enjoy.

4 comments:

Dominic Betts said...

I think that the point you are making about the way that exceptions should be handled in threads is important. However the use of the lock keyword does help avoid the risk of your application hanging if locks aren't correctly released:

Example 1:

private static void DoWork1()
{
try
{
Monitor.Enter(SyncObject);
// Exception thrown somewhere in here...
Monitor.Exit(SyncObject);
}
catch (Exception e)
{
Console.WriteLine("Dealing with Exception");
}
}

I appreciate that this isn't great code, but you could easily end up with something like this, and this could cause your application to hang.

Example 2:
This is even worse code, and again could cause your application to hang

private static void DoWork2()
{
Monitor.Enter(SyncObject);
try
{
// Exception thrown somewhere in here...
Monitor.Exit(SyncObject);
}
catch (Exception e)
{
Console.WriteLine("Dealing with Exception");
}
}

Example 3:
This version, which I think is what you are proposing, won't cause the application to hang. The point is that you have to be very careful using just Monitor.Enter/Monitor.Exit.

private static void DoWork2()
{
Monitor.Enter(SyncObject);
try
{
// Exception thrown somewhere in here...
}
catch (Exception e)
{
Console.WriteLine("Dealing with Exception");
}
Monitor.Exit(SyncObject);
}

Using lock rather than Monitor.Enter/Monitor.Exit prevents you from creating code like that in Example 2, and the application won't hang for either example 1 or 3.

Paul said...

Hey Dominic, thanks for your comments. Actually in most cases I think you *do* want your application hang. The alternative is that your application hobbles on with corrupt data. The hang will actually be much easier to debug and find than a intermittent threading bug that manifests itself at random places in your code.

Thanks again .. always nice to get some feedback.

-PJ

alski said...

Paul,
First, I have to say I agree with you that lock() isn't perfect and many times I found myself using...

if (Monitor.TryEnter(x)) //or TryEnter(x, timeout)
{
try
{
...
}
finally
{
monitor.Exit(x);
}
}

However for me the important bit here is the use of finally which it seems is the bit you seem to find fault with. I would argue that the finally is important to avoid the application (or worse service) from grinding to a halt with a deadlock.

Having worked on solutions being moved to massively parallel, there were very often instances where code being migrated would throw exceptions for all sorts of reasons. Some of them were not fatal.

This for me is the distinction. Threading code is usually part of an application framework. Exceptions are more commonly part of the functionality. By not rolling back the lock with a finally, then you are proposing that the framework commutes all functional exceptions (which may be non-fatal) into a terminal halt to the process, even though the framework does not understand the exception context.

Finally I have one other point. I believe Monitor.Enter is re-entrant. To see this, simply run two threads, Start one first, and then inside a loop let it use lock to get a resource, Trace.Write "T1.Entered", wait for some milliseconds, then release, Trace.Write "T1.Exited" and wait for a small time again. Start a 2nd thread shortly after and have it perform exactly the same operation, start a loop, call Monitor.Enter, Trace.Write "T2.Entered" wait a bit, (Dont call monitor.Exit) Trace.Write "T2.out" and wait a bit more.

What you should see happening is that the first thread works happliy UNTIL the 2nd thread kicks in. The 2nd thread can Monitor.Enter multiple times and will complete its operations. T1 never gets to finish until you kill the process.

I believe that in a production system this would result in less predictable failures than we would get if we have a true lock. In fact I would argue that the problem you are trying to solve is one of atomicity. If inside the lock we are going to change multiple states then we have to ensure that we do not get a partial update. Either calculate all data up front then apply all changes together, or remember the state when we get the lock and rollback if we have any exceptions.

Nice to see you thinking about things, and thanks for the WPF posts, I’m on catch-up there. :)

Paul said...

Hey alski, thanks for your comments and thanks for reading.

-PJ