RSS RSS feed | Atom Atom feed
Popular Articles: Tom Riddle's Magical Diary | AJAX Lego Robot | AJAX CAPTCHA | SQL Multisets

Subtle Guarding Error

RAII / Guard Anti-Pattern

Threading a Background

(Skip this section if you know what critical sections and semaphores are used for)

A mutex or a semaphore is used in concurrent multi threaded applications where sections of code cannot simultaneously be run.

The schoolbook example is the ATM scenario. What would happen if you went to the ATM and withdraw money at the very same time your wife deposited some money on another ATM on the other side of town?

Our bank account class and deposit/withdrawal code look like this
class BankAccount 
{
private:
   double balance;

public:
   double getBalance() { return balance; }
   void   setBalance(double newBalance) { balance = newBalance; }

}

...
BankAccount bankAccount;
...

void deposit(double amount) { 
  double balance = bankAccount.getBalance();
  bankAccount.setBalance(balance + amount);
}

bool withdraw(double amount) { 
  double balance = bankAccount.getBalance();
  if (balance < amount) {
     return false; 
  }
  bankAccount.setBalance(balance - amount);
  return true;
}
Consider the following sequence of unfortunate events
  1. You and your wife have a $100 on your joint account
  2. You withdraw $10. Your thread execute the line double balance = bankAccount.getBalance();. Your thread now correctly thinks there is $100. At this moment in time, before any other instructions run, there is a context switch to your wife’s deposit thread.
  3. She deposits $50. Her thread execute double balance = bankAccount.getBalance();, increments the amount with $50 and sets the new balance to $150. So far so good. Her thread exists and your thread continues in the belief that the current balance is $100. It subtracts $10 and sets the new balance to be $90 when it should be $140!

The cure to the problem above is we need a way to tell the system that only one spouse can be in either the deposit or withdrawal code at any one time.

To do this we can for example use a semaphore like so
...
BankAccount bankAccount;
Semaphore lock;
...
void deposit(double amount) { 
  lock.acquire();
  double balance = bankAccount.getBalance();
  bankAccount.setBalance(balance + amount);
  lock.release();
}

bool withdraw(double amount) { 
  lock.acquire();
  double balance = bankAccount.getBalance();
  if (balance < amount) {
     return false; 
  }
  bankAccount.setBalance(balance - amount);
  lock.release();
  return true;
}
This would solve the concurrency problem for sure but introduces another potential problem.

What would happen if you forgot to call lock.release(); or if there was an exception being thrown in the code between the calls of acquire() and release()? In the case of an exception, the exception will "travel" up the call stack, until a method catches it. If no method does, the process exits.

Okay, lets add some exception handling then

void deposit(double amount) { 
  lock.acquire();
  bool needRelease = true;
  try {
    double balance = bankAccount.getBalance();
    bankAccount.setBalance(balance + amount);
  } catch(...) {
    lock.release();
    return;
  }
  lock.release(); 
}

This will solve our two problems. But it will spawn a new different kind of problem. As it is, it is cluttering and messy and hard to maintain.

The Guard Post Design Pattern

A great way to create clean exception safe code is to use the Guard Post design pattern which enables the locking of critical sections without the need of worrying about exceptions or forgetting about calling release. Two birds with one stone!

class GuardPost 
{
private:
  Semaphore* s;

public:
  GuardPost(Semaphore* _s) : s(_s) { s->acquire(); }   
  ~GuardPpost() { s->release(); }
}
...
void deposit(double amount) {
  GuardPost guardPost(&lock);
  double balance = bankAccount.getBalance();
  if (balance < amount) {
     return false; 
  }
  bankAccount.setBalance(balance - amount);
}
With this code a GuardPost object is allocated on the stack. In it's constructor the semaphore's acquire() function is called. The C++ compiler will generate calls to delete on all objects allocated on the stack before returning, regardless if there was an exception or not. Since we are calling the semaphore's release() function in the destructor, we don't have to worry about deadlocking due to exceptions or forgetting to call release();. So far so good. The code above is perfect as can be.

The Lazy Guards

The Lazy Guard Post anti-pattern looks like this
...
Semaphore semaphore;
...
// LazyGuard, a local scope class
class LazyGuard {
public:
  LazyGuard() { semaphore.acquire(); }
  ~LazyGuard() { semaphore.release(); }
}

void deposit(double amount) {
  LazyGuard lazy;
  double balance = bankAccount.getBalance();
  if (balance < amount) {
     return false; 
  }
  bankAccount.setBalance(balance - amount);
}
The laziness consists of that now there is no need to pass in the semaphore object anymore. This might be tempting if you have a class with many functions needing synchronization.

The Trap

However, laziness comes at a price. Consider this flawed code
void deposit(double amount) {
  LazyGuard lazy();
  double balance = bankAccount.getBalance();
  if (balance < amount) {
     return false; 
  }
  bankAccount.setBalance(balance - amount);
}
What is the difference?...Subtle eh? You could have easily added the parenthesis without thinking twice about it. Besides, many of you will still wonder how this is flawed?

LazyGuard g; //creates an object on the stack
LazyGuard* g = new LazyGuard(); //creates an object on the heap
LazyGuard g(); //DOES ABSOLUTLY NOTHING AT ALL!

With LazyGuard g() you are telling the compiler that you are declaring a C type function with the return type of LazyGuard!

Unless you implement this g() function somewhere, nothing will be executed. At the most, the compiler will issue a warning saying that LazyGuard g(void) was declared but never defined.
If there actually was a function with that signature and name, the code would be in real trouble since you are calling a function unintentionally.

Be lazy...but not to lazy

Laziness is the source of great inventions such as the remote control, power steering and the Guard Post pattern.
However, it is also the source of many evil things such as airplane crashes caused by negligence/laziness.
As everything else on the life on this planet, things can be used for good and bad. As a reminder to myself...the wise man knows what's good, and what's bad...and what's in between...and yes, I learnt this anti pattern the hard way :).

Links

For more background about synchronization and multithreading, see http://en.wikipedia.org/wiki/Mutual_exclusion
To view an online ATM deposit/withdrawal demonstration, see http://applecore.cs.williams.edu/~cs134/s02/lectures/Lecture35/ATM1.html
slashdot digg del.icio.us technorati [more]



Re: Subtle Guarding Error

Except, for this to happen, you have to:

1) have a compiler that allows nested function declarations. This is NOT ANSI standard c++, it is some weirdo vendor-specific extension.

2) ignore warning messages, which is not a best practice.

C.

Re: Subtle Guarding Error

Is this truly an anti-pattern? Looks like a bug, rather than an anti-pattern to me. What the author is trying to do is okay, but they made an implementation mistake.

Re: Subtle Guarding Error

There is a programming error in the non-lazy guard: the object needs to store a reference to a semaphore, not a semaphore. Currently, a copy of the semaphore gets locked and released, which probably is not what you want ;-)

Re: Subtle Guarding Error

The code should be correct since the GuardPost constructor is using GuardPost(Semaphore& _s) as opposed to GuardPost(Semaphore* _s)

Re: Subtle Guarding Error

No, Evert is quite correct. The member variable is a value, so it will be constructed using a copy constructor -- a new Semaphore instance. The member variable should be a reference as well as the constructor parameter.

Re: Subtle Guarding Error

My bad, I see what Evert meant now. Thanks. (example updated)

Add a comment Send a TrackBack