Home » Coding » Dumb Stuff Not to do in Java: Synchronize on a Boolean

Dumb Stuff Not to do in Java: Synchronize on a Boolean

One of the great things about coding is that there’re always something new to learn: new techniques, new languages, and possibly most amusing of all, thousands of new ways to screw things up.

As a case in point, the other day, I wrote some code that synchronized on a Boolean variable, put out a code review (very useful practice – see The Joel Test), and within a few minutes, a colleague replied with the blunt, but informative “NO! You can’t synchronize on a Boolean”. Being an inquisitive sort, I enquired as to why, did some reading around, and messed with the code to break it.

Here’s an example to illustrate why:

private Boolean isOn = false;
private String statusMessage = "I'm off";
public void doSomeStuffAndToggleTheThing(){

   // Do some stuff

   synchronized(isOn){
      if(!isOn){
         isOn = true;
         statusMessage = "I'm on";

         // Do everything else to turn the thing on

      } else {
         isOn = false;
         statusMessage = "I'm off";

         // Do everything else to turn the thing off

      }
   }
}

So we have a thing that can be on or off. Our method will do a bit of work we don’t mind being run in parallel, then toggle our “thing” on or off depending on it’s current state. This will involve modifying several attributes of our class so we need to make sure this is atomic, which we do by synchronizing. We could synchronize the whole method, but we thought we’d be clever and only synchronize the key block where attributes change.

Admittedly, this is a massively contrived example, but you get the general idea.

Why This is Dumb

Thanks to the magic of autoboxing, you’re perfectly entitled to assign “true” to a Boolean object. However, the assignment isOn = true effectively points the reference isOn to a pre-created Boolean object representing a true value. This means that isOn is no longer the same object as it is when it was false.

Let’s say we had two threads, T1 and T2. They both call doSomeStuffAndToggleTheThing at roughly the same time. T1 reaches the synchronize statement first and acquires a lock on isOn, which is currently false. T1 sets isOn=true and at this point, T2 reaches the synchronize statement, and can get a lock on isOn because it’s now a completely different than the one on which T1 has a lock. So both threads are in the synchronized block at the same time, race to change our various attributes and leave the class in an inconsistent state.

Fixing it

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). So I figured it would be more than worth writing about, in the hope I might save someone else this trouble.

Bottom line: Don’t synchronize on references that might change.

Update (08/12/2011): Huge thanks to everyone that commented pointing out what was wrong with my original post. Hopefully I’ve cleared up those niggles.

  • Theothertomelliott Com

    You’re right. Use AtomicBoolean instead and forget about Synchronized…

  • Mcpdragon

    import android.app.Activity;
    import android.os.Bundle;
    import android.util.Log;

    public class MainActivity extends Activity {

    private static void Log(String msg) {
    Log.e(“ConcurrencyTest”, msg);
    }

    private static int LOCAL_INCREMENT = 1000000;

    private Thread mThread1 = null;
    private Thread mThread2 = null;
    private Thread mThread3 = null;

    private Boolean mIsLocked = false;
    private Object mLock = new Object();

    private int mVolitileInteger = 0;

        public void onCreate(Bundle savedInstanceState) {
            super.onCreate(savedInstanceState);
            setContentView(R.layout.main);
            
            mThread1 = new Thread(new Runnable() {
    public void run() {
    int localIncrement = 0;
    while (localIncrement != LOCAL_INCREMENT) {
    synchronized (mLock) {
    if (mIsLocked == false) {
    mIsLocked = true;

    // do work
    mVolitileInteger++;
    localIncrement++;
    // finish work

    mIsLocked = false;
    }
    }
    }

    Log(“Thread1 finished: ” + mVolitileInteger);
    }
            });
            
            mThread2 = new Thread(new Runnable() {
    public void run() {
    int localIncrement = 0;
    while (localIncrement != LOCAL_INCREMENT) {
    synchronized (mLock) {
    if (mIsLocked == false) {
    mIsLocked = true;

    // do work
    mVolitileInteger++;
    localIncrement++;
    // finish work

    mIsLocked = false;
    }
    }
    }
    Log(“Thread2 finished: ” + mVolitileInteger);
    }
            });
            
            mThread3 = new Thread(new Runnable() {
    public void run() {
    int localIncrement = 0;
    while (localIncrement != LOCAL_INCREMENT) {
    synchronized (mLock) {
    if (mIsLocked == false) {
    mIsLocked = true;

    // do work
    mVolitileInteger++;
    localIncrement++;
    // finish work

    mIsLocked = false;
    }
    }
    }
    Log(“Thread3 finished: ” + mVolitileInteger);
    }
            });
            
            mThread1.start();
            mThread2.start();
            mThread3.start();
        }
    }

    I think it could be synchronized using a Boolean object as above.
    Please tell me any opinion if it has logical defects or performance loss.

    • http://www.theothertomelliott.com Tom Elliott

      What’s your goal here?

      From what I can see, each thread would be skipping doing any work whenever someone else had a lock on mLock. It might be a bit clearer what was going on if your threads could inherit shared code (from what I can see the only different in each is the name they log).

  • Philip Robinson

    Of course you can synch on a Boolean, what you cannot synch on is any object you are going to reassign, so make the object final and all will be well.

  • Mark Thomas

    When T2 synchronizes on the new Boolean reference and enters the synchronized block while T1 is still in it, the if-statement that checks if(!jobRunning) will now return false because the new Boolean is true, which effectively prevents T2 from executing the main body of doJob() as intended.

    So while I agree with the general rule of not synchronizing on objects that can change object references, your code example could be a bit more clear.

    • Tom

      Thanks for the comment, you’re dead right. I’ll see if I can come up with a better example.

  • http://256.com/ Gray

    In your sample code however, you need to turn the boolean ON if it is OFF and OFF if it is ON.  This means that you _can’t_ use an AtomicBoolean without synchronizing on it because you will be doing two operations.  Given that you are synchronizing, you might as well just use the lock object.  It would be nice if AtomicBoolean had a toggle() method.

    • http://www.theothertomelliott.com Tom Elliott

      This is true. Note that I never said you wouldn’t need to synchronize on the AtomicBoolean. I’ll update to make this clearer.

  • http://256.com/ Gray

    Also, I think the code is wrong.  It says if (isOn) { isOn = true } …    I suspect you want to set it to be false if it is true and vice versa.

    • http://www.theothertomelliott.com Tom Elliott

      Well spotted, will edit.

  • Pingback: Why is it not good practice to synchronize on Boolean?()

  • Jeff Bischoff

    This post was very helpful to me. It reminded me to use an AtomicBoolean for this purpose, and not a Boolean.

    Thanks!

Social Widgets powered by AB-WebLog.com.