Producer-Consumer problem - i cannot figure out why it terminates prematurely

[Stanley Walker]

HI,


I had some time in my hand and went about trying to solve producer consume problem without taking help from the internet. I thought i was going good, however i cannot understand why the program terminates suddenly.
So i am sure i have done something wrong or i havent checked if the producer is actually waiting when the stack is full or the consumer is waiting when the stack is empty.

Please help, i am really clueless on this. I am giving my code here.

My Stack class - the data structure that I am using, i have added logic to wait in this class.

package com.org.thread.datastructure;

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

public class Stack<T> {

	private List<T> list = new ArrayList<T>();
	private int maxSize;

	public Stack(int maxSize) {
		this.maxSize = maxSize;
	}

	public void push(T e) {
		/*
		 * if (isFull()) throw new RuntimeException("The stack is full");
		 */
		synchronized (this) {
			if (isFull()) {
				try {
					// System.out.println("waiting for consumption to begin");
					this.wait();
					// System.out.println("completed consumption");
				} catch (InterruptedException e1) {
					// TODO Auto-generated catch block
					e1.printStackTrace();
				}
			}
			list.add(e);
			this.notifyAll();
		}

	}

	public T poll() {
		T element = null;
		synchronized (this) {
			// System.out.println("inside polling");
			if (isEmpty())
				try {
					// System.out.println("waiting for elements in stack");
					this.wait();
					// System.out.println("elements in stack");
				} catch (InterruptedException e) {
					// TODO Auto-generated catch block
					e.printStackTrace();
				}
			element = list.remove(0);
			this.notifyAll();
		}
		return element;
	}

	public int size() {
		return list.size();
	}

	public boolean isEmpty() {

		return list.isEmpty();
	}

	public boolean isFull() {
		if (list.isEmpty())
			return false;
		else if (list.size() == maxSize)
			return true;
		else
			return false;
	}
}

My consumer class:

package com.org.thread.consumer;

import com.org.thread.datastructure.Stack;

public class Consumer<E> implements Runnable {

	private Stack<E> stack;

	public Consumer(Stack<E> stack) {
		this.stack = stack;
	}

	public Stack<E> getStack() {
		return stack;
	}

	public void setStack(Stack<E> stack) {
		this.stack = stack;
	}

	@Override
	public void run() {
		// System.out.println("inside starting to consume");
		/*
		 * if (this.stack.isEmpty()) throw new
		 * RuntimeException("the stack is empty!!!"); else
		 */
		while (true) {
			System.out.println("i am consuming the element-->" + stack.poll()
					+ "  from the produced stack");

		}

	}
}

My producer class : the condition that i have given for checking is- if in any condition , the stack returns that the stack is full then the producer throws an exception stating that the stack is not waiting for consumption. since i have allowed my production/consumption to run in a infinite loop, my thought process is that it will never be the case.

package com.org.thread.producer;

import com.org.thread.datastructure.Stack;

@SuppressWarnings("hiding")
public class Producer implements Runnable {

	public Producer(int maxSize) {
		this.MAXSIZE = maxSize;
		this.stack = new Stack<String>(MAXSIZE);
	}

	private Stack<String> stack;

	public Stack<String> getStack() {
		return stack;
	}

	public void setStack(Stack<String> stack) {
		this.stack = stack;
	}

	private int MAXSIZE;

	@Override
	public void run() {

		if (stack.isFull()) {
			/*
			 * System.out .println(
			 * "\n\n The stack is totally full..please wait for consumption");
			 */throw new RuntimeException("Producer is refusing to wait");
		} else {
			/*
			 * System.out.println("stack is not full " +
			 * this.getClass().getCanonicalName());
			 */int n = 90;

			while (true) {
				java.lang.String element = "My Element in stack " + n;
				System.out.println(element);
				stack.push(element);
				// System.out.println(stack.size());
				n++;
			}
		}

	}
}

