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.

Afterwards

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) {
                ...
            }
        }
    }
}

Gunk

Thanks for your quick response very much ! – Afterwards

Now I can understand why the code works unexpected! Thanks for your help! – Afterwards

synchronized(this Integer variable) should work fine, but we shouldn't do that, we'd better use a business irrelevant variable, just as you mentioned, private static Object lock = new Object(). Am I right? – Afterwards

@Afterwards You are partly right: There is nothing wrong with synchronizing on an Integer variable except that, really, there is no such thing as synchronizing on a variable. You synchronize on an object, and in this case, the mistake is that each thread potentially synchronizes on a different object because count++ updates count to refer to a different Integer instance each time. – Bistro