HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Dining Philosophers

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
philosophersdiningstackoverflow

Problem

I've just finished my solution to the Dining Philosopher's Problem, but I am not confident with my code because I am still newbie to the concurrency world. I would appreciate it if you could leave me some feedback.

Here is my main class:

public class DiningPhilosophersTable {

    //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 DiningPhilosophersTable(){
        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 DiningPhilosophersTable();
    }
}


Here is the Philosopher class:

```
public class Philosopher extends Thread{

private static final int MAX_EATING_TIME = 1000;
private static final int MAX_THINKING_TIME = 800;
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, WAITI

Solution

Just a quick note:

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


Here you should synchronize on _rightChopstick too since isUsed could be called from other threads concurrently.


[...] synchronization has no effect unless both read and write operations are synchronized.

From Effective Java, 2nd Edition, Item 66: Synchronize access to shared mutable data.


Locking is not just about mutual exclusion; it is also about memory visibility.
To ensure that all threads see the most up-to-date values of shared mutable
variables, the reading and writing threads must synchronize on a common lock.

From Java Concurrency in Practice, 3.1.3. Locking and Visibility.

Another (and better) solution is using AtomicBooleans.

Code Snippets

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

Context

StackExchange Code Review Q#6348, answer score: 6

Revisions (0)

No revisions yet.