Why is the synchronized method not accessed synchronously in this multithreaded program?

Question

I've wrote some multithreading code in java and synchronized method that changed variable, but it doesn't synchronized my code, I still get random values. There is my code:

public class Main {
    public static void main(String[] args) throws Exception {
        Resource.i = 5;
        MyThread myThread = new MyThread();
        myThread.setName("one");
        MyThread myThread2 = new MyThread();
        myThread.start();
        myThread2.start();
        myThread.join();
        myThread2.join();
        System.out.println(Resource.i);
    }
}
class MyThread extends Thread {
    @Override
    public void run() {
        synMethod();
    }

    private synchronized void synMethod() {
        int i = Resource.i;
        if(Thread.currentThread().getName().equals("one")) {
            Thread.yield();
        }
        i++;
        Resource.i = i;
    }
}

class Resource {
    static int i;
}

Sometimes I get 7, sometimes 6, but I've synchronized synMethod, as I understand no thread should go at this method while some other thread executing this, so operations should be atomic, but they are not, and I can't understand why? Could you please explain it to me, and answer - how can I fix it?

Hollington

What you lock with synchronized matters. I suggest you not sub-class Thread as this can cause surprising results. – Leicestershire

Answer 1

Adding the synchronized method is like synchronizing on this. Since you have two different instances of threads, they don't lock each other out, and this synchronization doesn't really do anything.

In order for synchronization to take effect, you should synchronize on some shared resource. In your example, Resource.class could by a good choice:

private void synMethod() { // Not defined as synchronized
    // Synchronization done here:
    synchronized (Resource.class) {
        int i = Resource.i;
        if (Thread.currentThread().getName().equals("one")) {
            Thread.yield();
        }
        i++;
        Resource.i = i;
    }
}

Cathiecathleen

The read access to Resource.i also must be synchronized in order to ensure the proper visibility of the updates. – Une

@J.B You mean, reading Resource.i after join()ing on both threads? Well, join() creates a happens-before relation (All actions in a thread happen-before any other thread successfully returns from a join() on that thread), so that part is correct. – Forlini

Answer 2

Let's have a look at definition of synchronized methods from oracle documentation page.

Making the methods synchronized has two effects:

First, 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.

Coming back to your query:

synMethod() is a synchronized method object level. Two threads accessing same synchronized method acquire the object lock in sequential manner. But two threads accessing synchronized method of different instances (objects) run asynchronously in the absence of shared lock.

myThread and myThread2 are two different objects => The intrinsic locks are acquired in two different objects and hence you can access these methods asynchronously.

One solution : As quoted by Mureinik, use shared object for locking.

Other solution(s): Use better concurrency constructs like ReentrantLock etc.

You find few more alternatives in related SE question:

Avoid synchronized(this) in Java?

Martinamartindale