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();
    }
}

score 5 · Accepted Answer · answered Jul 02 '18 at 05:39

5

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()

answered Jul 02 '18 at 05:39

Erwin Bolwidt

30,799
15
56
79

Thanks, your solution worked, also I found that if I pass same Runnable instance ex. r1 to all 4 Thread instances (new Thread(r1,"Thread 4")) then also my code works without declaring my method static – LeoScott Jul 02 '18 at 06:01
@Akshay Yes it will work without making method **static** if you pass the same instance. However in this case it is even not necessary to make the variable static. You can remove it and that too should work as you have synchronized the method. – Neerav Vadodaria Jul 04 '18 at 15:01

score 1 · Answer 2 · answered Jul 02 '18 at 05:55

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();

score 0 · Answer 3 · answered Jul 02 '18 at 06:06

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();
    }
}

Why my synchronized method not working properly?

3 Answers3