Locking on Action Does Not Work

Question

I have come across some code that locks on an Action and have found that it does not work. Here is a (simplified and silly) version of the code:

class Program
{
    static void Main(string[] args)
    {
        var settings = new Settings(0);
        Action<int> threadAction = i =>
        {
            BusyBody.DoSomething(settings.GetANumber, settings.SetANumber, i);
        };
        ThreadPool.QueueUserWorkItem(delegate { threadAction(1); });
        ThreadPool.QueueUserWorkItem(delegate { threadAction(2); });
        Console.ReadLine();
    }

    class Settings
    {
        int i;

        public Settings(int i)
        {
            this.i = i;
        }

        public int GetANumber() => i;

        public void SetANumber(int i) => this.i = i;
    }

    class BusyBody
    {
        public static void DoSomething(Func<int> getInt, Action<int> setInt, int i)
        {
            lock (setInt)
            {
                Console.WriteLine("Getting int: " + getInt());
                Console.WriteLine("i " + i);
                setInt(i);
                Console.WriteLine("set");
            }
        }
    }
}

I would expect this to produce the following output:

Getting int: 0
i 1
set
Getting int: 1
i 2
set

OR

Getting int: 0
i 2
set
Getting int: 2
i 1
set

Depending on whichever thread gets through the lock first. However this isn't what I am seeing. Instead I see:

Getting int: 0
i 2
Getting int: 0
i 1
set
set

It looks like both threads enter the lock. Why does this happen? The Action being locked is the same method from the same object so shouldn't the references be the same?

Any help would be appreciated. Thanks!

Answer 1

The problem here is that you're locking on two different objects.

This line:

BusyBody.DoSomething(settings.GetANumber, settings.SetANumber, i);

is short for this:

BusyBody.DoSomething(new Func<int>(settings.GetANumber), new Action<int>(settings.SetANumber), i);

The two new Func<int>(...) and new Action<int>(...) expressions will produce new delegate objects on each call, thus you're not locking on the same object instance.

If you want to share these objects they have to be created once:

...
Func<int> getANumber = settings.GetANumber;
Action<int> setANumber = settings.SetANumber;
Action<int> threadAction = i =>
{
    BusyBody.DoSomething(getANumber, setANumber, i);
};
...