Critical Blocks and Improper Lock Objects

The blog is moving to a new domain. This post has found a new home: http://www.patrickdelancy.com/2012/05/critical-blocks-and-improper-lock-objects/

Quick catch-up... critical application blocks are blocks of code that can only be executed by one thread at a time. Stock example for illustration:

public static class SmackTracker {
    private static int smackCount = 0;
    private static object locker = new object();

    public static void HitItAgain() {
        lock (locker) {
            smackCount++;
        }
    }
}

No matter how many threads call this method, the smackCount will only be incremented by one thread at a time, but this is not a post on the value and usage of thread locking...

What I want to cover is some common pitfalls people get into, sometimes without even realizing it. The resulting issues can sometimes manifest as phantom performance issues, inconsistent data corruption, intermittent memory exceptions and any number of other vague or elusive symptoms, like clammy hands.

Don't become a victim! When using sync objects, follow these simple rules...

1. Don't use publicly accessible sync objects.
2. Avoid using strings as lock objects.

Don't use publicly accessible sync objects.

The short explanation here is this... if the sync object is publicly accessible, it can be used (incorrectly or accidentally) for locking an external code block, resulting in potential deadlocks or other issues.

Historically it was common, even recommended, to use "this" or "typeof(MyType)" as your lock object. This is no longer recommended. Consider this example:

//Don't use public sync objects, and "this" is public!

using System;
using System.Threading;

namespace FacepalmCoding
{
    public class Program
    {
        public static void Main(string[] args)
        {
            DateTime start = DateTime.Now;
            
            Console.WriteLine("Starting worker threads...");
            
            Person theMan = new Person("The Man");
            
            Thread p2thread = new Thread(new ThreadStart(theMan.Operate));
            
            p2thread.Start();
            Thread.Sleep(10);
            
            lock (theMan) {
                theMan.OperateNoLock();
            }
            
            p2thread.Join();
            
            Console.WriteLine("This should have taken about 3 seconds in parallel, instead it took {0} seconds.", (DateTime.Now - start).TotalSeconds);            

            Console.WriteLine("Done.");
        }
    }
    
    public class Person {
        private readonly string personId;

        public Person(string id) {
            personId = id;
        }
        
        public void Operate() {
            DateTime start = DateTime.Now;
            
            //use private instance string as sync object for this resource
            lock (this) {
                // ... do stuff that takes 3 seconds ...
                Thread.Sleep(3000);
            }

            Console.WriteLine("Person '{0}' operation took {1} seconds to complete.", personId, (DateTime.Now - start).TotalSeconds);
        }
        
        public void OperateNoLock() {
            DateTime start = DateTime.Now;
            
            // ... do stuff that takes 3 seconds ...
            Thread.Sleep(3000);

            Console.WriteLine("Person '{0}' operation took {1} seconds to complete (No Lock).", personId, (DateTime.Now - start).TotalSeconds);
        }
    }
}

Starting worker thread...
Person 'The Man' operation took 3.0108053 seconds to complete.
Person 'The Man' operation took 3.0108053 seconds to complete (No Lock).
This should have taken about 3 seconds in parallel, instead it took 6.0216106 seconds.
Done.

Click here to see it in action.

Note in this example, "theMan" is the same object reference as "this" internally. The result... an internal critical code block is locked in an external scope. This is bad!

Avoid using strings as lock objects.

In .NET and Java (and probably others), strings are public sync objects. The lock is identified by the string value, instead of the string object reference. This means that any string value (doesn't have to be string literal) can be used to as a sync key globally throughout the entire process, as if you were referencing a global object, no matter the scope of the string variable you are using. That may be hard to follow... here is an example...

//Do not use strings as thread lock/sync objects!

using System;
using System.Threading;

namespace FacepalmCoding
{
    public class Program
    {
        public static void Main(string[] args)
        {
            Console.WriteLine("Starting worker threads...");
            
            Person firstPerson = new Person("first");
            Person secondPerson = new Person("second");
            Role firstRole = new Role("first");
            
            Thread p1thread = new Thread(new ThreadStart(firstPerson.Operate));
            Thread p2thread = new Thread(new ThreadStart(secondPerson.Operate));
            Thread r1thread = new Thread(new ThreadStart(firstRole.Operate));
            
            p1thread.Start();
            p2thread.Start();
            r1thread.Start();
            
            p1thread.Join();
            p2thread.Join();
            r1thread.Join();
            
            Console.WriteLine("Done.");
        }
    }
    
    public class Person {
        private readonly string personLocker = "lock object";
        private readonly string personId;

        public Person(string id) {
            personId = id;
            //personLocker = id;
        }
        
        public void Operate() {
            DateTime start = DateTime.Now;
            
            //use private instance string as sync object for this resource
            lock (personLocker) {
                // ... do stuff that takes 3 seconds ...
                Thread.Sleep(3000);
            }
            
            Console.WriteLine("Person '{0}' operation took {1} seconds to complete.", personId, (DateTime.Now - start).TotalSeconds);
        }
    }

    public class Role {
        private readonly string roleLocker = "lock object";
        private readonly string roleId;

        public Role(string id) {
            roleId = id;
            //roleLocker = id;
        }

        public void Operate() {
            DateTime start = DateTime.Now;
            
            //use different private string as sync object here
            //but since the string value is the same, will wait 
            //for "personLocker" to be released!
            lock (roleLocker) {
                // ... do stuff that takes 3 seconds ...
                Thread.Sleep(3000);
            }
            
            Console.WriteLine("Role '{0}' operation took {1} seconds to complete.", roleId, (DateTime.Now - start).TotalSeconds);
        }
    }
}

Starting worker threads...
Person 'first' operation took 3.0108053 seconds to complete.
Person 'second' operation took 6.0216106 seconds to complete.
Role 'first' operation took 9.0324159 seconds to complete.
Done.

Click here to see it in action.

Un-comment lines 40 and 62 and watch how the behavior changes.

Closing Thoughts

In case you forgot why any of this matters, I'll summarize it in a bullet list:

• Deadlocks and race conditions can be extremely difficult to diagnose and troubleshoot.
• Improper use of critical application blocks can cause deadlocks and race conditions.
• Widely-scoped sync objects can be accidentally abused.
• 'this', 'typeof' and any string value are all public sync objects.