Synchronization Fail in Java

Question

From the tutorial I read:

it is not possible for two invocations of synchronized methods on the same object to interleave. When one thread is executing a synchronized method for an object, all other threads that invoke synchronized methods for the same object block (suspend execution) until the first thread is done with the object.

However, in my simple example there is still a race competition to access the message object.

public class TestThread extends Thread{
    int thread;
    StringBuilder message;

    public TestThread(int thread, StringBuilder message) {
        this.thread=thread;
        this.message=message;
        start();
    }

    public void run() {
        synchronized(this){
            for (int i=0; i<1000000; i++) {
                double a=2*2;
            }
            modifyMessage();
        }
    }

    public synchronized void modifyMessage() {
        message.append(thread);
    }
}

public class TestMultithreading {
    static TestThread[] testThreads = new TestThread[5];

    public static void main(String args[]) {
        StringBuilder message = new StringBuilder("A");
        for (int i=0;i<5;i++)
            testThreads[i] = new TestThread(i, message);
        for (int i=0;i<5;i++)
            try {
                testThreads[i].join();
            } catch (InterruptedException e) {}
        out.println(message);
    }
}

I expect this to have a guaranteed output of a string of length 6. However, from time to time I see something like this:

A1034

This means that one of the threads failed to modify the object. Can someone explain me, why this happens and propose a solution for the problem?

Answer 1

You already answered your own question:

When one thread is executing a synchronized method for an object, all other threads that invoke synchronized methods for the same object block

The synchronized only block access on the method of the same object, which means that in every thread the modifyMessage() can be called at the same time

What you are looking for is a something like this:

   for (int i=0; i<1000000; i++) {
       double a=2*2;
   }
   synchronized(message){
       modifyMessage();
   }

Now the method is only call once per StringBuilder instance.

Answer 2

Your methods are all synchronizing on a different object (this).

If you change the method to synchronize on a single object, it'll work. For example.

public void modifyMessage() {
    synchronized(message) {
        message.append(thread);
    }
}