Producer consumer water cooler example

Question

I have written a water cooler example in which the consumer drinks from the water cooler until it's empty and when it gets empty the producer fills it up. The consumers wait until it's filled up and then start drinking again. Can you please review?

Producer

package com.par.consumer.producer;

import java.util.List;

public class Producer implements Runnable {
    private List<Integer> mySharedList;
    int size;

    public Producer(List<Integer> sharedList,int size) {
        this.mySharedList=sharedList;
        this.size=size;
    }

    @Override
    public void run() {
        while(true)
        {
            try {
                Thread.sleep(1000);

            } catch (InterruptedException e1) {
                e1.printStackTrace();
            }

            synchronized (mySharedList) {
                System.out.println("Size is "+mySharedList.size());

                if(mySharedList.isEmpty()) // Water cooler is empty fill it up 
                {
                    for(int i=1;i<=size;i++){
                        System.out.println("Adding water "+i);
                        mySharedList.add(i);
                    }
                    mySharedList.notify();
                }

                try {
                    System.out.println("Producer waiting");
                    mySharedList.wait();
                    System.out.println("Producer waiting exit");
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
            }
        }
    }
}

Consumer

package com.par.consumer.producer;

import java.util.List;

public class Consumer implements Runnable{
    private List<Integer> mySharedList;

    public Consumer(List<Integer> sharedList) {
        this.mySharedList=sharedList;
    }
    @Override
    public void run() {
        while(true)
        {
            try {
                Thread.sleep(1000);
            } catch (InterruptedException e1) {
                e1.printStackTrace();
            }
            synchronized (mySharedList) {
                if(mySharedList.isEmpty())
                {
                    System.out.println("IS EMPTY");

                    try {
                        mySharedList.notify();
                        System.out.println("Consumer waiting");
                        mySharedList.wait();
                        System.out.println("Consumer waiting exit");
                    } catch (InterruptedException e) {
                        // TODO Auto-generated catch block
                        e.printStackTrace();
                    }
                }

                System.out.println("Drinking "+mySharedList.remove(0));
            }
        }
    }
}

WaterCooler (Driver)

package com.par.consumer.producer;

import java.util.ArrayList;
import java.util.List;

public class WaterCooler {
    volatile List<Integer> list=new ArrayList<Integer>();

    public static void main(String[] args) {
        new WaterCooler().run();
    }

    void run()
    {
        new Thread(new Producer(list, 4)).start();
        new Thread(new Consumer(list)).start();
    }
}

Answer 1

Producer and Consumer

1. You properly synchronize on the sharedList.

2. Both Producer and Consumer fail to use the proper wait() idiom to guard against spurious wake up.

   Producer waits until the list is empty :

   while(!sharedList.isEmpty())
       sharedList.wait();
   

   Consumer waits until the list is not empty :

   while(sharedList.isEmpty())
       sharedList.wait();
   

3. Given that both wait on the same monitor with a different condition you must use notifyAll() instead of notify() :

   Single notify can be used instead of notifyAllonly when both of the following conditions hold:

   • Uniform waiters. Only one condition predicate is associated with the condition queue, and each thread executes the same logic upon re-turning from wait
   • One-in, one-out. A notification on the condition variable enables at most one thread to proceed.

   (from Java Concurrency in Practice)

4. You set the threads in motion, but you have no way of stopping them. Consider using interruption for this.

   Check the interrupted flag in your loop condition :

   while (!Thread.currentThread().isInterrupted())
   

5. Properly deal with InterruptedException by restoring the interrupted state so the interruption is not lost.

   Call Thread.interrupt() in the catch block :

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

6. I assume this is some sort of exercise. Typically to connect producers and consumers you would use a blocking bounded queue.

Latstly : while println() gives you some nice output, it can give you a false sense of thread safety, as it does synchronisation under the hood, and in doing so introduces memory boundaries that may hide problems with your code, that will surface once you substitute them with actual production code.

WaterCooler

1. There is no reason to make list volatile. It is never reassigned. In fact it would be better to mark it final, to make sure this never happens.
2. Creating a thread, and starting it, gives you little control over it. (how will you stop them?)

Answer 2

It seems a little strange to have a size member in Producer that doesn't correspond to the number of producers running. It looks like you can either make the water cooler size hard-coded or have it provided by the user. You can then set the initial List size equal to this size and then use a foreach loop instead of an indexed loop in the run() method in Producer.

In addition, you're using some inconsistent spacing and lack some, such as the entire for loop statement in Producer. I almost didn't notice it at first since there was no spacing.

Here's what it should look like:

for (int i = 1; i <= size; i++) {

The same idea should be applied to other areas as well.