I have to write this produce consumer application using multithreading. I wrote the following java code but havn;t been able to figure out where it is getting wrong. Also i want to know whether my class design is apt or if my coding style is appropriate.
Thanks in Advance!!!
EDIT
I have modified the produce consumer code: But it still has some problem.
import java.util.*;
import java.lang.Thread;
public class pc_example {
public static void main (String [] args) {
Store store = new Store( 10 );
produce p = new produce(store);
consume c = new consume (store);
p.start();
c.start();
}
}
class Store {
public Queue<Integer> Q;
public int max_capacity;
Store( int max_capacity ) {
Q = new LinkedList<Integer>();
this.max_capacity = max_capacity;
}
}
class produce extends Thread {
private Store store;
private int element;
produce ( Store store ) {
this.store = store;
this.element = 0;
}
public void put() {
syn开发者_StackOverflow中文版chronized (store) {
if (store.Q.size() > store.max_capacity) {
try {
wait();
} catch (InterruptedException e) {}
}
else {
element ++;
System.out.println( "Producer put: " + element );
store.Q.add(element);
notify();
}
}
}
}
class consume extends Thread {
private int cons;
private Store store;
consume (Store store) {
this.store = store;
this.cons = 0;
}
public void get() {
synchronized (store) {
if (store.Q.size() == 0) {
try {
wait();
} catch (InterruptedException e) {}
}
else {
int a = store.Q.remove();
System.out.println( "Consumer put: " + a );
cons++;
if (store.Q.size() < store.max_capacity)
notify();
}
}
}
}
You are creating two instances of Producer_Consumer
which are having their own queues, so there's no sharing between. You should not instantiate the queue in the classes, but provide it outside as a constructor argument.
class Producer_Consumer extends Thread {
private final Queue<Integer> queue;
Producer_Consumer(int mode, Queue<Integer> queue)
{
this.queue = queue;
}
public static void main(String[] args)
{
Queue<Integer> queue = new LinkedQueue<Integer>();
Producer_Consumer produce = new Producer_Consumer(queue, 2);
Producer_Consumer consume = new Producer_Consumer(queue, 1);
produce.start();
consume.start();
}
}
Further improvements could be done as suggested using a blocking queue from java.util.concurrent
package. There's really no need of using Object
's methods wait()
and notify()
for this kind of tasks.
For a complete example see the producer-consumer example in the java api for BlockingQueue.
There are several errors in the code. For the first the producer and the consumer are not using the same queue e.g. there are two instances of the queues. Secondly notify
and wait
methods are also operating on different objects.
Getting your example to work needs several things:
- Only one queue
- Thread safe handling of the queue
- Handling notification and waiting on the same object
The producer code could be rewritten to:
public void produce() {
int i = 0;
while (i < 100) {
synchronized(Q) {
if (Q.size() < max_capacity) {
Q.add(i);
System.out.println("Produced Item" + i);
i++;
Q.notify();
} else {
try {
Q.wait();
} catch (InterruptedException e) {
System.out.println("Exception");
}
}
}
}
}
1, Use appropriate types. Your mode is much better off as en enumeration instead as an int.
2, Your conduit between the threads, Q, isn't actually shared since it is not declared static.
You would have problems anyway since linkedlist isn't synchronized.
Synchronizing produce()
and consume()
makes no difference.
This is what a BlockingQueue
is for.
Each of your objects is working on a a different instance of the
Queue<Integer> Q
so the producer puts stuff into one, but the consumer never looks in that one - it's trying to get items from a Q
that never gets anything put into it.
However, once you address that you need to make sure that the Queue<>
object is handled in a threadsafe manner. While the produce()
and consume()
methods are each synchronized, the synchronization at this level won't help since you're dealing with two distinct Producer_Consumer
objects. They need to synchronize their access to the shared resource some other way.
I suggest to look at the classes in java.util.concurrent (available from Java 1.5). In particular, instead of a Queue, you might use a BlockingQueue.
It allows you to produce:
try {
while(true) { queue.put(produce()); }
} catch (InterruptedException ex) { ... handle ...}
and consume:
try {
while(true) { consume(queue.take()); }
} catch (InterruptedException ex) { ... handle ...}
Otherwize, (if this is an exercise on java synchronization), you should
- improve the visibility of fields (why only max_capacity is private?)
- improve the design (I prefer to create two separate classes for producers and consumers)
- ensure that producers and consumers wait and notify on the SAME object
- make producers and consumers work on the same queue
Run methods are missing in your Thread classes. So your threads did start and finish doing nothing. Rename the put and get methods to run and use while loop. Also note that you need to call the notify and wait on the store (monitor).
public void run() {
while(true){
synchronized (store) {
if (store.Q.size() > store.max_capacity) {
try {
store.wait();
} catch (InterruptedException e) {}
}
else {
element ++;
System.out.println( "Producer put: " + element );
store.Q.add(element);
store.notify();
}
}
}
}
精彩评论