Producer consumer water cooler example 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&lt;Integer&gt; mySharedList;
    int size;

    public Producer(List&lt;Integer&gt; 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&lt;=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&lt;Integer&gt; mySharedList;

    public Consumer(List&lt;Integer&gt; 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&lt;Integer&gt; list=new ArrayList&lt;Integer&gt;();

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

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


You properly synchronize on the sharedList.
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();

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)
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())

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

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


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.
Creating a thread, and starting it, gives you little control over it. (how will you stop them?)