Why is it not a good practice to synchronize on Boolean?

Question

My architect always says that

Never synchronize on Boolean

I am not able to understand the reason why and would really appreciate if someone could explain with an example as to why it is not a good practice. Reference Sample Code

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

Crisp

Your example code comes from a blog post explaining why it's bad? theothertomelliott.com/node/40 – Rachellerachis

@JamesMontagne: Yes, but i did not clearly understood the explanation. – Crisp

This would make for a great interview question! You've got one correct answer so far :-) – Eclair

@dasblinkenlight: In our current project, we are having lot of issues around this and my architect is pissed off...big time... – Crisp

@Crisp I can certainly understand her or him! There are only two instances of java.lang.Boolean objects in a running JVM, no matter how many Boolean variables you create. This creates a serious aliasing: everyone synchronized on the same pair objects, while their program makes it look like they synchronize on entirely different instances! – Eclair

@dasblinkenlight: can you explain in more, i want to learn more about it? – Crisp

Interestingly, nobody seems to have noticed that this will never turn on, it's going to stay off. – Cola

@X-Zero: Exactly because value of boolean is always false – Crisp

@Crisp You synchronize on objects that you intentionally share across threads that need mutual exclusion. You use one synchronization object per logical area that needs synchronization. There may be many such areas - potentially - as many as there are variables used in synchronized blocks in your program. Let's say you have ten areas that need mutual exclusion, so you create ten objects on which you synchronize. However, when your objects are Boolean, all your ten variables will point to only two objects - True and False, so your program will not work as you intended it to. – Eclair

@dasblinkenlight: I wish I could add points to your comment, simple, sweet and clear :) – Crisp

@Crisp Oh, don't worry about it, you gave me a great suggestion for interview question that lets you check candidate's understanding of several areas at once. – Eclair

@dasblinkenlight: Thanks for clarifying...me too have got an good interview question, different one from the standard set. – Crisp

@dasblinkenlight Also it's one of those rare questions that actually tests some useful knowledge of the interviewee. I've seen several hard to find race conditions where rather experienced programmers violated the general principle behind this - though Java does make that way too easy (synchronizing on this - e.g. nice bug if you also extend a thread instead of using runnable) – Outstretch

@X-Zero there was a typo in my original code example that I've since fixed on my blog page. Would edit the post here too, but that would be less than 6 characters. Should be "if(!isOn){" – Corey

Answer 1

I am not able to understand the reason why we should "never synchronize on Boolean"

You should always synchronize on a constant object instance. If you synchronized on any object that you are assigning (i.e. changing the object to a new object) then it is not constant and different threads will be synchronizing on different object instances. Because they are synchronizing on different object instances, multiple threads will be entering the protected block at the same time and race conditions will happen. This is the same answer for synchronizing on Long, Integer, etc..

