Understanding why is it unsafe to start a thread inside a constructor in terms of the Java memory model [duplicate]

Question

According to Java Concurrency in Practice, it is dangerous to start a thread within a class constructor. The reason is that this exposes the this pointer to another thread before the object is fully constructed.

Despite this topic being discussed in many previous StackOverflow questions, I am still having difficulty understanding why this is such a concern. In particular, I hope to seek clarity on whether starting a thread inside a constructor can lead to memory consistency problems from the point of view of the Java memory model.

Let me give you a concrete example of the kind of thing I want to do. (The desired output for this piece of code is for the number 20 to be printed to the console.)

private static class ValueHolder {

    private int value;
    private Thread thread;

    ValueHolder() {
        this.value = 10;
        thread = new Thread(new DoublingTask(this)); // exposing "this" pointer!!!
        thread.start();   // starting thread inside constructor!!!
    }

    int getValue() {
        return value;
    }

    void awaitTermination() {
        try {
            thread.join();
        } catch (InterruptedException ex) {}
    }

}

private static class DoublingTask implements Runnable {

    private ValueHolder valueHolder;

    DoublingTask(ValueHolder valueHolder) {
        this.valueHolder = valueHolder;
    }

    public void run() {
        System.out.println(Thread.currentThread().getName());
        System.out.println(valueHolder.getValue() * 2);  // I expect to print out 20...
    }

}

public static void main(String[] args) {
    ValueHolder myValueHolder = new ValueHolder();
    myValueHolder.awaitTermination();
}

Yes, I know that the thread is started before we return from the constructor. Yes, I know that the this pointer is exposed to the thread. And yet, I am convinced that the code is correct. I believe it will always print the number 20 to the console.

• The assignment this.value = 10 happens-before thread.start(). (This is because this.value = 10 precedes thread.start() in program order.)
• The call of thread.start() in the main thread happens-before the start of the .run() method in the newly-created thread. (Because thread.start() is a synchronising action.)
• The start of the .run() method happens-before the System.out.println(valueHolder.getValue() * 2); print statement. (Again, by program order.)

Therefore, by the Java Memory Model, the print statement should read the correctly-initialised value of valueHolder.value (namely, 10). So despite having ignored the advice from Java Concurrency in Practice, I still appear to have written a correct piece of code.

Have I made a mistake? What am I missing?

---

UPDATE: Based on the answers and comment, I now understand that my code example is functionally correct for the reasons I provided. However, it is bad practice to write code in this way because there is a chance that other developers will in future add further initialisation statements after the starting of the thread. One situation where such errors are likely is when implementing subclasses of this class.

Answer 1

Suppose I subclass your class. It may not have initialized its fields by the time they are required.

class BetterValueHolder extends ValueHolder
{
    private int betterValue;

    BetterValueHolder(final int betterValue)
    {
        // not actually required, it's added implicitly anyway.
        // just to demonstrate where your constructor is called from
        super();

        try
        {
            Thread.sleep(1000); // just to demonstrate the race condition more clearly
        }
        catch (InterruptedException e) {}

        this.betterValue = betterValue;
    }

    @Override
    public int getValue()
    {
        return betterValue;
    }
}

This will print zero, regardless of what value is given to the constructor of BetterValueHolder.