Lock and Mutex are showing different results

Question

I was trying out some concepts related to lock and Mutex in C# Threading. However if found that using Mutex gave me correct results while that by using lock were inconsitent.

With lock construct:

class BankAccount
{
  private int balance;
  public object padlock = new object();
  public int Balance { get => balance; private set => balance = value; }

  public void Deposit(int amount)
  {
    lock ( padlock )
    {
      balance += amount;
    }
  }

  public void Withdraw(int amount)
  {
    lock ( padlock )
    {
      balance -= amount;
    }
  }

  public void Transfer(BankAccount where, int amount)
  {
    lock ( padlock )
    {
      balance = balance - amount;
      where.Balance = where.Balance + amount;
    }
  }
}

static void Main(string[] args)
{
  var ba1 = new BankAccount();
  var ba2 = new BankAccount();

  var task = Task.Factory.StartNew(() =>
  {
    for ( int j = 0; j < 1000; ++j )
      ba1.Deposit(100);
  });

  var task1 = Task.Factory.StartNew(() =>
  {
    for ( int j = 0; j < 1000; ++j )
      ba2.Deposit(100);
  });

  var task2 = Task.Factory.StartNew(() =>
  {
    for ( int j = 0; j < 1000; ++j )
      ba1.Transfer(ba2, 100);
  });

  Task.WaitAll(task, task1, task2);
  Console.WriteLine($"Final balance is {ba1.Balance}.");
  Console.WriteLine($"Final balance is {ba2.Balance}.");
  Console.ReadLine();
}

The code was giving incorrect balance for ba2 while ba1 was set to 0. This is the case even though each operation is surrounded by lock statement. It is not working correctly.

With Mutex construct:

class BankAccount
{
  private int balance;

  public int Balance { get => balance; private set => balance = value; }

  public void Deposit(int amount)
  {
    balance += amount;
  }

  public void Withdraw(int amount)
  {
    balance -= amount;
  }

  public void Transfer(BankAccount where, int amount)
  {
    balance = balance - amount;
    where.Balance = where.Balance + amount;
  }
}

static void Main(string[] args)
{
  var ba1 = new BankAccount();
  var ba2 = new BankAccount();

  var mutex1 = new Mutex();
  var mutex2 = new Mutex();

  var task = Task.Factory.StartNew(() =>
  {
    for ( int j = 0; j < 1000; ++j )
    {
      var lockTaken = mutex1.WaitOne();

      try
      {
        ba1.Deposit(100);
      }
      finally
      {
        if ( lockTaken )
        {
          mutex1.ReleaseMutex();
        }
      }
    }
  });

  var task1 = Task.Factory.StartNew(() =>
  {
    for ( int j = 0; j < 1000; ++j )
    {
      var lockTaken = mutex2.WaitOne();

      try
      {
        ba2.Deposit(100);
      }
      finally
      {
        if ( lockTaken )
        {
          mutex2.ReleaseMutex();
        }
      }
    }
  });

  var task2 = Task.Factory.StartNew(() =>
  {
    for ( int j = 0; j < 1000; ++j )
    {
      bool haveLock = Mutex.WaitAll(new[] { mutex1, mutex2 });
      try
      {
        ba1.Transfer(ba2, 100);
      }
      finally
      {
        if ( haveLock )
        {
          mutex1.ReleaseMutex();
          mutex2.ReleaseMutex();
        }
      }
    }
  });

  Task.WaitAll(task, task1, task2);
  Console.WriteLine($"Final balance is {ba1.Balance}.");
  Console.WriteLine($"Final balance is {ba2.Balance}.");
  Console.ReadLine();
}

With this approach I was getting correct balances every time I ran it.

I am not able to figure out why first approach is not working correctly. Am I missing something with respect to lock statements?

Answer 1

The main problem is with this line:

public int Balance { get => balance; private set => balance = value; }

You are allowing external code to meddle with the balance field, without the protection of the padlock. You also allow out-of-order reads of the balance field, because of the lack of a memory barrier, or even worse torn reads in case you later replace the int type with the more appropriate decimal.

The second problem can be solved by protecting the read with the padlock.

public int Balance { get => { lock (padlock) return balance; } }

As for the Transfer method, it can now be implemented without access to the other BankAccounts balance, like this:

public void Transfer(BankAccount where, int amount)
{
    Withdraw(amount);
    where.Deposit(amount);
}

This Transfer implementation is not atomic though, since an exception in the where.Deposit method could lead to the amount vanishing into thin air. Also other threads are not prevented from reading inconsistent values for the two BankAccounts Balances. This is why people generally use databases equipped with the ACID properties for this kind of work.

Answer 2

The two codes give the same results on my machine VS2017 .NET Framework 4.7.2 and work fine. So perhaps a difference with your system.

Final balance is 0.
Final balance is 200000.

Mutex are historically and originally for inter-process synchronization.

So in the process where the mutex is created, it is never locked against itself unless it was released like in the code provided in the question.

Using an operating system mutex object to synchronize threads is a bad practice and an anti-pattern.

Use Semaphore or Monitor within a process if having problems with lock and volatile.

Mutex : "A synchronization primitive that can also be used for interprocess synchronization."

Semaphore : "Limits the number of threads that can access a resource or pool of resources concurrently."

Monitor : "Provides a mechanism that synchronizes access to objects."

lock : "The lock statement acquires the mutual-exclusion lock for a given object, executes a statement block, and then releases the lock. While a lock is held, the thread that holds the lock can again acquire and release the lock. Any other thread is blocked from acquiring the lock and waits until the lock is released."

volatile : "The volatile keyword indicates that a field might be modified by multiple threads that are executing at the same time. The compiler, the runtime system, and even hardware may rearrange reads and writes to memory locations for performance reasons. Fields that are declared volatile are not subject to these optimizations. Adding the volatile modifier ensures that all threads will observe volatile writes performed by any other thread in the order in which they were performed. There is no guarantee of a single total ordering of volatile writes as seen from all threads of execution."

Hence you may try to add volatile:

private volatile int balance;

You can also set the locker object as static to be shared between instances if needed:

static private object padlock = new object();