Wednesday, July 17, 2013

Threads: Why is it not good practice to synchronize on Boolean?

I was thinking to improve my threading fundamentals, so I planned to make few notes for myself that will assist me in future (Obviously at the time of Interviews). Following questions and answers have been taken from stackoverflow.com. This post is a sticky notes post for me. Please do not use the answers for reference purpose.

Here I got information to post for my reference.

Instead writing something new, I am taking the same example that is also copied from other blog post.

private Boolean isOn = false;
private String statusMessage = "I'm off";
public void doSomeStuffAndToggleTheThing(){
   // Do some stuff
   synchronized(isOn){
      if(isOn){
         isOn = false;
         statusMessage = "I'm off";
         // Do everything else to turn the thing off
      } else {
         isOn = true;
         statusMessage = "I'm on";
         // Do everything else to turn the thing on
      }
   }
}
As it turned out that it is not possible to get a neat lock on Booleans. As due to auto boxing mechanism (As you can get lock on objects only), true and false are Auto boxed to Boolean.True and Boolean.False objects. In a JVM, there can be only one object for Boolean.True and another object for Boolean.False. So application is written in a way that multiple threads locks on Boolean at various places, then it is possible that the complete code will be messed up without our knowledge, and it will be difficult to figure out the reason without knowing about Boolean classes.

Taken from the answer 2by McDowell
This is a terrible idea. isOn will reference the same object as Boolean.FALSE which is publicly available. If any other piece of badly written code also decides to lock on this object, two completely unrelated transactions will have to wait on each other. Locks are performed on object instances, not on the variables that reference them:
Any Boolean that is created through autoboxing (isOn = true) is the same object as Boolean.TRUE which is a singleton in theClassLoader across all objects. Your lock object should be local to the class it is used in. The proper pattern if you need to lock around a boolean is to define a private final lock object:


Let’s assume that our application does not have any other thread which is locking on Boolean object, then also above mentioned code is vulnerable and it is a good candidate for RACE Condition.

Let’s say one thread enters in the synchronized block when isOn was false. In this case Thread.False object is locked, and thread is in critical section of the code. Now isOn is changed to true, and after this another thread try to grab a lock on the same section. Thread.True is not locked with any other thread, which means that Thread 2 will grab the lock and will enter into critical section.

In this scenario mentioned above, both the threads are accessing critical section simultaneously which is not a desired scenario for us.

There are loads of ways you could fix this:
  • Synchronize on this
  • Synchronize on a private final Object specifically designated for the purpose (neater if someone else might extend our class)
  • Replace isOn with a final AtomicBoolean that you can alter using get and set methods rather than assignments (you’ll still need to synchronize for testing the state)
  • Redesign the class to avoid this sort of faffing about (such as using constant message Strings for each state)
This is a fairly subtle consequence of the Java abstraction, and could have caused me a huge headache if it had gone unchecked – leaving one of those wonderfully annoying intermittent bugs floating around (the kind that take forever to track down and debug).

BOTTOM LINE: Avoid using primitives types which might change in critical section, and be aware of Autoboxing.

No comments:

Post a Comment