CODEWITHSUNDEEP
javamultithreadingsynchronized
user2438582
user2438582

Reputation: 13

Method Synchronizing Threading Issue

I can't Seem to get a final counter value Of 20000. What is wrong with this code?

public class Synchronize2 {

    public static void main(String[] args) {
        Threading t1 = new Threading();
        Threading t2 = new Threading();

        t1.start();
        t2.start();

        try {
            t1.join();
            t2.join();
        } catch (InterruptedException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }

        System.out.println(Threading.counter);
    }
}


class Threading extends Thread {

    static int counter;

    public synchronized void incrementer() {
        counter++;
    }

    public void run() {
        for (int i=0; i<10000; i++) {
            incrementer();
        }
    }
}

Upvotes: 1

Views: 62

Answers (2)

Matthias
Matthias

Reputation: 837

You synchronize on two different Objects. Your incrementer is a short form of this:

    public void incrementer() {
        synchronized (this) {
            counter++;
        }
    }

But the two instances of "this" are not the same Object. Thus, you do not synchronize at all. Try it this way:

    private static Object sync = new Object();

    public void incrementer() {
        synchronized (sync) {
            counter++;
        }
    }

You should also make the variable counter volatile. It is not strictly neccessary here, because you use it only in synchronized blocks. But in real code you might read it outside such a block, and then you will get problems. Non volatile variables can be read from a local thread cache, instead from the memory.

Upvotes: 0

rgettman
rgettman

Reputation: 178243

Your synchronized incrementer method will lock on the object itself. But you have 2 different objects, each locking on themselves, so the method isn't thread safe; both threads can still access incrementer at the same time.

Additionally, the post-increment operation isn't thread safe because it's not atomic; there is a read operation and an increment operation, and a thread can be interrupted in the middle of the two operations. This non-thread-safe code presents a race condition, where thread one reads the value, thread two reads the value, then thread one increments and thread two increments, yet only the last increment "wins" and one increment is lost. This shows up when the ending value is less than 20000.

Make the method static too, so that because it's synchronized, it will lock on the class object of the class, which is proper synchronization here.

public static synchronized void incrementer() {

Upvotes: 3

Related Questions