5
\$\begingroup\$

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();
    }
}
\$\endgroup\$
4
\$\begingroup\$

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?)
\$\endgroup\$
  • \$\begingroup\$ My response on the points: 1)Seems like your satisfied here. 2) Actually I wanted that the water cooler should be filled up once its empty, so does this seem fine in that case? 3) Agree with you on this it should be notifyAll. And thanks for the info. 4)Agree on this to. Should add an interrupt check in while. 5)Yes, this should also be done. 6) I'll look into blocking bounded queue. Didn't knew what you told about println(). Thanks. Yup list should not be volatile. Final seems good suggestion. 2. Its just an example so, we can ignore this for now. \$\endgroup\$ – Harinder Apr 27 '15 at 17:25
  • \$\begingroup\$ 2) yes, the producer waits until the list is empty. But you need to wait in a loop that chacks the condition you're waiting for to guard against spurious wake ups (= being awoken when there was no notification.) see tech-read.com/2010/01/28/spurious-wakeup-in-java for a brief explanation or consult Effective Java or Java Concurrency in Practice if you have either available. \$\endgroup\$ – bowmore Apr 27 '15 at 18:57
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for reviewing. When you that I can have size provided by user, so what other approach i should use?. On the for each loop: I did this just to make it user friendly when it would print like 1 ,2 ,3 ,4 while removing/adding. Agree on the spaces thing, It would make it more readable. \$\endgroup\$ – Harinder Apr 27 '15 at 15:58
  • \$\begingroup\$ @Harinder: Sorry for the delay. In order to get a size from the user (assuming it was provided on the command line), you can do: final int size = Integer.parseInt(args[0]);. If provided during runtime instead, you would need to use a Scanner object. \$\endgroup\$ – Jamal Apr 28 '15 at 20:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.