Reentrant lock not updating the shared resource properly

Question

I have a shared variable count, which I am incrementing in increment() method and two threads are access this. I'm getting the wrong final count. Here is the snippet of code:

public class ReentrantLockTest {
    static volatile int count = 0;

    public static void main(String[] args) throws InterruptedException {

        Thread t1 = new Thread(new Runnable() {
            @Override
            public void run() {
                for(int i = 0; i < 10000; i++){
                    increment();
                }
            }
        });


        Thread t2 = new Thread(new Runnable() {
            @Override
            public void run() {
                for(int i = 0; i < 10000; i++){
                    increment();
                }
            }
        });

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

        t1.join();
        t2.join();
        System.out.println("Counter variable : "+count);

    }
    private static void increment(){
        ReentrantLock lock = new ReentrantLock();
        lock.lock();
        try {
            count++;
        }finally {
           lock.unlock();
        }


    }
}

The counter variable coming as 19957/19678..ect. It should come as 20000. If I am using synchronized then I am getting 20000. Please help me on this.

Answer 1

You are creating a new ReentrantLock every time increment() is invoked. In order for state to be properly guarded, you need to synchronize/lock on the same object whenever accessing that particular state. You need to change your code so that the same lock instance is used every time.

import java.util.concurrent.locks.ReentrantLock;

public class ReentrantLockTest {

  private static final ReentrantLock LOCK = new ReentrantLock();
  private static int count;

  public static void increment() {
    LOCK.lock();
    try {
      count++;
    } finally {
      LOCK.unlock();
    }
  }

  public static void main(String[] args) throws InterruptedException {
    Runnable action = () -> {
      for (int i = 0; i < 10_000; i++) {
        increment();
      }
    };

    Thread t1 = new Thread(action);
    Thread t2 = new Thread(action);
 
    t1.start();
    t2.start();

    t1.join();
    t2.join();
    System.out.println("Counter variable : " + count);
  }
}

Also, note that since count is guarded by a lock, and because t1.join() and t2.join() are called before reading the value of count to print it out, it is actually unnecessary for count to be volatile in this case.