My test class

	@Test
	public void testProducerConsumer() {
		Producer producer = new Producer(3);
		Consumer<String> consumer = new Consumer<String>(producer.getStack());
		System.out.println(consumer.getStack().isEmpty());
		Thread consumerThread = new Thread(consumer);
		consumerThread.setName("Consumer");
		Thread producerThread = new Thread(producer);
		producerThread.setName("Producer");
		producerThread.start();
		consumerThread.start();

	}

the output of the above code is given below. I am not sure why it always terminates after reaching some point. the point where it terminates is not same.
sample output:

true
My Element in stack 90
My Element in stack 91
i am consuming the element-->My Element in stack 90 from the produced stack
i am consuming the element-->My Element in stack 91 from the produced stack
My Element in stack 92
My Element in stack 93
My Element in stack 94
My Element in stack 95
i am consuming the element-->My Element in stack 92 from the produced stack
i am consuming the element-->My Element in stack 93 from the produced stack
i am consuming the element-->My Element in stack 94 from the produced stack
i am consuming the element-->My Element in stack 95 from the produced stack
My Element in stack 96
My Element in stack 97
My Element in stack 98
My Element in stack 99
i am consuming the element-->My Element in stack 96 from the produced stack
My Element in stack 100
i am consuming the element-->My Element in stack 97 from the produced stack
My Element in stack 101
i am consuming the element-->My Element in stack 98 from the produced stack
My Element in stack 102
i am consuming the element-->My Element in stack 99 from the produced stack
My Element in stack 103
i am consuming the element-->My Element in stack 100 from the produced stack
My Element in stack 104
i am consuming the element-->My Element in stack 101 from the produced stack
My Element in stack 105
i am consuming the element-->My Element in stack 102 from the produced stack
My Element in stack 106
i am consuming the element-->My Element in stack 103 from the produced stack
My Element in stack 107
i am consuming the element-->My Element in stack 104 from the produced stack
My Element in stack 108
i am consuming the element-->My Element in stack 105 from the produced stack
My Element in stack 109
i am consuming the element-->My Element in stack 106 from the produced stack
My Element in stack 110
My Element in stack 111
i am consuming the element-->My Element in stack 107 from the produced stack
i am consuming the element-->My Element in stack 108 from the produced stack
i am consuming the element-->My Element in stack 109 from the produced stack
i am consuming the element-->My Element in stack 110 from the produced stack
My Element in stack 112
My Element in stack 113
i am consuming the element-->My Element in stack 111 from the produced stack
i am consuming the element-->My Element in stack 112 from the produced stack
My Element in stack 114
i am consuming the element-->My Element in stack 113 from the produced stack
My Element in stack 115
i am consuming the element-->My Element in stack 114 from the produced stack
My Element in stack 116
i am consuming the element-->My Element in stack 115 from the produced stack
My Element in stack 117
i am consuming the element-->My Element in stack 116 from the produced stack
i am consuming the element-->My Element in stack 117 from the produced stack
My Element in stack 118
My Element in stack 119
i am consuming the element-->My Element in stack 118 from the produced stack
My Element in stack 120
i am consuming the element-->My Element in stack 119 from the produced stack
i am consuming the element-->My Element in stack 120 from the produced stack
My Element in stack 121
My Element in stack 122
My Element in stack 123

[Steve Luke]

At what point does it stop? I am running the code exactly as posted, and am >450K and still running. Are you running from the command prompt? Do you have some other event that might be stealing time or killing the process (an overly aggressive antivirus or something)?

[Steve Luke]

I have some comments on your code, but none of them would produce a problem that would stop the application...

	private List<T> list = new ArrayList<T>();
	private int maxSize;

	public Stack(int maxSize) {
		this.maxSize = maxSize;
	}

Whenever possible, for multi-threaded state, make things final. In this case, both list and maxSize should be final. You could also pre-size the list to fit maxSize (prevents resizing)

	private final List<T> list;
	private final int maxSize;

	public Stack(int maxSize) {
		this.maxSize = maxSize;
		this.list = new ArrayList<T>(this.maxSize);
	}

