C# lock

Why is lock(this) {…} bad?

https://stackoverflow.com/questions/251391/why-is-lockthis-bad

It is bad form to use this in lock statements because it is generally out of your control who else might be locking on that object.

In order to properly plan parallel operations, special care should be taken to consider possible deadlock situations, and having an unknown number of lock entry points hinders this. For example, any one with a reference to the object can lock on it without the object designer/creator knowing about it. This increases the complexity of multi-threaded solutions and might affect their correctness.

A private field is usually a better option as the compiler will enforce access restrictions to it, and it will encapsulate the locking mechanism. Using this violates encapsulation by exposing part of your locking implementation to the public. It is also not clear that you will be acquiring a lock on this unless it has been documented. Even then, relying on documentation to prevent a problem is sub-optimal.

Finally, there is the common misconception that lock(this) actually modifies the object passed as a parameter, and in some way makes it read-only or inaccessible. This is false. The object passed as a parameter to lock merely serves as a key. If a lock is already being held on that key, the lock cannot be made; otherwise, the lock is allowed.

This is why it's bad to use strings as the keys in lock statements, since they are immutable and are shared/accessible across parts of the application. You should use a private variable instead, an Object instance will do nicely.

Run the following C# code as an example.

using Xunit.Abstractions;
using System;
using System.Threading;
using Xunit;
using Xunit.Abstractions;

namespace AssemblyTest
{
    public class TestBase 
    {
        protected readonly ITestOutputHelper Output;

        public TestBase(ITestOutputHelper tempOutput)
        {
            Output = tempOutput;
        }
    }
}

public class Person
    {
        public int Age { get; set; }
        public string Name { get; set; }

        public void LockThis()
        {
            lock (this)
            {
                System.Threading.Thread.Sleep(10000);
            }
        }
    }

    public class LockTest : TestBase
    {
        public LockTest(ITestOutputHelper tempOutput) : base(tempOutput)
        {
        }

        [Fact]
        public void Test()
        {
            var nancy = new Person {Name = "Nancy Drew", Age = 15};
            var a = new Thread(nancy.LockThis);
            a.Start();
            var b = new Thread(Timewarp);
            b.Start(nancy);
            Thread.Sleep(10);
            var anotherNancy = new Person {Name = "Nancy Drew", Age = 50};
            var c = new Thread(NameChange);
            c.Start(anotherNancy);
            a.Join();
        }

        private void Timewarp(object subject)
        {
            var person = subject as Person;
            if (person == null) throw new ArgumentNullException("subject");
            // A lock does not make the object read-only.
            lock (person.Name)
            {
                while (person.Age <= 23)
                {
                    // There will be a lock on 'person' due to the LockThis method running in another thread
                    if (Monitor.TryEnter(person, 10) == false)
                    {
                        Output.WriteLine("'this' person is locked!");
                    }
                    else Monitor.Exit(person);

                    person.Age++;
                    if (person.Age == 18)
                    {
                        // Changing the 'person.Name' value doesn't change the lock...
                        person.Name = "Nancy Smith";
                    }

                    Output.WriteLine("{0} is {1} years old.", person.Name, person.Age);
                }
            }
        }

        private void NameChange(object subject)
        {
            var person = subject as Person;
            if (person == null) throw new ArgumentNullException("subject");
            // You should avoid locking on strings, since they are immutable.
            if (Monitor.TryEnter(person.Name, 30) == false)
            {
                Output.WriteLine(
                    "Failed to obtain lock on 50 year old Nancy, because Timewarp(object) locked on string \"Nancy Drew\".");
            }
            else Monitor.Exit(person.Name);

            if (Monitor.TryEnter("Nancy Drew", 30) == false)
            {
                Output.WriteLine(
                    "Failed to obtain lock using 'Nancy Drew' literal, locked by 'person.Name' since both are the same object thanks to inlining!");
            }
            else Monitor.Exit("Nancy Drew");

            if (Monitor.TryEnter(person.Name, 10000))
            {
                string oldName = person.Name;
                person.Name = "Nancy Callahan";
                Output.WriteLine("Name changed from '{0}' to '{1}'.", oldName, person.Name);
            }
            else Monitor.Exit(person.Name);
        }
    }

 

Why does the lock object have to be static?

It isn't "very common to use a private static readonly object for locking in multi threading" - rather, it is common to use a lock at the appropriate / chosen granularity. Sometimes that is static. More often, IMO, it isn't - but is instance based.

The main time you see a static lock is for a global cache, or for deferred loading of global data / singletons. And in the latter, there are better ways of doing it anyway.

So it really depends: how is Locker used in your scenario? Is it protecting something that is itself static? If so, the lock should be static. If it is protecting something that is instance based, then IMO the lock should also be instance based.

 

lock statement (C# Reference)

https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/lock-statement

 

Locking

http://www.albahari.com/threading/part2.aspx#_Locking

C#’s lock statement is in fact a syntactic shortcut for a call to the methods Monitor.Enter and Monitor.Exit, with a try/finally block. Here’s (a simplified version of) what’s actually happening within the Go method of the preceding example:

Monitor.Enter (_locker);
try
{
  if (_val2 != 0) Console.WriteLine (_val1 / _val2);
  _val2 = 0;
}
finally { Monitor.Exit (_locker); }

Calling Monitor.Exit without first calling Monitor.Enter on the same object throws an exception.