rspec/rules/S2445/csharp/rule.adoc

== Why is this an issue?

https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/statements/lock[Locking] on a class field synchronizes not on the field itself, but on the object assigned to it. Thus, there are some good practices to follow to avoid problems related to https://learn.microsoft.com/en-us/dotnet/standard/threading/threads-and-threading[thread] synchronization.

* Locking on a non-`readonly` field makes it possible for the field's value to change while a thread is in the code block, locked on the old value. This allows another thread to lock on the new value and access the same block concurrently.

+
[source,csharp]
----
private Color color = new Color("red");
private void DoSomething()
{
  // Synchronizing access via "color"
  lock (color) // Noncompliant: lock is actually on object instance "red" referred to by the "color" field
  {
    //...
    color = new Color("green"); // other threads now allowed into this block
    // ...
  }
}
----

* Locking on a new instance of an object undermines synchronization because two different threads running the same method in parallel will lock on different instances of the same object, allowing them to access the synchronized block at the same time.

+
[source,csharp]
----
private void DoSomething()
{
  lock (new object()) // Noncompliant: every thread locks on a different new instance
  {
    // ...
  }
}
----

* Locking on a string literal is also dangerous since, depending on whether the string is interned or not, different threads may or may not synchronize on the same object instance.

+
[source,csharp]
----
private readonly string colorString = "red";
private void DoSomething()
{
  lock (colorString)  // Noncompliant: strings can be interned
  {
    // ...
  }
}
----

== How to fix it

=== Code examples

==== Noncompliant code example

[source,csharp,diff-id=1,diff-type=noncompliant]
----
private Color color = new Color("red");
private void DoSomething()
{
  // Synchronizing access via "color"
  lock (color) // Noncompliant: lock is actually on object instance "red" referred to by the "color" field
  {
    //...
    color = new Color("green"); // other threads now allowed into this block
    // ...
  }
}
----

==== Compliant solution

[source,csharp,diff-id=1,diff-type=compliant]
----
private Color color = new Color("red");
private readonly object lockObj = new object();

private void DoSomething()
{
  lock (lockObj) 
  {
    //...
    color = new Color("green");
    // ...
  }
}
----

== Resources

* https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/statements/lock[Lock Statement] - lock statement - ensure exclusive access to a shared resource
* https://learn.microsoft.com/en-us/dotnet/api/system.string.intern[String.Intern] - `String.Intern(String)` Method
* CWE - https://cwe.mitre.org/data/definitions/412[CWE-412 - Unrestricted Externally Accessible Lock]
* CWE - https://cwe.mitre.org/data/definitions/413[CWE-413 - Improper Resource Locking]
* https://learn.microsoft.com/en-us/dotnet/standard/threading/threads-and-threading[Threads and threading]

ifdef::env-github,rspecator-view[]

'''
== Implementation Specification
(visible only on this page)

=== Message

Do not lock on writable field "xxx", use a readonly field instead.

Do not lock on a new instance because is a no-op, use a readonly field instead.

Do not lock on strings as they can be interned, use a readonly field instead.

=== Highlighting

locked object in `lock (xxx)` statement

'''
== Comments And Links
(visible only on this page)

include::../comments-and-links.adoc[]

endif::env-github,rspecator-view[]