C# lock and code analysis warning CA2002

Question

In my application I have a form that starts synchronization process and for number of reasons I want to allow only one synchronization to run at a time. So I've added a static bool field to my form indicating whether sync is in progress and added a lock to set this field to true if it wasn't already set so that first thread could start synchronization but when it's running every other thread that will try to start it will terminate.

My code is something like this:

internal partial class SynchronizationForm : Form
{
    private static volatile bool workInProgress;

    private void SynchronizationForm_Shown(object sender, EventArgs e)
    {
        lock (typeof(SynchronizationForm))
        {
            if (!workInProgress)
            {
                workInProgress = true;
            }
            else
            {
                this.Close();
            }
        }
    }
}

This is working well but when I run Code Analysis on my project I'm getting the following warning message:

CA2002 : Microsoft.Reliability : 'SynchronizationForm.SynchronizationForm_Shown(object, EventArgs)' locks on a reference of type 'Type'. Replace this with a lock against an object with strong-identity.

Can anyone explain to me what's wrong with my code and how can I improve it to make the warning gone. What does it mean that object has a strong-identity?

Answer 1

What is wrong is that you are locking on something public (typeof(SynchronizationForm)) which is accessible everywhere from your code and if some other thread locks on this same thing you get a deadlock. In general it is a good idea to lock only on private static objects:

private static object _syncRoot = new object();
...
lock (_syncRoot) 
{

}

This guarantees you that it's only SynchronizationForm that could possess the lock.

Answer 2

From the MSDN explanation of the rule

An object is said to have a weak identity when it can be directly accessed across application domain boundaries. A thread that tries to acquire a lock on an object that has a weak identity can be blocked by a second thread in a different application domain that has a lock on the same object.

Since you can't necessarily predict what locks another AppDomain might take, and since such locks might need to be marshalled and would then be expensive, this rule makes sense to me.

Answer 3

The problem is that typeof(SynchronizationForm) is not a private lock object, which means that any other piece of code could use it to lock on, which could result in deadlock. For example if some other code did this:

var form = new SynchronizationForm();
lock(typeof(SynchronizationForm))
{
    form.SomeMethodThatCausesSynchronizationForm_ShownToBeCalled();
}

Then deadlock will occur. Instead you should delcare a private lock object in the SynchronizationForm class and lock on that instead.

Answer 4

The System.Type object of a class can conveniently be used as the mutual-exclusion lock for static methods of the class.

Source: http://msdn.microsoft.com/en-us/library/aa664735(VS.71).aspx

To add to Doug's answer, what you have here is a locking mechanism which should only be used in static methods, being used in an instance method.