Java: Multiple threads decrementing static variable via synchronized method accessing variable at the same time

Question

I'm trying to synchronize my Person class Methods so that my static counter variable gets decremented by one thread at the a time.

public class Person extends Thread {

    private static int count = 10;

    public void decrement() {
        synchronized(Person.class) {
            count--;
        }

    }

    public int getCount() {
        return count;
    }

    public void run(){
        while( count > 0){
            this.decrement();
            System.out.print(this.getCount() + ",");
        }
    }
}

Here's my main class. Each thread will decrement to static counter via synchronized method to avoid mutiple thread access to same resource.

public class Main {

    /**
     * @param args the command line arguments
     */
    public static void main(String[] args) {

        Person p1 = new Person();
        Person p2 = new Person();
        Person p3 = new Person();
        Person p4 = new Person();
        Person p5 = new Person();

        p1.start();
        p2.start();
        p3.start();
        p4.start();
        p5.start();
    }
}

But when i run my program, it is printing duplicate counter values. What am i doing wrong?

Outputs:

8,8,7,6,5,4,3,2,1,0

8,7,5,3,1,0,6,8,4,0

Answer 1

What's going on in the original code is the following:

1. Thread one decrements count (count -> 9)
2. Thread two decrements count (count -> 8)
3. Thread three decrements count (count -> 7)
4. Thread four decrements count (count -> 6)
5. Thread one outputs count (count: 6)
6. Thread two outputs count (count: 6)
7. Thread three outputs count (count: 6)
8. Thread four outputs count (count: 6)

Since you are locking the decrement and not the decrement and output together, it's appearing to decrement multiple times.

In other words, there is no guarantee that this code will execute back-to-back:

        this.decrement();
        System.out.print(this.getCount() + ",");

Here's the fixed code. It returns the current count value on decrement so that the new value can be returned and printed.

public class Person extends Thread {

    private static int count = 10;

    public int decrement() {
        synchronized(Person.class) {
            count = count - 1;
            return count;
        }

    }

    public int getCount() {
        synchronized(Person.class) {
            return count;
        }
    }

    public void run(){
        while( getCount() > 0){
            int count = this.decrement();
            System.out.println(count);
        }
    }
}

I'd recommend AtomicInteger though for this task:

import java.util.concurrent.atomic.AtomicInteger;

public class Person extends Thread {

    private static AtomicInteger count = new AtomicInteger(10);

    public int decrement() {
        return count.decrementAndGet();
    }

    public void run(){
        while(count.get() > 0){
            int currentCount = this.decrement();
            System.out.print(currentCount + ",");
        }
    }
}

Answer 2

It's not enough to synchronize only writes, you have to synchronize the getter as well (otherwise the reader thread may read a stale value). But in this case the problem is that other threads can interleave executing between the time a thread decrements and the time that same thread retrieves a value.

Use a java.util.concurrent.atomic.AtomicInteger to store the count. But if you keep separate methods for the decrementing and the getting (locking the decrement and lock the getter separately), still there's nothing to guarantee that threads don't interleave in a way that causes duplicates to get written out. Using AtomicInteger's decrementAndGet method makes sure that the value that's decremented is the one that's returned.