CODEWITHSUNDEEP
c#multithreadinglocking
shortspider
shortspider

Reputation: 1055

Locking on Action Does Not Work

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!

Upvotes: 3

Views: 53

Answers (2)

Lasse V. Karlsen
Lasse V. Karlsen

Reputation: 391336

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);
};
...

Upvotes: 1

Yacoub Massad
Yacoub Massad

Reputation: 27861

When you cast a method to an Action multiple times (when you pass settings.SetANumber), that does not mean that you get the same Action object. You get multiple Action objects that do the same thing.

Try this:

 Action<int> set_action = settings.SetANumber;

 Action<int> threadAction = i =>
 {
     BusyBody.DoSomething(settings.GetANumber, set_action, i);
 };

This makes sure that you have a single Action object.

Upvotes: 0

Related Questions