Why synchronized block is giving wrong answer?

Question

I am trying to learn synchronization. Got stuck here according to what I have learned the following code should give 8000 as the final result but I am getting a random result like below
package threads;

import java.time.LocalDateTime;

public class A implements Runnable {
    String name;
    static Integer j=0;
    A(String name){
        this.name=name;
    }
    @Override
    public synchronized void  run() {
        for(int i=1;i<=1000;i++){
            synchronized(this){
            A.j++;
            }
        }
        System.out.println(j);
    }

package threads;

public class MainClass {
public static void main(String args[]){
    Thread t1=new Thread(new A("i am thread A "));
    Thread t2=new Thread(new A("i am thread B "));
    Thread t3=new Thread(new A("i am thread C "));
    Thread t4=new Thread(new A("i am thread D "));
    Thread t5=new Thread(new A("i am thread E "));
    Thread t6=new Thread(new A("i am thread F "));
    Thread t7=new Thread(new A("i am thread G "));
    Thread t8=new Thread(new A("i am thread H "));
    t1.setPriority(Thread.MAX_PRIORITY);
    t8.setPriority(Thread.MIN_PRIORITY);
    t1.start();
    t2.start();
    t3.start();
    t4.start();
    t5.start();
    t6.start();
    t7.start();
    t8.start();
    try {
        t1.join();
        t2.join();
        t3.join();
        t4.join();
        t5.join();
        t6.join();
        t7.join();
        t8.join();
    } catch (InterruptedException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }


}
}

still getting output like 1293 2214 1403 3214 4214 5214 6224 7037 can anyone explain to me how to achieve synchronization and what is going wrong here?

Answer 1

It is a common mistake to think that synchronized means "critical section", and that no other threads will run while a synchronized block is running. But synchronized blocks are only exclusive with respect to other synchronized blocks that lock on the same lock.

The answers you got ("use a common lock") are right, but didn't really tell you why. The other common mistake is to think about synchronized as protecting code, when really you should be thinking about it protecting data. Any shared mutable data should be guarded by one and only one lock, and you should know exactly what that lock is. (The more complex your locking scheme, the less likely you'll know what locks guard what data.) So you should always be thinking in terms of "data X is guarded by lock L", and then make sure you acquire lock L whenever you access (read or write) that data.

Answer 2

There are a couple of issues in the code.

• Issue 1: Lock object added in synchronized(..) is not shared among all thread instances

• Issue 2: System.out.println(j); line should be in the end after t8.join(); otherwise, you will be given 8 times output.

The rectified code

public class A implements Runnable {

    String name;
    static Integer j = 0;

    static Object lockObject = new Object();

    A(String name) {
        this.name = name;
    }

    @Override
    public void run() {
        for (int i = 1; i <= 1000; i++) {
            synchronized (lockObject) {
                A.j++;
            }
        }
    }


    public static void main(String args[]) {
        Thread t1 = new Thread(new A("i am thread A "));
        Thread t2 = new Thread(new A("i am thread B "));
        Thread t3 = new Thread(new A("i am thread C "));
        Thread t4 = new Thread(new A("i am thread D "));
        Thread t5 = new Thread(new A("i am thread E "));
        Thread t6 = new Thread(new A("i am thread F "));
        Thread t7 = new Thread(new A("i am thread G "));
        Thread t8 = new Thread(new A("i am thread H "));
        t1.setPriority(Thread.MAX_PRIORITY);
        t8.setPriority(Thread.MIN_PRIORITY);
        t1.start();
        t2.start();
        t3.start();
        t4.start();
        t5.start();
        t6.start();
        t7.start();
        t8.start();
        try {
            t1.join();
            t2.join();
            t3.join();
            t4.join();
            t5.join();
            t6.join();
            t7.join();
            t8.join();
        } catch (InterruptedException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }
        System.out.println(A.j);

    }
}

Answer 3

This will solve the issue. You have to synchronize using a shared lock to all the threads since you are incrementing a static field. Otherwise each object will have it's own lock and increment the static field in parallel leading to a race condition. That's why you are not getting correct value which is 8000 in this case.

package bookmarks;

public class A implements Runnable {
    String name;
    static Integer j = 0;
    private static Object lock = new Object();

    A(String name) {
        this.name = name;
    }

    @Override
    public void run() {
        for (int i = 1; i <= 1000; i++) {
            synchronized (lock) {
                A.j++;
            }
        }
        System.out.println(j);

    }

}