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 mySharedList;
    int size;

    public Producer(List 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 mySharedList;

public Consumer(List sharedList) {
this.mySharedList=sharedList;
}
@Override
public void run() {
while(true)
{
try {
Thread.sleep(1000);
} catch (InterruptedException e1) {
e1.printStackTrace();
}

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

StackExchange Code Review Q#88107, answer score: 5