Java MultiThreading using wait and notify - follow-up

Question

After following the tips from @VoiceOfUnreason on my previous question, I want to post the revised code.

The purpose of the code is to simulate a Shop and a Customer that use the Box service of the Shop while have a Paypal account that he must deposit money into before being able to use that Box service.

Main.Java

package exe5;

import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;

public class Main {

    public static void main(String[] args) throws Exception {
        Shop postOffice = new Shop(new Box(),new Paypal(), "Post Office");
        Customer daniel = new Customer(postOffice, "Daniel Netzer");

        ExecutorService executor = Executors.newCachedThreadPool();
        executor.submit(postOffice);
        executor.submit(daniel);

        Thread mainThread = Thread.currentThread();
        mainThread.sleep(5);

        // interrupting all executor threads and closing them safely.
        executor.shutdownNow();

        // Console output as log for safe closure of threads.
        mainThread.sleep(100);
        daniel.printsCustomer(4);
        postOffice.printsShop(5);
        System.out.println(postOffice.getPaypal().getStatus());
    }

}

Changes done on Main are the executor added to easily control the Thread pool.

Paypal.Java

package exe5;

public class Paypal {

    private int status;

    public Paypal(){
        this.setStatus(0);
    }

    public int getStatus() {
        return status;
    }

    public void setStatus(int status) {
        this.status = status;
    }

}

Box.Java

package exe5;

public class Box {

    private boolean boxStatus;

    public Box(){
        this.boxStatus = false;
    }

    public boolean isBoxStatus() {
        return boxStatus;
    }

    public void setBoxStatus(boolean boxStatus) {
        this.boxStatus = boxStatus;
    }
}

Shop.Java

package exe5;

public class Shop implements Runnable{

    private Box box;
    private Paypal paypal;
    private String name;

    public Shop(Box box, Paypal paypal, String name){
        this.box = box;
        this.setPaypal(paypal);
        this.name = name;
    }

    public String getName() {
        return name;
    }

    public synchronized void setName(String name) {
        this.name = name;
    }

    public Paypal getPaypal() {
        return paypal;
    }

    public void setPaypal(Paypal paypal) {
        this.paypal = paypal;
    }

    public Box getBox() {
        return box;
    }

    public synchronized void setBox(Box box) {
        this.box = box;
    }

    public synchronized void  depositBox(){
        this.box.setBoxStatus(true);
        printsShop(1);
        synchronized(this.box){
            this.box.notifyAll();}
    }

    public synchronized void withdrawPaypal(int withdrawAmount){
        this.paypal.setStatus(this.paypal.getStatus()-withdrawAmount);
        printsShop(4);
        synchronized (this.paypal){
            this.paypal.notifyAll();
        }
    }

    public void  printsShop(int caseNum){
        switch(caseNum){
        case 1: System.out.println(this.getName() +" deposited new package in box.");
                break;
        case 2: System.out.println(this.getName() +" box is full, waiting for customer withdrawal.");
                break;
        case 3: System.out.println("Insufficient funds in paypal account, waiting for customer deposit.");
                break;
        case 4: System.out.println(this.getName() +" withdrawad money from paypal accout.");
                break;
        case 5: System.out.println("Closed safely " +this.getName() +" thread.");
                break;
        }
    }

    @Override
    public void run() {
        while(!Thread.currentThread().isInterrupted()){
            while((this.paypal.getStatus() < 100)){
                synchronized(this.paypal){
                    try{
                        printsShop(3);
                        this.paypal.wait();
                    }catch (InterruptedException e) {
                        Thread.currentThread().interrupt();
                        break;
                    }
                }
                while(this.box.isBoxStatus()){
                    synchronized(this.box){
                        try {
                            printsShop(2);
                            this.box.wait();
                        } catch (InterruptedException e) {
                            Thread.currentThread().interrupt();
                            break;
                        }
                    }
                }
            }
            if ((this.paypal.getStatus() >= 100)){
                withdrawPaypal(100);
                depositBox();
            }
        }
    }
}

Customer.Java

package exe5;

public class Customer implements Runnable {

    private String name;
    private Shop shop;
    private Box box;
    private Paypal paypal;

    public Customer(Shop shop, String name){
        this.setShop(shop);
        this.setName(name);
        this.box = this.shop.getBox();
        this.setPaypal(this.shop.getPaypal());
    }

    private void setShop(Shop shop) {
        this.shop = shop;       
    }

    public String getName() {
        return name;
    }

    public synchronized void setName(String name) {
        this.name = name;
    }

    public Paypal getPaypal() {
        return paypal;
    }

    public void setPaypal(Paypal paypal) {
        this.paypal = paypal;
    }

    public synchronized void  withdrawBox(){
        this.box.setBoxStatus(false);
        printsCustomer(1);
        synchronized(this.shop.getBox()){
            this.shop.getBox().notifyAll();}
    }

    public synchronized void depositPaypal(int depositAmount){
        this.paypal.setStatus(this.paypal.getStatus()+ depositAmount);
        printsCustomer(3);
        synchronized(this.paypal){
            this.paypal.notifyAll();
        }
    }