// this is not final so it might reference different objects
Boolean isOn = true;
...
synchronized (isOn) {
   if (isOn) {
      // this changes the synchronized object isOn to another object
      // so another thread can then enter the synchronized with this thread
      isOn = false;

To make matters worse, any Boolean that is created through autoboxing (isOn = true) is the same object as Boolean.TRUE (or .FALSE) which is a singleton in the ClassLoader across all objects. Your lock object should be local to the class it is used in otherwise you will be locking on the same singleton object that other classes might be locking on in other lock cases if they are making the same mistake.

The proper pattern if you need to lock around a boolean is to define a private final lock object:

private final Object lock = new Object();
...

synchronized (lock) {
   ...

Or you should also consider using the AtomicBoolean object which means you may not have to synchronize on it at all.

private final AtomicBoolean isOn = new AtomicBoolean(false);
...

// if it is set to false then set it to true, no synchronization needed
if (isOn.compareAndSet(false, true)) {
    statusMessage = "I'm now on";
} else {
    // it was already on
    statusMessage = "I'm already on";
}

In your case, since it looks like you need to toggle it on/off with threads then you will still need to synchronize on the lock object and set the boolean and avoid the test/set race condition:

synchronized (lock) {
    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
    }
}

Lastly, if you expect the statusMessage to be accessed from other threads then it should be marked as volatile unless you will synchronize during the get as well.

Arguseyed

So if I do final boolean then it's ok? – Crisp

Can you give an example of AtomicBoolean ? – Crisp

I guess private final Object lock = new Object(); approach would be always recommended, right? – Crisp

In the case where you have to do something in both the off and the on case, then yes @Rachel. – Arguseyed

@Gary: Can you check dasblinkenlight's comment and add some stuffs to your answer focusing on boolean nature, i want to learn more about it. – Crisp

While that's all true and important the answer is missing one very important part: Never synchronize on objects you don't have control over and especially not on singletons that are shared throughout the whole architecture. This will work exactly once, but as soon as someone else has the same idea you've got big problems – Outstretch

@Voo: How it will create problem if someelse is also doing on singletons, can you give an example? There's so much to learn in threading area – Crisp

The issue in this case @Outstretch is that it is not immediately apparent that you code is working with a JVM singleton. The more important pattern that I mention covers the situation of Integer and other assignments. – Arguseyed

@Rachel, Voo is talking about the fact that Boolean x = true; and Boolean y = true; are now pointing to the same constant object. If you lock on x and someone else locks on y, even in another class, they will be locking on the same object. – Arguseyed

@Arguseyed Oh I agree. Important message #1 Always synchronize/lock on final objects. I just think an explanation of why lock = new Object() is important would also help, that it's the correct pattern to use we all agree on. – Outstretch

@Gary: So essentially, they are both locking on Boolean.TRUE object, this can get crazy if there are 5 to 6 similar locks in whole application. – Crisp

@Arguseyed - IIRC, this usage of AtomicBoolean will not force the synchronization of the statusMessage reference between processor caches unless unless statusMessage is declared volatile. But it would be subject to race conditions in any case. – Intelligentsia

Right @Rachel. That's why there never should be. Locking on a Boolean type is a bug. – Arguseyed

@Intelligentsia I was talking about the race conditions because there is no AtomicBoolean.toggle() method. – Arguseyed

truly eye-opening, about the instance difference. thanks – Jonme

@Arguseyed toggle() can be implemented using a while loop and compareAndSet – Outbreed

Agreed @ThomasTimbul. – Arguseyed

Answer 2

private Boolean isOn = false;
public void doSomeStuffAndToggleTheThing(){
   synchronized(isOn){

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:

Intelligentsia

Can you give an example of other bad code that would give rise to Deadlock condition – Crisp

Example? Any OTHER code that syncs on a Boolean, even if they call it something else besides isOn. There are only two Boolean objects, so all those threads with be competing with each other. Ouch. – Coaptation

+1 That's the real problem here. Although I think some more general explanation of why it's a bad idea to sync on objects you don't have complete control over would be even better - doing it on a singleton just makes the problem much more obvious. Edit: Comment and edit to the post were done at the same time, now it's a good bit clearer I think. – Outstretch

+1 I love the picture! You did not draw it to answer this question, did you? – Eclair

Answer 3

I think your problem is more with synchronized itself than with sync'ing on Booleans. Imagine that each Thread is a road, where statements (cars) go one after another. At some point there may be an intersection: without a semaphore collisions may happen. The Java language has a built in way to describe this: since any object can be an intersection, any object has an associated monitor acting as a semaphore. When you use synchronized in your code, you are creating a semaphore, thus you must use the same one for all the roads (threads). So this problem is not really boolean-specific because only two Booleans exist, this problem happens every time you synchronize on an instance variable and then point the same variable to a different object. So your code is wrong with Booleans, but is equally dangerous with Integers, Strings and any Object if you don't understand what's going on.

Ceil

Answer 4

All wrapper classes are immutable. One of the major reason why one should not synchronize on them.

As if 2 threads synchronize on wrapper class object and one of them modifies its value,it will be synchronized on a new/modified object and both threads will be synchronized on 2 different objects. So,The whole purpose of synchronization is lost.

Nicki

Answer 5

An update for Java 16:

Since version 16 Java takes the issue so seriously that a special jdk.internal.ValueBased annotation has been introduced for the classes that behave similar to Boolean. The documentation on Values-based classes states:

A program may produce unpredictable results if it attempts to distinguish two references to equal values of a value-based class, whether directly via reference equality or indirectly via an appeal to synchronization, identity hashing, serialization, or any other identity-sensitive mechanism

Among built-in JDK classes, annotated with ValueBased, are all primitive wrappers, classes in java.time and java.time.chrono packages, and even java.util.Optional. So, don't synchronize on these as well.

The logic of this prohibition seems pretty clear to me: for example, Integer has inner IntegerCache where first 128 integer values are cached. So, if you are going to synchronize, for example, on private final Integer mutex = Integer.of(10); this value will be shared between the all instances of all classes that have such construct (as @Gray very correctly pointed out - classes loaded with the same ClassLoader). private final Integer mutex = new Integer(10); might still be safe for synchronization, but Integer(int) constructor (as well as other similar) is deprecated "for removal" since version 9.

Lakeshialakey

Answer 6

Edit: Gray's answer is correct.

What I want to add is: Your architect is right, if from perspective of Boolean is immutable, why synchronize it? But multi thread is complex and based on the scenario.

Downtrodden

volatile is not enough because there is a race condition between the set and the get. You need a synchronized block to handle this. Also, Boolean values that are autoboxed are always singletons meaning that you should never synchronize on them. See @McDowell's answer. – Arguseyed

@Arguseyed I know what you mean totally. In this senario we should synchronize the logic block. I will correct my English statement. – Corundum

You sure you want to leave this answer here? – Arguseyed