synchronized, not always true?

Question

Does the synchronized block always works fine? I am confused now! Am I wrong when using synchronized keyword?

The code snippet is as following:

package com.company;

public class Main {

    public static void main(String[] args) {

        for (int i = 0; i < 100; i++) {
            new MyThread().start();
        }
    }
}

class MyThread extends Thread {

    // no atomic
    private static Integer count = 0;

    @Override
    public void run() {
        while (true) {
            synchronized (count) {
            //synchronized (this) {

                if (count > 100)
                    break;

                count++;
                System.out.println("ThreadId:" + currentThread().getId() + ","
                        + "inc MyThread.count to : " + count);
            }

        }
    }
}

the result is:

ThreadId:10,inc MyThread.count to : 2
ThreadId:9,inc MyThread.count to : 2
ThreadId:9,inc MyThread.count to : 4
ThreadId:9,inc MyThread.count to : 5
ThreadId:9,inc MyThread.count to : 6
....

I don't know why Thread 10 and 9 output the same value.

Answer 1

You are doing absolutely the wrong thing. First, you synchronize on the updated field. The synchronization will be kept on the object which was in the count variable upon synchronization block entry, but you modify it later. So if the second thread is started when first one already incremented a value, then first will be synchronized on Integer.valueOf(0) and second on Integer.valueOf(1) and they will not wait for each other.

The second problem is that you synchronizing on the Integer which is cached for small values and may be reused in completely different part of program, so you may end up waiting for something completely unrelated.

Note that synchronization on this will not help either as this is the MyThread object which is different in every thread.

You can fix your code synchronizing on some shared object which does not change through the execution. It's better to create a special lock object for this purpose:

class MyThread extends Thread {
    private static final Object lock = new Object();
    // no atomic
    private static Integer count = 0;

    @Override
    public void run() {
        while (true) {
            synchronized (lock) {
                ...
            }
        }
    }
}