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?

Side note: throwing ArgumentOutRangeException is better (more readable and exact) than ArgumentException in the context — Dmitrii Bychenko
– Dmitrii Bychenko, Commented May 26, 2022 at 10:30
You should also avoid throwing exceptions. It makes the calling code require exception handling and effectively introduces goto calls in your code. You're better off returning a result from the Withdraw method. — Enigmativity
– Enigmativity, Commented May 27, 2022 at 5:06

Dmitrii Bychenko · Accepted Answer · 2022-05-26 11:09:28Z

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

Thread #1 tries to withdraw cash1, which is valid (cash1 < Account), validation's passed
Thread #2 tries to withdraw cash2, which is valid (cash2 < Account), validation's passed
However cash1 + cash2 > Account
Thread #1 calls for WithdrawAndEmulateTransactionDelay, now Amount == Amount - cash1 < cash2
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); } }

Enigmativity · Accepted Answer · 2022-05-27 05:15:39Z

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

Collectives™ on Stack Overflow

Using lock statement in c#

2 Answers 2

Comments

Comments

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

Comments

Related