Dining Philosopher - Last thread not terminating properly

Question

I wrote this Dining Philosopher code but the last thread does not produce the desired "xxx has finished his dinner" line? What did I do wrong?

It seems the last thread terminates prematurely.

I would appreciate any help on this.

import java.util.Random;



public class DiningPhilosophers {



    //An array holding all the chopsticks
    private final Chopstick[] chopsticks = new Chopstick[5];

    /*Constructor for the main class
    * Creates all the chopsticks 
    * Creates and starts all the threads*/
    public DiningPhilosophers(){
        putChopsticksOnTheTable();
        Thread t1 = new Thread(new Philosopher("First",this.chopsticks[4],this.chopsticks[0]));
        Thread t2 = new Thread(new Philosopher("Second",this.chopsticks[0],this.chopsticks[1]));
        Thread t3 = new Thread(new Philosopher("Third",this.chopsticks[1],this.chopsticks[2]));
        Thread t4 = new Thread(new Philosopher("Fourth",this.chopsticks[2],this.chopsticks[3]));
        Thread t5 = new Thread(new Philosopher("Fifth",this.chopsticks[3],this.chopsticks[4]));
        t1.start();
        t2.start();
        t3.start();
        t4.start();
        t5.start();


    }

    /*Initialise the chopsticks in the array*/
    private void putChopsticksOnTheTable(){
        for(int i = 0;i < chopsticks.length;i++)
        chopsticks[i]= new Chopstick(); 
    }

    public static void main(String[] args){
        new DiningPhilosophers();
    }
}


class Philosopher extends Thread{
private static final int EATING_TIME_LIMIT = 1000;
private static final int THINKING_TIME_LIMIT = 800;
private int EAT_TIMES = 5;
private final Random randomise = new Random();
private final Chopstick _leftChopstick;
private final Chopstick _rightChopstick;
private final String _name;
private State _state;

/* Enumeration class that holds 
* information about the possible 
* Philosopher's states 
*/
public enum State {
    EATING, THINKING
}

/*
* Main constructor for the Philosopher class
* @param name   the name of the Philosopher
* @param leftChopstick  the chopstick that is currently on the left of the Philosopher
* @param rightChopstick the chopstick currently on the right of the Philosopher
* 
*/
public Philosopher(String name, Chopstick leftChopstick, Chopstick rightChopstick) {

    this._leftChopstick = leftChopstick;
    this._rightChopstick = rightChopstick;
    this._name = name;

}

/*
* The method eat that uses two chopsticks. It blockes the two Chopstick
* objects so they could not be changed then it changes their state 
* as well as the state of the philosopher
* At the end of the method, the chopsticks' state is reverted and
* the Philosopher goes into the Thinking state 
*/
private void tryToEat() throws InterruptedException 
{       

     synchronized(_leftChopstick){
            while(_leftChopstick.inUse() || _rightChopstick.inUse())

                try{
                    //this.setPhilosopherState(Philosopher.State.WAITING);
                    _leftChopstick.wait();
                }catch (InterruptedException e){}
                    synchronized(_rightChopstick) {
                    try{
                        Thread.sleep(1);
                        _leftChopstick.pickUp();
                        System.out.println(_name + " picks up the left chopstick...");
                        _rightChopstick.pickUp();
                        System.out.println(_name + " picks up the right chopstick...");
                        eat();
                    }
                    finally {
                        _leftChopstick.putDown();
                        System.out.println(_name + " puts down the left chopstick...");
                        _rightChopstick.putDown(); 
                        System.out.println(_name + " puts down the right chopstick...");
                        //_leftChopstick.notify();
                        //_rightChopstick.notify();   
                    }
                    }
               }


    if(this.EAT_TIMES > 0)
            think();  

}

private void eat() throws InterruptedException
{
    setPhilosopherState(State.EATING);
    Thread.sleep(randomise.nextInt(EATING_TIME_LIMIT));        
    this.EAT_TIMES--;
    if(this.EAT_TIMES == 0)
        System.out.println("***************************" + _name + " has finished his dinner");
}

/*
* This method only changes the state 
* of the Philosopher to Thinking
*/
private void think() throws InterruptedException{
    setPhilosopherState(Philosopher.State.THINKING);
    Thread.sleep(randomise.nextInt(THINKING_TIME_LIMIT));
}

/*
* Set the current state of the Philosopher
*/
private void setPhilosopherState(State state){
    this._state = state;        

    if(_state == State.EATING)
        System.out.println ("*** " + _name + " is EATING for the " + (6 - EAT_TIMES) + " time!");
    else
        System.out.println( _name + " is THINKING...");
}

/*
* Get the current state of the Philosopher
*/
public State getPhilosopherState(){
    return _state;
}

/*
* The method is invoked with the start of the thread
* and runs the eat function for 10 times
*/
public void run(){
    while(this.EAT_TIMES > 0){
        try {
            tryToEat();
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
    }


}    

}


class Chopstick
{
   private boolean _inUse;

    /*
    * @return the current state of the chopstick
    */
    public boolean inUse(){
        return _inUse; 
    }

    /*
    * @param usedFlag the new state of the chopstick
    */
    public synchronized void pickUp()
    {           
        _inUse = true;
    }

    public void putDown()
    {
        _inUse = false;
        this.notify();
    }
}

Answer 1

This is a nice lesson on why you should synchronize access to all shared mutable data. Your Chopstick has a field that is not volatile but is accessed by multiple threads via inUse. The only spot where inUse is being referenced without synchronization is at

while (_leftChopstick.inUse() || _rightChopstick.inUse())

Without synchronization rightChopstick.inUse() can return values that are unexpected via data race. As a result all threads that are hanging are stuck at

_leftChopstick.wait();

If you synchronize inUse you should have it finishing correctly. Also I would highly recommend synchronizing on all the methods in Chopstick

Answer 2

In the Chopstick class you also need to synchronize the pickUp() method, otherwise the value may not be visible to other threads:

    public synchronized void pickUp() {
        _inUse = true;
    }

Or simply use AtomicBoolean.

Answer 3

It isn't necessary to call

Thread t1 = new Thread(new Philosopher(...));

You can instead call

Thread t1 = new Philosopher(...);

To ensure that all threads complete their work, call their .join methods. This will wait for the threads to die before exiting the DiningPhilosophers constructor.

t1.start();
....
t5.start();
t1.join();
...
t5.join();