Question about strange behavior of Atomic boolean while concurrent access is done

Question

I got such code:

public class ConcurrencyCheck {
    private volatile static AtomicBoolean top=new AtomicBoolean(false);
    private  int i=0;

    public class Toppler extends Thread{
        private final boolean bool;

        public Toppler(boolean myBool,String name) {
            super(name);
            bool=myBool;
        }

        @Override
        public void run() {
            while(!isInterrupted()){

                i++;
                synchronized (top) {
                    if (top.get() == bool) top.set(!top.get());
                    System.err.println(super.getName() + "  "+ bool +"->" + top + ". i is " + i);
                }

                try {
                    sleep(100);
                } catch (InterruptedException e) {
                    break;
                }
            }
        }
    }

    public static void main(String[] args) throws InterruptedException {
        ConcurrencyCheck cc = new ConcurrencyCheck();
        Thread t1= cc.new Toppler(true,"thread1");
        Thread t2= cc.new Toppler(false,"thread2");
        Thread t3= cc.new Toppler(true,"thread3");
        Thread t4= cc.new Toppler(false,"thread4");
        Thread t5= cc.new Toppler(true,"thread5");
        Thread t6= cc.new Toppler(false,"thread6");
        Thread t7= cc.new Toppler(true,"thread7");
        Thread t8= cc.new Toppler(false,"thread8");
        t1.start();
        ...
        t8.start();
        sleep(950);
        t1.interrupt();
        ...
        t8.interrupt();
    }
}

It is designed to check how AtomicBoolean works. Class Toppler is a thread that periodically topples Atomic boolean. The block of code which topples boolean is synchronized. As I guessed, each output line have to topple value of "top" variable, so output have to be "true->false false->true true->false...". But for some reason sometimes I can see such output:

thread1  true->false. i is 1
thread8  false->true. i is 8
thread7  true->false. i is 8
thread4  false->true. i is 8
thread6  false->true. i is 8
thread3  true->false. i is 8
thread5  true->false. i is 8
thread2  false->true. i is 8
thread1  true->false. i is 9
thread8  false->true. i is 10
thread7  true->false. i is 11
thread4  false->true. i is 12
thread6  false->true. i is 13
thread3  true->false. i is 14

The question is: why two consequent false->true or true->false output lines are possible?

Answer 1

tl;dr

You asked:

The question is: why two consequent false->true or true->false output lines are possible?

Two reasons:

• You cannot expect threads to run in any particular order. The JVM and host OS schedule threads for execution time on a CPU core at their own whim, in a manner unpredictable to us.
• I suspect that calls to System.err.println, like System.out.println, do not always appear on the console in chronological order.

➥ If you want to coordinate activities between threads, such as alternating their work in a particular pattern, you must take additional steps.

Details

The goal of your experimental code is not quite clear to me. Apparently each task is constructed with a target boolean value. If that task finds the currently flag variable top to match our target boolean, then we flip top to the opposite.

As for the interleaving of threads in numbered order of 1, 8, 7, 4, and so on, that is to be expected. There is no predicting the order of execution of threads. Which thread gets scheduled next for execution on a CPU core, and for how long to execute, is up to the JVM and host OS, depending on their algorithms and current runtime conditions. Never expect a set of threads to run in a certain order.

Your code has several issues.

One is that you are incrementing a shared int variable named i with no concurrency protection. You declare that int var as a class member of ConcurrencyCheck and then call i++ in each of your threads. That code incrementing i is not thread-safe, sharing a mutable resource across threads. That is why you are seeing failure with the values there. You should be using an AtomicInteger for that purpose of incrementing a counter.

Regarding synchronized (top) {, there is no need to mark that code synchronized as your code is currently written. The reason to use AtomicBoolean rather than a mere Boolean is to to be automatically thread-safe, obviating the need for synchronized. However, you are trying to do some other things at the same time: increment a counter, and report your progress with current values. So for that reason, to group together multiple other operations together atomically, you do need a synchronized, but you do not need a synchronized simply for using the AtomicBoolean by itself alone.

Regarding if (top.get() == bool) top.set(!top.get());, you should be using one of the sophisticated methods on AtomicBoolean to perform a get-and-set kind of operation atomically. That is the job of AtomicBoolean, to make such operations atomic. Specifically I believe you are looking for AtomicBoolean#compareAndExchange to set a new value if the expected value is found, and then return the found value.

Your message string for reporting results is confusing. I would write something more like this, where your bool var is renamed to expectedValue per terminology of the Javadoc for compareAndExchange. Caveat: I am no expert here, so verify my logic and verify my understanding of that Javadoc.

boolean witnessValue = top.compareAndExchange( this.expectedValue , ! this.expectedValue );
String msg = "Thread ID # " + Thread.currentThread().getId() + " expected: " + this.expectedValue + ", found: " + witnessValue + ", ended with top being: " + witnessValue + ". countAttempts: " + countAttempts + "." + " Now: " + Instant.now();

Regarding private volatile static AtomicBoolean top, there is no need to mark AtomicBoolean as volatile here. By initializing where declared, you have ensured there is already one AtomicBoolean object assigned. You never replace that object with another, so there is no chance of any thread having the visibility problem where the CPU core caches have different values. You should mark the field final to prevent you from mistakenly swapping out another object.

Another issue is that in modern Java we rarely need to address the Thread class directly. The executor service framework was added to Java 5 to relieve us of the chores of juggling threads. Define your task as a Runnable (or Callable), and submit an instance to the ExecutorService implementation of your choice (see Executors class).

Do not expect your console output to be in chronological order. As for order of output on System.err, I imagine like System.out that it is not chronological. I do not know if it is due to threads being suspended between the message content being generated inside the println parens and actually being passed to the println method, or if it is due to internal buffering issues, but I can tell you that I often see a series of println calls appearing not in chronological order on the console. I recommend (a) Always include a call to Instant.now or System.nanoTime, and (b) Collect your messages in a thread-safe List.