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 everything up.