Do Not Lock This

ID

csharp.do_not_lock_this

Severity

high

Remediation Complexity

trivial

Remediation Risk

low

Remediation Effort

low

Resource

Reliability

Language

CSharp

Tags

concurrency, reliability

Description

Reports lock statements whose target is a publicly reachable object. Locking on this, on a typeof(X) expression, or on a string literal exposes the lock to every caller that can reach the same object — typically the entire program — and creates a non-obvious risk of deadlock that is very hard to diagnose later.

Rationale

A lock is only useful when the locked object is private to the synchronisation. When the locked object is this, any external caller that happens to call lock(myObj) will block on the same monitor. The same applies to typeof(SomeClass), which returns a shared Type instance, and to string literals, which the runtime interns globally — two different files containing lock("MyKey") will lock on the same object.

The recommended pattern is to declare a private readonly object field used only for locking and to lock on that field.

public class Cache
{
    private readonly object sync = new object();

    public void Bad()
    {
        lock (this)              // FLAW — external callers can also lock(this)
        {
            // ...
        }
    }

    public void BadType()
    {
        lock (typeof(Cache))     // FLAW — Type objects are shared globally
        {
            // ...
        }
    }

    public void BadString()
    {
        lock ("globalKey")       // FLAW — interned string is shared across the process
        {
            // ...
        }
    }

    public void Good()
    {
        lock (sync)              // OK — private dedicated lock object
        {
            // ...
        }
    }
}

Remediation

Introduce a private readonly object field that is used only for synchronisation and lock on that field. Never expose the lock object to external code.

References