    public void  printsCustomer(int caseNum){
        switch(caseNum){
        case 1: System.out.println(this.getName() +" withdrawad package from box at " +this.shop.getName());
                break;
        case 2: System.out.println(this.getName() +" box is empty, waiting for a new package to arrive.");
                break;
        case 3: System.out.println(this.getName() +" deposited money into " +this.shop.getName() +" paypal account.");
                break;
        case 4: System.out.println("Closed safely " +this.getName() +" thread.");
                break;
        }
    }

    @Override
    public void run() {
        while(!Thread.currentThread().isInterrupted()){
                while(!this.box.isBoxStatus()){
                    synchronized(this.shop.getBox()){
                    try {
                        printsCustomer(2);
                        depositPaypal(100);
                        this.shop.getBox().wait();

                    } catch (InterruptedException e) {
                        Thread.currentThread().interrupt();
                        break;
                    }
                }
            }
            withdrawBox();
        }
    }
}

I removed all static variables and made them private in all codes. I removed the 2 Monitor objects and used the shared PayPal account and Box.

The program is working just fine, but I still feel that the main issue I was trying to handle here is that the wait/notify still lacks finesse.

Answer 1

public synchronized void depositPaypal(int depositAmount){
    this.paypal.setStatus(this.paypal.getStatus()+ depositAmount);
    printsCustomer(3);
    synchronized(this.paypal){
        this.paypal.notifyAll();
    }
}

This is broken. As janos noted, this.paypal isn't a final field; by the time you get the lock on the paypal reference, some other thread could have changed the value of the reference, and you would end up notifying on a different object than you synchronized.

Presumably, you want to do all of the work on the same object...

public synchronized void depositPaypal(int depositAmount){
    // Somebody can change the paypal property after this point
    // but the reference local to the function will be unchanged.

    final Paypal target = this.paypal;
    target.setStatus(target.getStatus()+ depositAmount);
    printsCustomer(3);
    synchronized(target){
        target.notifyAll();
    }
}

Problem #2, of course, is that you are making a change to shared state when you don't have exclusive write access to that change. The modification of the object needs to happen in the synchronization block.

public synchronized void depositPaypal(int depositAmount){
    // Somebody can change the paypal property after this point
    // but the reference local to the function will be unchanged.

    final Paypal target = this.paypal;
    synchronized(target){
        target.setStatus(target.getStatus()+ depositAmount);
        // this status message could potentially be moved outside the
        // synchronization block
        printsCustomer(3);
        target.notifyAll();
    }
}

Now, that logic implies that -- somewhere in the code, there is going to be a thread waiting on a Paypal; the fact that only the status changed is a hint that this other thread is going to be watching for a change in status -- but it might be watching something that depends on the change. We don't know what the function is, or what it does, but we can know that it should have the following shape.

    final Paypal target = ...; //
    synchronized(target) {
        // we have the lock!
        while (! canMakeProgress(target)) {
            // but we can't make progress now
            // yield the lock
            target.wait();
        }
        // we have the lock
        // AND we can make progress!

        // do the work
    }

When I look for that shape, the closest match I see is

public class Shop {

@Override
public void run() {
    while(!Thread.currentThread().isInterrupted()){

        while((this.paypal.getStatus() < 100)){
            synchronized(this.paypal){
                try{
                    printsShop(3);
                    this.paypal.wait();
                }catch (InterruptedException e) {
                    Thread.currentThread().interrupt();
                    break;
                }
            }
            while(this.box.isBoxStatus()){
                synchronized(this.box){
                    try {
                        printsShop(2);
                        this.box.wait();
                    } catch (InterruptedException e) {
                        Thread.currentThread().interrupt();
                        break;
                    }
                }
            }
        }
        if ((this.paypal.getStatus() >= 100)){
            withdrawPaypal(100);
            depositBox();
        }
    }
}

Broken broken broken

• The checks for evaluating whether you can make progress are being made before you have the lock, rather than inside it
• The work you are doing is occurring after you have released the locks

Here's another way of saying the same thing - the idiom below...

final Object lock = ... //
synchronized(lock) {
    lock.wait();
}

what it is really doing is something like

1. Block waiting to acquire the lock
2. Acquire the lock
3. Enter the synchronized block
4. Release the lock
5. Wait to be signalled
6. Block waiting to acquire the lock
7. Acquire the lock
8. Leave the synchronized block
9. Release the lock

That's not nothing (the Java Memory Model guarantees that you can see writes that "happened before" the signal at step 5); but it isn't actually protecting the reads and writes in your logic.

Compare what you've written with this example

@Override
public void run() {
    while(!Thread.currentThread().isInterrupted()){
        // NOTE: when holding multiple locks, it's super
        // important that the locks are always acquired in
        // the same order.

        final Paypal paypal = this.paypal;
        final Box box = this.box;

        try {
            synchronized(paypal) {
                // spin until we can make progress
                while(paypal.getStatus() < 100) {
                    paypal.wait();
                }
                synchronized(box) {
                    while(box.isBoxStatus()) {
                        box.wait();
                    }
                    depositBox();
                    withdrawPaypal(100);
                }
            }
        } catch(InterruptedException e) {
            Thread.currentThread().interrupt();
        }
    }
}