Lock taken on Me, on a Type object or on a string literal

ID

vbnet.correctness.do_not_lock_this

Severity

high

Remediation Complexity

medium

Remediation Risk

medium

Remediation Effort

medium

Resource

Concurrency

Language

VB.NET

Description

Reports a lock taken on an object that code outside this class can also reach: SyncLock Me, SyncLock GetType(SomeType), SyncLock someObject.GetType(), and SyncLock on a string literal. The explicit form is reported too - Monitor.Enter(Me) and Monitor.Enter(GetType(SomeType)). A lock on a private field held for that purpose, such as SyncLock _gate, is the correct pattern and is not reported. Locks taken on a Shared or non-private field are not reported, because deciding whether such a field is reachable from outside needs the whole declaration in view.

Rationale

A lock only works if every party that needs mutual exclusion agrees on the object, and only if no other party locks it for unrelated reasons. All the forms reported here break the second half of that. Me is reachable by anyone holding a reference to the instance, so a caller can write SyncLock cache around its own critical section and end up serialising against this class’s internals - or deadlock against it, since the two sides acquire unrelated locks in whatever order each happens to use. GetType(T) and x.GetType() are worse, because a Type object is a process-wide singleton: every assembly in the process that locks that type contends on the same object, including code you do not own, and the contention is invisible from either side. A string literal is the same hazard through a different route - literals are interned per process, so two unrelated components that both lock "cache" silently share one lock. These are not theoretical: the failure is a deadlock or a stall that appears only under concurrent load, in a build that passed every test, and it is caused by code in a different file from the one that has to change.

The following code illustrates the pattern detected by this rule:

Public Sub Store(ByVal key As String, ByVal value As String)
    ' FLAGGED: Lock taken on Me, on a Type object or on a string literal
    SyncLock Me
        _items(key) = value
    End SyncLock
End Sub

Remediation

Lock on a dedicated object that nothing outside the class can reach - declare Private ReadOnly _gate As New Object() and use SyncLock _gate. ReadOnly matters: if the field can be reassigned, two threads may end up locking different objects. For state shared across all instances, use a Private Shared ReadOnly object in the same way. Keep one lock object per independent piece of state rather than one for the whole class, so unrelated operations do not serialise against each other, and hold it for as little work as possible - never call out to code you do not control while holding it, which is how lock-ordering deadlocks arise. Where the shared state is a single collection, consider whether a concurrent collection such as ConcurrentDictionary removes the need for explicit locking altogether.

Configuration

This detector does not need any configuration.

References