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 cannot provide reliable mutual exclusion. Three families of bad target are covered: objects that are publicly reachable, targets whose identity varies per call, and targets whose value can be replaced while the lock is held.

Rationale

A lock is only useful when the locked object is private to the synchronisation, stable for the lifetime of the guarded state, and shared by exactly the threads that must exclude one another. Each family below breaks one of those conditions.

Publicly reachable. When the locked object is this, any external caller that happens to call lock(myObj) blocks on the same monitor, so code outside the type can deadlock it. typeof(SomeClass) returns a globally shared Type instance, and strings are interned by the runtime — two unrelated files containing lock("MyKey") lock the same object. A readonly string field is no better: readonly fixes the reference, while the instance it points at remains interned and shared.

Identity varies per call. A local variable or a parameter is evaluated afresh on every invocation, so two threads running the same method lock two different objects and neither excludes the other. This is synchronisation that compiles, runs, and protects nothing — the most dangerous variety, because the code looks correct.

The exception is a local or parameter captured by a lambda, anonymous method or local function that contains the lock: the captured variable lives in the closure created once, shared by every invocation of that delegate, so its identity is stable and the lock is correct. The rule tells the two apart by comparing the variable’s declaring method/lambda with the nearest enclosing method/lambda of the lock statement — a mismatch means the lock runs inside a closure that captured the variable from an outer scope, so no finding is reported.

Value can be replaced. A writable field may be reassigned while one thread holds the monitor. Later threads then lock the new instance and enter the critical section concurrently with the thread still holding the old one.

The recommended pattern is a private readonly object field used only for locking.

public class Cache
{
    private readonly object sync = new object();
    private object pending = 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 BadLocal()
    {
        var gate = new object();
        lock (gate)              // FLAW — every call locks a different object
        {
            // ...
        }
    }

    public void BadWritableField()
    {
        lock (pending)           // FLAW — the field can be reassigned mid-lock
        {
            // ...
        }
    }

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

    public Action GoodCapturedLocal()
    {
        var gate = new object();
        Action locker = () =>
        {
            lock (gate)           // OK — gate is captured by the lambda, so every call
            {                     // through "locker" shares the same instance
                // ...
            }
        };
        return locker;
    }
}

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.

If the lock target is currently a local or a parameter, promote it to such a field: the whole point is that all threads needing exclusion observe the same instance. If it is an existing field, add readonly — and if that field is a string, replace it with a dedicated object rather than relying on readonly, because interning still shares the instance process-wide.

References