concurrency - Why does this Java program terminate despite that apparently it shouldn't (and didn't)?

Question

A sensitive operation in my lab today went completely wrong. An actuator on an electron microscope went over its boundary, and after a chain of events I lost $12 million of equipment. I've narrowed down over 40K lines in the faulty module to this:

import java.util.*;

class A {
    static Point currentPos = new Point(1,2);
    static class Point {
        int x;
        int y;
        Point(int x, int y) {
            this.x = x;
            this.y = y;
        }
    }
    public static void main(String[] args) {
        new Thread() {
            void f(Point p) {
                synchronized(this) {}
                if (p.x+1 != p.y) {
                    System.out.println(p.x+" "+p.y);
                    System.exit(1);
                }
            }
            @Override
            public void run() {
                while (currentPos == null);
                while (true)
                    f(currentPos);
            }
        }.start();
        while (true)
            currentPos = new Point(currentPos.x+1, currentPos.y+1);
    }
}

Some samples of the output I'm getting:

$ java A
145281 145282
$ java A
141373 141374
$ java A
49251 49252
$ java A
47007 47008
$ java A
47427 47428
$ java A
154800 154801
$ java A
34822 34823
$ java A
127271 127272
$ java A
63650 63651

Since there isn't any floating point arithmetic here, and we all know signed integers behave well on overflow in Java, I'd think there's nothing wrong with this code. However, despite the output indicating that the program didn't reach the exit condition, it reached the exit condition (it was both reached and not reached?). Why?

---

I've noticed this doesn't happen in some environments. I'm on OpenJDK 6 on 64-bit Linux.

Answer 1

Obviously the write to currentPos doesn't happen-before the read of it, but I don't see how that can be the issue.

currentPos = new Point(currentPos.x+1, currentPos.y+1); does a few things, including writing default values to x and y (0) and then writing their initial values in the constructor. Since your object is not safely published those 4 write operations can be freely reordered by the compiler / JVM.

So from the perspective of the reading thread, it is a legal execution to read x with its new value but y with its default value of 0 for example. By the time you reach the println statement (which by the way is synchronized and therefore does influence the read operations), the variables have their initial values and the program prints the expected values.

Marking currentPos as volatile will ensure safe publication since your object is effectively immutable - if in your real use case the object is mutated after construction, volatile guarantees won't be enough and you could see an inconsistent object again.

Alternatively, you can make the Point immutable which will also ensure safe publication, even without using volatile. To achieve immutability, you simply need to mark x and y final.

As a side note and as already mentioned, synchronized(this) {} can be treated as a no-op by the JVM (I understand you included it to reproduce the behaviour).