Producer consumer not giving desired result

Question

I'm learning thread synchronization from java documentation. I implemented a famous problem Producer-consumer problem. But it is not giving result as expected. I've searched a lot about this problem at HERE, HERE, HERE, HERE, HERE and some other stack-exchange and non-stack exchange sites but unable to resolve my problem. Here is my code:

GetSetItem.java

public class GetSetItem {

   private volatile boolean available = false;

   private int item;

   public synchronized void set(int item) {
      while(available) {
        try {
            wait();
         } catch (InterruptedException ie) {
            System.err.println("Interrupted: " + ie.getMessage());
         }
       }

       this.item = item;
       available = true;
       notifyAll();
   }

   public synchronized int get() {
      while(!available) {
        try {
           wait();
         } catch (InterruptedException ie) {
            System.err.println("Interrupted: " + ie.getMessage());
         }
       }
       available = false;
       notifyAll();
       return item;
   }

}

Consumer.java

public class Consumer implements Runnable {

   private int number; // Just for show #1,#2 etc. For future use
   private GetSetItem consumer;

   public Consumer(GetSetItem item, int seq) {
      consumer = item;
      number = seq;
   }

   @Override
   public void run() {
      int value = -1;

      for(int i = 0; i < 10; i++) {
         value = consumer.get();
         System.out.println("Consumer #" + number + " get: " + value);
      }

   }
}

Producer.java

public class Producer implements Runnable  {

    private GetSetItem producer;

    private int number = 0;

    public Producer(GetSetItem item, int seq) {
       producer = item;
       number = seq;
    }

    @Override
    public void run() {
       for(int i = 0; i < 10; i++) {
         producer.set(i);
         System.out.println("Producer #" + number + " Put: " + i);
       }
    }

}

ProducerConsumerMain.java

public class ProducerConsumerMain {

    public static void main(String[] args) {

       GetSetItem item = new GetSetItem();

       Producer p = new Producer(item, 1);
       Consumer c = new Consumer(item, 1);

       new Thread(p).start();
       new Thread(c).start();

    }

}

Output is:

Consumer #1 get: 0
Producer #1 Put: 0
Producer #1 Put: 1
Consumer #1 get: 1
Producer #1 Put: 2
Consumer #1 get: 2
Producer #1 Put: 3
Consumer #1 get: 3
Producer #1 Put: 4
Producer #1 Put: 5
Consumer #1 get: 4
Consumer #1 get: 5
Producer #1 Put: 6
Producer #1 Put: 7
Consumer #1 get: 6
Consumer #1 get: 7
Consumer #1 get: 8
Producer #1 Put: 8
Producer #1 Put: 9
Consumer #1 get: 9

But Output should be in producer -> consumer format. It mean consumer can consume an item only if it is available and produced by producer. I've also tried private boolean available = false instead of private volatile boolean available = false; but not receiving expected output.

So please tell me what wrong I'm doing and how can I successfully implement this problem.

score 1 · Answer 1 · answered Feb 24 '15 at 11:30

Your code seems fine, the problem most likely is that System.out is not thread-safe. You'll want to synchronize your println() calls as well:

@Override
public void run() {
    for (int i = 0; i < 10; i++) {
        producer.set(i);
        synchronized (System.out) {
            System.out.println("Producer #" + number + " Put: " + i);
        }
    }
}

Then, the out put will be similar to:

Producer #1 Put: 0
Producer #1 Put: 1
Consumer #1 get: 0
Consumer #1 get: 1
Producer #1 Put: 2
Producer #1 Put: 3
Consumer #1 get: 2
Consumer #1 get: 3
Producer #1 Put: 4
Consumer #1 get: 4
Producer #1 Put: 5
Producer #1 Put: 6
Consumer #1 get: 5
Consumer #1 get: 6
Consumer #1 get: 7 <<<<
Producer #1 Put: 7 <<<<
Producer #1 Put: 8
Consumer #1 get: 8
Consumer #1 get: 9 <<<<
Producer #1 Put: 9 <<<<

It's still possible your threads will be paused between the get/set and println statements, in which case it will appear as if your consumers are consuming things that have not yet been produced, like where I indicated in the above output. It's only a matter of output, though, your code runs fine and does what it's supposed to do.

Can I use "if(available)" statement instead of "while(available)" without changing the behavior because I studied that "wait()" method will block the object until "notify() or notifyAll()" is called. — Sachin Kumar, Feb 24 '15 at 11:36
Blocking is what you're trying to achieve here: Your `Consumer` must wait until the `Producer` says it's done, and then `Producer` cannot produce anything new until the value has been consumed. You could use `if` in this case, but if you have multiple `Producer`s and `Consumer`s, then you'd get in trouble. It would be best to use `notify()`, as it only wakes one thread. Then, it's probably safe to use `if`, but it won't matter much. — DennisW, Feb 24 '15 at 11:40

score 1 · Accepted Answer · answered Feb 25 '15 at 05:48

I solved the problem by using the System.out.println(...) statement in get() and set() methods of GetSetItem class and removing System.out.println(...) from corresponding producer and consumer class. As:
get() Method of GetSetItem.java

public synchronized void set(int item, int number) {
    while(available) {
        try {
        wait();
        } catch (InterruptedException ie) {
        System.err.println("Interrupted: " + ie.getMessage());
        }
    }

    this.item = item;


    /* Putting this line here gives expected output because this   
     * statement is synchronized due to method synchronization.  
     */
        System.out.println("Producer #" + number + " Produced: " + item);            

        available = true;
        notifyAll();
}

set() method of GetSetItem.java

public synchronized int get(int number) {
    while (!available) {
        try {
        wait();
        } catch (InterruptedException ie) {
        System.err.println("Interrupted: " + ie.getMessage());
        }
    }
    /*
     * Putting this line here gives expected output because this statement
     * is synchronized due to method synchronization.
     */


    System.out.println("Consumer #" + number + " Consumed: " + item);


    available = false;
    notifyAll();
    return item;
}

EDIT: output:

Producer #1 Produced: 0
Consumer #1 Consumed: 0
Producer #1 Produced: 1
Consumer #1 Consumed: 1
Producer #1 Produced: 2
Consumer #1 Consumed: 2
Producer #1 Produced: 3
Consumer #1 Consumed: 3
Producer #1 Produced: 4
Consumer #1 Consumed: 4
Producer #1 Produced: 5
Consumer #1 Consumed: 5
Producer #1 Produced: 6
Consumer #1 Consumed: 6
Producer #1 Produced: 7
Consumer #1 Consumed: 7
Producer #1 Produced: 8
Consumer #1 Consumed: 8
Producer #1 Produced: 9
Consumer #1 Consumed: 9

Producer consumer not giving desired result

2 Answers2