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?

change your code to `public void run() { synchronized(A.class) }` or better use `AtomicInteger` — Eugene, Mar 14 '18 at 14:34
`synchronized(this){..}` means that block is synchronized on *current* (this) instance of `A`, each Thread has *different* (new) instance so they are not synchronized on same lock so they are free to operate in the same time modifying same shared `j`. — Pshemo, Mar 14 '18 at 14:34
Your `A` instances are `synchronized` to themselves, not to each other... — Usagi Miyamoto, Mar 14 '18 at 14:34
I think `public synchronized void run() [...] synchronized(this)` is a bit redundant — phflack, Mar 14 '18 at 14:38
Create one lock object and share it among all A instances (you can pass it as constructor parameter and later use it like `synchronized(lock){...}`). Or don't reinvent the wheel and use `AtomicInteger` which does all required synchronization for you. — Pshemo, Mar 14 '18 at 14:38
Pshemo Thank you, got the idea and @Eugene's answer helped me. But can you explain how this works? — RaM PrabU, Mar 14 '18 at 14:39
"But can you explain how this works?" can you be more specific about which part is not clear? — Pshemo, Mar 14 '18 at 14:40
synchronized(A.class) actually it worked. But I wonder how it happened. — RaM PrabU, Mar 14 '18 at 14:43
Possible duplicate of [Static versus non-static lock object in synchronized block](https://stackoverflow.com/questions/18356795/static-versus-non-static-lock-object-in-synchronized-block) — payloc91, Mar 14 '18 at 15:22

score 12 · Answer 1 · answered Mar 14 '18 at 15:13

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.

Ravindra Ranwala · Accepted Answer · 2018-03-14T14:40:46.107

5

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

    }

}

edited Mar 14 '18 at 14:40

answered Mar 14 '18 at 14:38

Ravindra Ranwala

20,744
6
45
63

3

As a side note, there is no reason to use a boxed `Integer` here; declaring the variable as `static int j=0;` would do as well. – Holger Mar 14 '18 at 15:13
Yeah, prefer primitives over boxed primitives always. – Ravindra Ranwala Mar 14 '18 at 15:16

score 1 · Answer 3 · answered Mar 14 '18 at 14:40

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

    }
}

issue 2 8 times output is not a matter. But your answer was helpful. Thank you — RaM PrabU, Mar 14 '18 at 14:44

Why synchronized block is giving wrong answer?

3 Answers3