CODEWITHSUNDEEP
javamultithreadingsynchronizationthread-safety
Fakeer
Fakeer

Reputation: 185

Thread output inconsistent even when using synchronized keyword

I am very new to threads. I wrote a code and expected my output as 20000 consistently. But that's not the case. Please find the code below:

class Runner4 implements Runnable {

    static int count = 0;

    public synchronized void increase() {
        count++;
    }

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

public class threading4 {

    public static void main(String[] args) {

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

        System.out.println(Runner4.count);
    }
}

Any explanation?

Thanks!!

Upvotes: 0

Views: 484

Answers (3)

Kakalokia
Kakalokia

Reputation: 3191

The way you're trying to achieve what you want is is not really the best way. A better way to do it is define a class called Counter, as the following:

public class Counter 
{
    int count;
    public Counter()
    {
        count = 0;
    }
    public synchronized void increase() {
        count++;
    }

    public int getCount()
    {
        return count;
    }

}

The class has the methods of increasing the counter and getting it. What you need to do now is have a Counter object to be shared by two threads that call the increase() method. So your thread class would look like this:

class Runner4 extends Thread {

    Counter count;
    public Runner4(Counter c)
    {
        count = c;
    }


    @Override
    public void run() {
        for (int i = 0; i < 10000; i++) {
            count.increase();
        }

    }
}

Notice that the class takes a Counter object and calls the increase method. Also the class extends Thread instead of implementing Runnable. There is really no much difference, it's just now your Runner4 can use Thread class methods.

From your main defines a Counter object and two Runner4 threads, and then pass the Counter object to each one of them:

public static void main(String[] args) {

        Counter count = new Counter();
        Thread t1 = new Runner4(count);
        t1.start();
        Thread t2 = new Runner4(count);
        t2.start();
        try {
            t1.join();
            t2.join();
        } catch (InterruptedException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }

        System.out.println(count.getCount());
    }

Upvotes: 0

Jeffrey
Jeffrey

Reputation: 44808

Your code would work if count was not static.

public synchronized void increase() {
    // method body
}

is equivalent to

public void increase() {
    synchronized(this) {
        // method body
    }
}

Since count is static, both t1 and t2 are accessing it with different locks, resulting in non-deterministic behavior. Either make Runner4.increase synchronize on a common lock (Runner4.class or a private static lock object would work just fine), or make count non-static.

Upvotes: 0

Perception
Perception

Reputation: 80603

You are synchronizing on two different objects in your code (corresponding to the two objects you created). As such, there is no protection of the shared static variable, and you get unpredictable results. Basically, there is no effective synchronization going on in your program. You can fix this with a simple modification.

Change:

public synchronized void increase(){
    count++;
}

To:

public void increase(){
    synchronized(Runner4.class) {
        count++;
    }
}

Note that I am not saying this is the best way to accomplish this kind of synchronization - but the important take-away is that, if you are modifying a class level variable, you need class level synchronization as well.

Upvotes: 1

Related Questions