Using lock statement in c#

Question

I need to use the lock construction, and edit the following methods to execute in parallel:

    public void Withdraw(int amountToWithdraw)
            {
                if (amountToWithdraw <= 0)
                {
                    throw new ArgumentException("The amount should be greater than 0.");
                }
    
                if (amountToWithdraw > MaxAmountPerTransaction)
                {
                    throw new ArgumentException($"The value {amountToWithdraw} exceeds transaction limit: {MaxAmountPerTransaction}.");
                }
    
                if (amountToWithdraw > Amount)
                {
                    throw new ArgumentException("Insufficient funds.");
                }
                
                WithdrawAndEmulateTransactionDelay(amountToWithdraw);
    
            }

Here is the result

private readonly object balanceLock = new object();
public void Withdraw(int amountToWithdraw)
        {
            if (amountToWithdraw <= 0)
            {
                throw new ArgumentException("The amount should be greater than 0.");
            }

            if (amountToWithdraw > MaxAmountPerTransaction)
            {
                throw new ArgumentException($"The value {amountToWithdraw} exceeds transaction limit: {MaxAmountPerTransaction}.");
            }

            if (amountToWithdraw > Amount)
            {
                throw new ArgumentException("Insufficient funds.");
            }

            lock (balanceLock)
            {
                WithdrawAndEmulateTransactionDelay(amountToWithdraw);
            }

        }

This is a description of the method WithdrawAndEmulateTransactionDelay which shouldn't be changed

private void WithdrawAndEmulateTransactionDelay(int amountToWithdraw)
        {
            Thread.Sleep(1000);
            Amount -= amountToWithdraw;
        }

However, the unit test failed. Where is the mistake in my code?

Answer 1

I'd also avoid all of those exceptions. Bad input is this code is often to be expected so it's not exceptional.

Try this code:

public TransactionStatus Withdraw(int amountToWithdraw)
{
    bool successful = false;
    string message = "OK";
    int balanceBefore = Amount;
    int balanceAfter = Amount;
    if (amountToWithdraw <= 0)
    {
        message = "The amount should be greater than 0.";
    }
    else if (amountToWithdraw > MaxAmountPerTransaction)
    {
        message = $"The value {amountToWithdraw} exceeds transaction limit: {MaxAmountPerTransaction}.";
    }
    else
    {
        lock (balanceLock)
        {
            if (amountToWithdraw > Amount)
            {
                message = "Insufficient funds.";
            }
            else
            {
                Thread.Sleep(1000);
                Amount -= amountToWithdraw;
                successful = true;
                balanceAfter = Amount;
            }
        }
    }
    return new TransactionStatus()
    {
        Successful = successful, Message = message, BalanceBefore = balanceBefore, BalanceAfter = balanceAfter
    };
}

public struct TransactionStatus
{
    public bool Successful;
    public string Message;
    public int BalanceBefore;
    public int BalanceAfter;
}

Answer 2

It seems, that you should put the last validation within the lock: in your current implementation it's possible that

1. Thread #1 tries to withdraw cash1, which is valid (cash1 < Account), validation's passed
2. Thread #2 tries to withdraw cash2, which is valid (cash2 < Account), validation's passed
3. However cash1 + cash2 > Account
4. Thread #1 calls for WithdrawAndEmulateTransactionDelay, now Amount == Amount - cash1 < cash2
5. Thread #2 calls for WithdrawAndEmulateTransactionDelay; since Amount - cash1 < cash2 you have the test failed

private readonly object balanceLock = new object();

public void Withdraw(int amountToWithdraw) {
  // These validations are not depended on Amount, they don't want lock
  if (amountToWithdraw <= 0)
    throw new ArgumentOutOfRangeException(nameof(amountToWithdraw), 
      "The amount should be greater than 0.");

  if (amountToWithdraw > MaxAmountPerTransaction)
    throw new ArgumentOutOfRangeException(nameof(amountToWithdraw), 
      $"The value {amountToWithdraw} exceeds transaction limit: {MaxAmountPerTransaction}.");

  // from now on we start using Amount, so we need the lock:
  lock (balanceLock) {
    if (amountToWithdraw > Amount)
      throw new ArgumentException("Insufficient funds.", nameof(amountToWithdraw));

    WithdrawAndEmulateTransactionDelay(amountToWithdraw);
  } 
}