Thread Pools decreasing data to negative even when condition is to stop at zero

Question

Processor class -

public class Processor extends Thread {

    private static int stock = 10;

    private static Object lock1 = new Object();
    private static Object lock2 = new Object();
    private static Object lock3 = new Object();

    private void snooze(long millis) {
        try {
            Thread.sleep(millis);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
    }

    private boolean isStockEmpty() {
        boolean value;
        synchronized (lock1) {
            if (stock == 0) {
                value = true;
            } else {
                value = false;
            }
        }
        return value;
    }

    private void decreaseStock() {
        synchronized (lock2) {
            stock--;
        }
    }

    private int getStockCount() {
        int value;
        synchronized (lock3) {
            value = stock;
        }
        return value;
    }

    private  void doWork() {
        if (!isStockEmpty()) {
            decreaseStock();
            System.out.println(Thread.currentThread().getName() + " takes 1 item from stock\n" +
                    "Items remaining in stock: " + getStockCount());
            snooze(2000);
        } else {
            System.out.println("Stock is empty, " + Thread.currentThread().getName() + "is idle\n" +
                    "Items remaining in stock: " + getStockCount());
            snooze(2000);
        }
    }

    @Override
    public void run() {
        doWork();
    }
}

main method -

public class Main {
    public static void main(String[] args) throws InterruptedException {
        ExecutorService executorService = Executors.newFixedThreadPool(3);
        for (int i = 1; i <= 12; i++) {
            executorService.submit(new Processor());
        }
        executorService.shutdown();
        System.out.println("All tasks successfully submitted");
        executorService.awaitTermination(1, TimeUnit.DAYS);
        System.out.println("All tasks completed successfully");
    }
}

Here, I am creating a thread pool of three threads and assigning them a job to take 12 items from a stock of 10 with the condition that if stock gets empty, the threads will sit idle.

This program is not thread safe and stock count at the end becomes negative.

How to make this program thread safe?

Answer 1

How to make this program thread safe?

First identity the shared state among threads; one can identify four items, namely: stock, lock1, lock2, and lock3. Those fields are static, hence shared among threads. Now look for potential race conditions involving that shared state.

Are those fields only being read? if yes, then there is no race condition. Are those fields being modified in some way? yes, then you need to ensure mutual exclusion on the access to those fields.

The fields lock1, lock2, and lock3 are used as follows:

synchronized (lock1) { ... }
... 
synchronized (lock2) { ... }
...
synchronized (lock3) { ... }

therefore, no race condition there. How about field stock?! We have a read within the method isStockEmpty (i.e., stock == 0), a read and the write within the method decreaseStock (i.e., stock--;), and finally another read within the method getStockCount (i.e., value = stock;). So there is potential for multiple reads and writes happening in parallel by multiple threads, one has, therefore, to ensure mutual exclusion on that field. You have added the synchronization part, however, you need to use the same lock to ensure that threads will not read and write concurrently.

From the oracle tutorial one can read:

Every object has an intrinsic lock associated with it. By convention, a thread that needs exclusive and consistent access to an object's fields has to acquire the object's intrinsic lock before accessing them, and then release the intrinsic lock when it's done with them. A thread is said to own the intrinsic lock between the time it has acquired the lock and released the lock. As long as a thread owns an intrinsic lock, no other thread can acquire the same lock. The other thread will block when it attempts to acquire the lock.

So instead of synchronizing using three different objects to ensure mutual exclusion of the same data, let us just use one, so your code would look like the following:

public class Processor extends Thread {
  
    private static final Object stock_lock = new Object();

    @GuardedBy("lock")
    private static int stock = 10;

    private void snooze(long millis) { ...}

    private boolean isStockEmpty() {
       synchronized (stock_lock) {
           return stock == 0;
       } 
    }

    private void decreaseStock() {
        synchronized (stock_lock) {
            stock--;
        }
    }

    private int getStockCount() {
       synchronized (stock_lock) {
          return stock;
       }
   }

   ...
}

Is the program now thread-safe?!, almost but there is still a sneaky race condition there, namely:

if (!isStockEmpty()) {
    decreaseStock();

even though separately both methods isStockEmpty and decreaseStock are thread-safe, when they are called together there are not, why? because the entire operation of checking if the stock is empty and decrease it needs to be done sequentially. Otherwise, the following race condition can happen :

The field stock is 1, Thread 1 synchronizes in isStockEmpty check if is empty and !isStockEmpty() evaluates to true, Thread 1 moves on to call decreaseStock(), at the same time (before Thread 1 calls synchronized (stock_lock)) Thread 2 also calls !isStockEmpty() which will also evaluate to true. Thread 1 performs the operation stock--, making stock = 0, and because Thread 2 is already inside the block wrap by the if (!isStockEmpty()), Thread 2 will also perform stock--, making stock = -1. Moreover, you have a similar race condition with the getStockCount() called inside the doWork method as well. The solution is to synchronize the entire block of code, namely:

private void doWork() {
    synchronized (stock_lock) {
        ....
    }
}

Now, because isStockEmpty, decreaseStock, and getStockCount are all private methods that are called within synchronized (stock_lock) of the doWork method, we can actually remove from those methods the synchronization that we have added in the beginning. So the entire code would look like this:

public class Processor extends Thread {
    private static final Object stock_lock = new Object();
    
    @GuardedBy("lock")
    private static int stock = 10;
    
    
    private void snooze(long millis) {...}
    
    private boolean isStockEmpty() { return stock == 0; }
    
    private void decreaseStock() { stock--;}
    
    private int getStockCount() {  return stock;}
    
    private void doWork() {
        synchronized (stock_lock) {
            ....
        }
    }
    
    @Override
    public void run() {
        doWork();
    }
}

Now in a real-life example if you synchronize the entire program like that you might as well just execute the code sequentially.

Alternatively, you could make your current program thread-safe just by using AtomicInteger for the stock field variable:

private static AtomicInteger stock = new AtomicInteger(10);

private void snooze(long millis) {...  }
private void doWork() {
        int value = stock.getAndDecrement();
        if (value != 0) {
            System.out.println(Thread.currentThread().getName() + " takes 1 item from stock\n" +
                    "Items remaining in stock: " + value);
            snooze(2000);
        } else {
            System.out.println("Stock is empty, " + Thread.currentThread().getName() + "is idle\n" +
                    "Items remaining in stock: " + value);
            snooze(2000);
        }
}

There is no race condition because 1) getAndDecrement is done atomically; 2) we save the returning value into a local variable (which are private to threads), and use that value instead of getStockCount. Notwithstanding, the stock variable can in theory get negative values. But those values would not be displayed.