Why does this critical section not work in Java?

Question

I am starting to learn about multithreading at uni and while I am beginning to get a grasp at the concept in general I am also playing around with some examples, to get a feel how this works in practice. My problem with the following class is:

It works like it is supposed to, if I synchronize the getters/setters with my Object lock, then the main thread sets the boolean done to true, the while loop in run() breaks and main just waits for the termination of the worker as stated trough join(). The last printout is made and I am happy. Here perhaps you can help me understand, why it is important to synchronize the getter/setter for done, but it seems to not matter if the getter for count is synchronized.

But my real problem is, I do not understand, why the worker thread never terminates, if I set the boolean done with the setter in a synchronized block/ critical section inside the main method. Then it seems to run forever and main waits for it, because of join(). On the other hand, when I let this run trough IntelliJ debugger it seems to eventually terminate, but with normal running, it just runs forever. Why is that so? To my understanding in the critical section, when the boolean is set to true, the running while loop in run() should notice this in a short timespan and terminate worker?

public class VisibilitySynchronized extends Thread {
    private static Object lock = new Object();
    private boolean done = false;
    public long counter = 1;

    public  boolean getDone() {
    //synchronized (lock) {
            return this.done;
    //}
    }

    public long getCounter() {
    //synchronized(lock) {
            return this.counter;
        //}
    }

    public void setDone(boolean changeDone){
        //synchronized(lock) {
            this.done = changeDone;
        //}
    }

    @Override
    public void run() {

        while (!getDone()) {
            counter++;
        }
    }



    
    public static void main(String[] args) throws InterruptedException {

        Thread main = Thread.currentThread();

        main.setName("I am the main thread executing the main method right now and am ybout to start 
        a worker of the class VisibilitySynchronized");

        System.out.println("Main Thread is: " + main.getName());


        VisibilitySynchronized worker = new VisibilitySynchronized();
        worker.start();

        Thread.sleep(1000);

        synchronized (lock) {
            worker.setDone(true);
        }
            //worker.setDone(true); this would be working if method was synchronized
            System.out.println("Waiting for other Thread to terminate...");

        System.out.println("Done in worker is " + worker.getDone());

        worker.join();


        System.out.printf("Done! Counted until %d.", worker.getCounter());

    }

}

Answer 1

I ran this code myself and when you have the synchronized blocks set up, it works (the yield() call returns quickly), but when you remove them, it doesn't (the app hangs, the yield() call seemingly never returning).

This is expected behaviour, more or less. If this code did return swiftly from that yield() call, that would ALSO be expected behaviour.

Welcome to the Java Memory Model.

Every thread gets an unfair coin. it is unfair, in that it is free to screw with you, and flip the same way every time, making it look like something works reliably, juuuust for it to fail when you're giving that demo to that important client. The basic rule is simple. If a thread ever flips the coin and the way your code runs differs depending on how it lands, you lost the game: You have a bug, and writing a test that catches it is virtually impossible. Writing threading code like this is therefore an act that requires extreme discipline.

The thread will flip the coin anytime it reads or writes a field. Each thread has a custom local clone copy of every field of every object* which it will read from and write to. But everytime it does so, it flips that evil coin. If the coin lands heads, it will arbitrarily copy its values to some or all other thread's copies, or copy the values from some other thread's local copy to itself. On tails, it doesn't do that and just uses its local copy.

In this case, you're observing a (near) endless coinflip of tails. In real life, you don't get a few million tails in a row, but did I mention the coin is unfair? It's 'normal' here. But the crucial point is: The VM makes no guarantees about that coin flip at all - a VM that flips heads every time (thus having yield() return quickly) is just as good, will pass the TCK, etcetera.

Thus: If ever the coin flip matters to how your code runs, you've messed up.

And you've done so here: worker-thread (the thread, not the object) reads its copy (which is false and remains false), and main-thread sets its copy of workerthread's (the object, not the thread)'s done flag to true. 1 field, but 2 cached copies, and now we're waiting for a coinflip of heads before that worker-thread ever manages to see what main-thread did.

Fortunately, we can force the VM to not flip that coin, and the route to do so is to establish a comes-before relationship.

The java memory model has written down a series of rules which establish that the VM will then acknowledge that 'event A happened before event B', and will guarantee that anything A did will be visible to B. No coin flip - B's copy will necessarily reflect what A did.

There's a long list but the important ones are:

• imperative: Within a single thread, any statement that runs before another came-before that other. This is the 'duh, obviously' one: In { foo(); bar(); }, anything foo does is visible to bar, because same thread.

• synchronized: Whenever a thread exits a synchronize-on-object-X block, that 'happens-before' any thread entering a synchronize-on-object-X block, if the thread entering does so afterwards.

• volatile: volatile is a keyword you can put on a field. This is a bit trickier; volatile is defined in terms of CB/CA too, but it's a little easier to consider that any writing to volatile variables forces the coin to flip heads, writing out the update to every thread's copy. That makes volatile seem amazing, but it comes at quite a cost: volatile is rather slow, and not quite as useful as you might initially think, because you can't use it to set up atomic operations.

• Thread start: Whenever you start a thread, anything the thread that starts the new thread did before calling .start() is visible to the new thread immediately.

Of course, if some method internally synchronizes on things, by combining the synchronized rule and the imperative rule, then that method effectively also serves as a comes-before-establishing factor. You should use this - a bunch of JDK methods are defined to do so!

So, toss the synchronized blocks back in, thus establishing CB/CA between main-thread writing the done flag and worker-thread reading it, and the code works (and will in fact work on every JVM, regardless of OS, CPU, phase of the moon, or VM vendor). Without it, this code can do whatever it wants. Exit, or not, or exit after 4 hours, or exit only when your winamp switches songs. All's fair then, because you wrote code that depends on the result of a coin flip, so it's on you. Alternatively, mark the field 'volatile', that'll do it too.

---

So, how do you write multicore java code if it's this much of a minefield??

The true answer is: You mostly don't. It's too complicated. Instead, you do this:

• isolate: If no thread interacts with any other at all (nothing reads/writes fields except stuff entirely local to that thread's goingson), then trivially all the coin flips in the world aren't going to change anything, so do that. Set up the thread properly before you start it, let the thread run to its end, and have it communicate the thing it produced (if you need a return value at all) via a safe channel, and don't worry about it.
• streamline comms: Instead of communicating via fields (here your threads 'communicate'; main communicates to worker in the form of that boolean), communicate via a channel that is designed for concurrent control: Use a DB (thread A writes to DB, thread B also does so and the same tables and rows as A; DBs have facilities to manage this), or a message queue. This can be a full blown library like rabbitMQ, or a collection type from java.util.concurrent, such as BlockingQueue. These have all solved the CB/CA issues for you.
• rely on a framework: A web framework has many threads and will call your 'web handler' code on one appropriately. The handlers chat via DB, the web framework takes care of all the threading: All your CPU cores are zooming along and you never need to worry about any coinflips. You can also use the framework at the opposite end and use e.g. fork/join or the concept of stream->map->filter->collect (a.k.a. MapReduce).

*) It doesn't really, I'm giving you a mental model of how to think about it so that you won't write this bug. The point is, it MAY make a clone.