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

Join the thread immediately after start method. From this thread-1 will start and go to dead state after that thread-2 will start and go to dead state. So it will print your expected output always.

Change the code as shown below:-

public class Main{

    public static int sharedVar = 0;

    public static void main(String[] args)

        {
            MyThread mt1 = new MyThread();
            MyThread mt2 = new MyThread();

            try

                {
                    mt1.start();
                    mt1.join();
                    mt2.start();
                    mt2.join();
                }

            catch (InterruptedException e1)

                {
                    e1.printStackTrace();
                }

            System.out.println(sharedVar);

        }
}

class MyThread extends Thread
{
    private int times = 1000000;

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

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

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.

Answer 3

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 4

When a thread is about to execute a 'synchronized' instance method, it aqcuires the lock on the Object(to be precise, lock on that object monitor).

So in your case, Thread mt1 acquires lock on Object mt1 and Thread mt2 acquires lock on Object mt2 and they do not block each Other as the two threads are working on two different locks.

And when two threads modify a shared variable concurrently(not synchronized way), the result is unpredictable.

Well about the case of value 1000, for smaller inputs the interleaved execution might have resulted in correct result(luckily).

Sol : remove the synchronized keyword from addOne method and make sharedVal as type of 'AtomicInteger'

Answer 5

Define sharedVar as an AtomicLong instead of int. Making the function synchronized works as well but it is less efficient because you only need the increment to be synchronized.