Java Puzzler: busy wait threads stop working

Question

This is some sort of a Java Puzzler, that I stumbled across and can't really explain. Maybe somebody can?

The following program hangs after a short time. Sometimes after 2 outputs, sometimes after 80, but almost always before terminating correctly. You might have to run it a few times, if it doesn't happen the first time.

public class Main {
    public static void main(String[] args) {
        final WorkerThread[] threads = new WorkerThread[]{ new WorkerThread("Ping!"), new WorkerThread("Pong!") };
        threads[0].start();
        threads[1].start();

        Runnable work = new Runnable() {
            private int counter = 0;
            public void run() {
                System.out.println(counter + " : " + Thread.currentThread().getName());
                threads[counter++ % 2].setWork(this);
                if (counter == 100) {
                    System.exit(1);
                }
            }
        };

        work.run();
    }
}

class WorkerThread extends Thread {
    private Runnable workToDo;

    public WorkerThread(String name) {
        super(name);
    }

    @Override
    public void run() {
        while (true){
            if (workToDo != null) {
                workToDo.run();
                workToDo = null;
            }
        }
    }

    public void setWork(Runnable newWork) {
        this.workToDo = newWork;
    }
}

Now, it's clear that busy waiting loops are not a great idea in general. But this not about improving, it's about understanding what is happening.

Since everything works as expected when WorkerThread.setWork() is synchronized or when the WorkerThread.workToDo field is set to volatile I suspect a memory issue.

But why exactly is it happening? Debugging doesn't help, once you start stepping through, everything behaves as expected.

An explanation would be appreciated.

Answer 1

1. The first problem is that you are setting the Runnable workToDo from the main thread and then reading it in the 2 forked threads without synchronization. Any time you modify a field in multiple threads, it should be marked as volatile or someone synchronized.

   private volatile Runnable workToDo;
   

2. Also, because multiple threads are doing counter++ this also needs to be synchronized. I recommend an AtomicInteger for that.

   private AtomicInteger counter = new AtomicInteger(0);
   ...
   threads[counter.incrementAndGet() % 2].setWork(this);
   

3. But I think the real problem may be one of race conditions. It is possible for both threads to set the workToDo to be the Runnable and then have them both return and set it back to be null so they will just spin forever. I'm not sure how to fix that.

   1. threads[0] has it's `workToDo` set to the runnable.  It calls `run()`.
   2. at the same time threads[1] also calls `run()`.
   3. threads[0] sets the `workToDo` on itself and threads[1] to be the runnable.
   4. at the same time threads[1] does the same thing.
   5. threads[0] returns from the `run()` method and sets `workToDo` to be `null`.
   6. threads[1] returns from the `run()` method and sets `workToDo` to be `null`.
   7. They spin forever...
   

And, as you mention, the spin loop is crazy but I assume this is a demonstration thread program.

Answer 2

The problem occurs right between these lines:

workToDo.run();
workToDo = null;

Suppose the following sequence of events occurs:

- Original Runnable runs.  "Ping!".setWork() called
- Ping! thread realizes workToDo != null, calls run(), the stops between those two lines
  - "Pong!".setWork() called
- Pong! thread realizes workToDo != null, calls run()
  - "Ping!".setWork() called
- Ping! thread resumes, sets workToDo = null, ignorantly discarding the new value
- Both threads now have workToDo = null, and the counter is frozen at 2,...,80
  Program hangs

Answer 3

my 2 cents....

import java.util.concurrent.atomic.AtomicReference;

class WorkerThread extends Thread {
    private AtomicReference<Runnable> work;

    public WorkerThread(String name) {
        super(name);
        work = new AtomicReference<Runnable>();
    }

    @Override
    public void run() {
        while (true){
            Runnable workToDo = work.getAndSet(null);
            if ( workToDo != null ) {
                workToDo.run();
            }
        }
    }

    public void setWork(Runnable newWork) {
        this.work.set(newWork);
    }
}