Synchronized method does not work as expected

Question

I have a variable which is shared by two threads. The two threads will do some operations on it. I don't know why the result of sharedVar is different every time I execute the program.

public class Main
{
    public static int sharedVar = 0;
    public static void main(String[] args) 
    {
        MyThread mt1 = new MyThread();
        MyThread mt2 = new MyThread();
        mt1.start();
        mt2.start();

        try
        {
            // wait for the threads
            mt1.join();
            mt2.join();
        }
        catch (InterruptedException e1)
        {
            e1.printStackTrace();
        }

        System.out.println(sharedInt); // I expect this value to be 20000, but it's not
    }
}

The following is the class "MyThread"

public class MyThread extends Thread
{
    private int times = 10000;
    private synchronized void addOne()
    {
        for (int i = 0; i < times; ++i)
        {
            Main.sharedVar ++;
        }
    }

    @Override
    public void run()
    {
        addOne();
    }
}

The final result of sharedVar sometimes are 13735, 12508, or 18793; but never 20000, which is the result I expect. Another interesting thing about the program is when times=1000. I always get 2000 as the final result.

Can anyone explain this phenomenon?

Answer 1

A synchronized method protects the resource this that means that your code is equivalent to:

private void addOne()
{
    synchronized(this)
    {
        for (int i = 0; i < times; ++i)
        {
            Main.sharedVar ++;
        }
    }
}

But you have 2 objects for which addOne method is called. That means this for mt1.addOne is not the same than this for mt2.addOne and therefore you don't have a common resource of synchronization.

Try changing yout addOne code to:

private void addOne()
{
    synchronized(MyThread.class)
    {
        for (int i = 0; i < times; ++i)
        {
            Main.sharedVar ++;
        }
    }
}

And you will observe the expected behaviour. As the comments below suggest, it is better to use a different object than MyThread.class for synchronization since class objects are accesible from many points and it is easy that other code may try to synchronize using the same object.

Answer 2

When you use synchronized on non-static method, you use current object as monitor.

When you use synchronized on static method, you use current object of class (ClassName.class static field) as monitor.

In your case, you use synchronized on Thread's object (2 different instances), so two different threads will modify your sharedVar static field at same time.

You can fix it in different ways.

Move addOne method to Main and make it static.

private static synchronized void addOne(int times)
{
    for (int i = 0; i < times; ++i)
    {
        sharedVar++;
    }
}

Or you can create class called SharedVar with field private int var; and method synchronized void addOne(int times) and pass single instance of SharedVar to your treads.

public static void main(String[] args) 
{
    SharedVar var = new SharedVar();
    MyThread mt1 = new MyThread(var);
    MyThread mt2 = new MyThread(var);
    mt1.start();
    mt2.start();

    try
    {
        // wait for the threads
        mt1.join();
        mt2.join();
    }
    catch (InterruptedException e1)
    {
        e1.printStackTrace();
    }

    System.out.println(var.getVar()); // I expect this value to be 20000, but it's not
}

But if you need only one integer to be changed in multiple threads, you can use classes from java.til.concurrent.*, like AtomicLong or AtomicInteger.