Why my synchronized method not working properly?

Question

I have this synchronized method that prints counter, I have 4 Threads so I am expecting final value of my counter to be 400000 as my counter is a static variable.

but every time I run my code, it is giving me different values of counter.

Following is my code:

class MyThread implements Runnable{

    private static int counter=1;
    @Override
    public void run() {
        try {
            this.syncMethod();
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
    }
    public synchronized void syncMethod() throws InterruptedException{
        for(int i=0;i<100000;i++){
            System.out.println(Thread.currentThread().getName()+" : "+counter++);
        }
    }
}

public class MyController {
    public static void main(String[] args) throws InterruptedException {
        Runnable r1=new MyThread();
        Runnable r2=new MyThread();
        Runnable r3=new MyThread();
        Runnable r4=new MyThread();
        Thread t1;
        Thread t2;
        Thread t3;
        Thread t4;
        t1=new Thread(r1,"Thread 1");
        t2=new Thread(r2,"Thread 2");
        t3=new Thread(r3,"Thread 3");
        t4=new Thread(r4,"Thread 4");

        t2.start();
        t1.start();
        t3.start();
        t4.start();
    }
}

Answer 1

The variable is static, but the method that you synchronized is not static. This means that it will acquire the monitor on the current instance, and every thread has a different current instance.

A simple solution is to make the syncMethod method static as well; in that case, it will take a lock on the monitor that is shared by all instances of the MyThread class:

public static synchronized void syncMethod()

Answer 2

Erwin Bolwidt's answer is right to solve your problem. As another way to increment a static shared counter in multiple threads safely, you can turn to AtomicLong.

Define it as this:

private static AtomicLong counter = new AtomicLong();

Increment it as:

counter.getAndIncrement();

And in the end, get the result:

counter.get();

Answer 3

synchronised key word in non static methods means exactly synchronize me for this methods : this two code a striclty equivalent :

public synchronised void dojob(){
     //the job to do 
}

et

public void dojob(){
     synchronised (this){
     //the job to do 
     }
}

in your case your synchronized methods are synchronizing on different object (t1,t2,t3 and t4) so didn't block them each other . the best solution is thaat your thread will use a common object to synchronized each other. an other point it alway better to get its thread back to do this call join here is a code to do what you want with this 2 fixes

class MyThread implements Runnable {

    public static class JobDoer {

        public synchronized void syncMethod() throws InterruptedException {
            for (int i = 0; i < 100000; i++) {
                System.out.println(Thread.currentThread().getName() + " : " + counter++);
            }
        }
    }

    private static int counter = 1;

    public MyThread(JobDoer doer) {
        this.doer = doer;
    }

    private JobDoer doer;

    @Override
    public void run() {
        try {
            doer.syncMethod();
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
    }

    public static void main(String[] args) throws InterruptedException {
        JobDoer doer = new JobDoer();
        Thread t1 = new Thread(new MyThread(doer), "Thread 1");
        Thread t2 = new Thread(new MyThread(doer), "Thread 2");
        Thread t3 = new Thread(new MyThread(doer), "Thread 3");
        Thread t4 = new Thread(new MyThread(doer), "Thread 4");

        t2.start();
        t1.start();
        t3.start();
        t4.start();
        t1.join();
        t2.join();
        t3.join();
        t4.join();
    }
}