This one you repeat several times, I will only show you this once, but you can apply is other places...

	public void push(T e) {
		synchronized (this) {
			if (isFull()) {
				try {
					// System.out.println("waiting for consumption to begin");
					this.wait();
					// System.out.println("completed consumption");
				} catch (InterruptedException e1) {
					// TODO Auto-generated catch block
					e1.printStackTrace();
				}
			}

If an interruption occurs, then you can progress through the method while the list is past capacity. You should change the if(isFull()) to while(isFull()) so, even if interrupted, the method won't progress.

	public void push(T e) {
		synchronized (this) {
			while (isFull()) {
				try {
					this.wait();
				} catch (InterruptedException e1) {
					e1.printStackTrace();
				}
			}

The alternative would be to not catch the interrupted exception, which is how it would be handled in core APIs - since it wouldn't be clear to this class if an interruption was meant to signal stop or was an accident.

	public void push(T e) throws InterruptedException {
		synchronized (this) {
			while (isFull()) {
				this.wait();
			}

But what you shouldn't do is catch it, log it, then pretend it never happened.

	public T poll() {
		T element = null;
		synchronized (this) {
			//...
		}
		return element;
	}

Not sure if defining and returning the element outside the synchronized block is the right thing to do. It doesn't save you anything, and sets you up to use the same paradigm later when it will cause a problem. I would avoid it.

	public T poll() {
		synchronized (this) {
			//...
			T element = list.remove(0);
			this.notifyAll();
			return element;
		}
	}

	public int size() {
		return list.size();
	}

	public boolean isEmpty() {

		return list.isEmpty();
	}

	public boolean isFull() {
		if (list.isEmpty())
			return false;
		else if (list.size() == maxSize)
			return true;
		else
			return false;
	}

These methods are not safe, they need to be synchronized or the results are unreliable.

My producer class : the condition that i have given for checking is- if in any condition , the stack returns that the stack is full then the producer throws an exception stating that the stack is not waiting for consumption.

This check only happens once, when the run() method starts, and the isFull() method is not safe so it can not be relied on. This would only work as a check if the stack were full before the producer was started.

[Steve Luke]

p.s. >3 million loops now, no stop.

[Stanley Walker]

Hi Steve,

I was running the method from a junit test case.
When i ran it from a main method, the method actually continues.
Thank you.

[Stanley Walker]

Thank you Steve for the response. I do have couple of questions though.

This check only happens once, when the run() method starts, and the isFull() method is not safe so it can not be relied on. This would only work as a check if the stack were full before the producer was started.

I totally agree with you that the other methods should have been synchronized and i also understand why my check is incorrect. Please help with a condition/check which will underline that the producer consumer problem is not solved.

Not sure if defining and returning the element outside the synchronized block is the right thing to do. It doesn't save you anything, and sets you up to use the same paradigm later when it will cause a problem. I would avoid it

agreed, that way my bad.

If an interruption occurs, then you can progress through the method while the list is past capacity. You should change the if(isFull()) to while(isFull()) so, even if interrupted, the method won't progress.

That is a very good point and i wanted to confirm that you meant this

public void push(T e) throws InterruptedException{
		synchronized (this) {
			while(isFull()) {
					this.wait();
			}
			list.add(e);
			this.notifyAll();
		}

Whenever possible, for multi-threaded state, make things final. In this case, both list and maxSize should be final

Do you mean that if i want to write thread safe classes i should make all my instance variables as final?

[Steve Luke]

Stanley Walker wrote:Please help with a condition/check which will underline that the producer consumer problem is not solved.

I am not sure I understand the condition you want, or when you want it checked. Take the time to think about it, write it up as a formal spec proposal, and I bet by the time you have it written up you will understand what condition you need.

public void push(T e) throws InterruptedException{
		synchronized (this) {
			while(isFull()) {
					this.wait();
			}
			list.add(e);
			this.notifyAll();
		}

Yup, that looks good to me!

Do you mean that if i want to write thread safe classes i should make all my instance variables as final?

Absolutely. whenever possible, and if it isn't possible, try to re-work it so it is possible. The middle section of this recent topic explains how important final is.

[Stanley Walker]

Thank you steve for all your help and especially for the link.
I will try and write down the checking conditions