0

Hi I have been trying to solve the producer consumer problem in java without semaphores. When I use single producer and single consumer then my code is working fine. But when I add more than one consumer then it is completely messing up, all the consumer threads are going into the synchronized block. I'm not sure why this is happening. Here is my code :

Producer class:

public class Producer implements Runnable {

    Object SharedObject = null;
    String producerName= null;
    Random rn = new Random();

    public Producer(Main m, String s) {
        this.SharedObject = m;
        this.producerName=s;
    }

    public Producer(Main m) {
        this.SharedObject = m;
    }

    public void run() {
        while (true) {
            synchronized (SharedObject) {
                if (Main.itemCount == Main.bufferSize) {
                    try {
                        System.out.println("Producer is sleeping and waiting for notification form Consumer");
                        SharedObject.wait();
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                }

                Main.itemCount++;
                System.out.println(this.producerName+" Produced the item and the item count is : " + Main.itemCount);

                if (Main.itemCount == 1) {
                    SharedObject.notify();
                    System.out.println("Producer Notified the cosumer to wake up");
                }
            }
            try {
                int i = rn.nextInt(100);
                Thread.sleep(i);
            } catch (Exception e) {
                e.printStackTrace();
            }
        }
    }
}

Consumer Class:

public class Consumer implements Runnable {

    Object SharedObject = null;
    String consumerName= null;
    Random rn = new Random();
    public Consumer(Main m, String s) {
        SharedObject = m;
        this.consumerName=s;
    }
    Consumer c= new Consumer((Main) SharedObject,consumerName);
    synchronized void consume(){
        synchronized (SharedObject) {
            if (Main.itemCount == 0) {
                try {
                    System.out.println(this.consumerName+" is sleeping and waiting for notify from Producer");
                    SharedObject.wait();

                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
            }
            Main.itemCount--;
            System.out.println(this.consumerName+" consumed 1 item and the item Count is " + Main.itemCount);
            if (Main.itemCount == 4) {
                SharedObject.notifyAll();
                System.out.println("Consumer notified the producer to wake up");
            }
        }
    }
    public void run() {
        while (true) {
            c.consume();
            try {
                int i = rn.nextInt(100);
                Thread.sleep(i);
            } catch (Exception e) {
                e.printStackTrace();
            }
        }
    }
}

Main Class:

public class Main {

    static int itemCount = 0;
    static int bufferSize = 5;

    public static void main(String[] args) {
        Main m = new Main();
        Thread objP = new Thread(new Producer(m, "Producer1"));
        Thread objC = new Thread(new Consumer(m, "Consumer1"));
        Thread objC2 = new Thread(new Consumer(m, "Consumer2"));
        Thread objC3 = new Thread(new Consumer(m, "Consumer3"));

        objP.start();
        objC.start();
        objC2.start();
        objC3.start();
    }
}
Gray
  • 115,027
  • 24
  • 293
  • 354

3 Answers3

0

You are using notifyAll in the producer, which wakes up all consumer threads waiting on the monitor. If you want only one consumer to wake up, you should use notify From the API documentation:

notify()

Wakes up a single thread that is waiting on this object's monitor.

notifyAll()

Wakes up all threads that are waiting on this object's monitor.

It would also be better for your consumers to actually check that they can consume a resource when they are woken up. If you want to continue to use notifyAll, a consumer should be able to be awoken, and if insufficient resource is available, go back to waiting.


I suggest printing the main.itemCount. This will make it more obvious what the problems you have are.


You have to pay attention to when you are calling notify.

Why does your producer only call notify when there is exactly one item available? Shouldn't the producer call notify whenever there is an item available?

The consumer only tells the producer to wake up when there are 4 items (isn't this full?).

mattm
  • 5,851
  • 11
  • 47
  • 77
  • Hii, Thanks for the reply, I changed the monitor in producer to notify() , even then am not able to get the expected result. When I used notifyAll() I supposed that even though all the consumers are awoken by it, as we have synchronized block I assumed that only one thread would be inside the block at a given time. Now I changed it to notify() but am not able to proceed... – Captain Cold Feb 02 '17 at 22:15
  • @CaptainCold updated answer. There are more problems with your implementation. I was answering why all of your consumer threads wake up. – mattm Feb 02 '17 at 22:47
  • As per my code, the consumer goes to waiting stage only when the item count is "0" so I just added the condition to notify when the item count goes to "1", because when the count is anything other than "0" the consumer will not be waiting. and the similar thing for the producer, The producer goes to sleep stage only when the buffer is full i.e. 5, So we need to wake up the producer only when a consumer consumes an item, making the item count 4, Once the item count is 4 the produces wakes up and keeps trying to get control of the lock to add the item. In mean time even if consumer... – Captain Cold Feb 03 '17 at 03:02
  • consumes again the count will be reduced to 3, 2 and 1 but in all those cases, the produces is already notified and is trying to acquire the lock.@mattm – Captain Cold Feb 03 '17 at 03:08
0

Actually changing notifyAll() to notify() kindoff worked!!! thanks for ua suggestion guys. Here is my code:

Producer class:

package com.source;

import java.util.Random;

public class Producer implements Runnable {

Object SharedObject = null;
String producerName = null;
Random rn = new Random();

public Producer(Main m, String s) {
    this.SharedObject = m;
    this.producerName = s;
}

public Producer(Main m) {
    this.SharedObject = m;
}

public void run() {

    while (true) {

        synchronized (SharedObject) {

            if (Main.itemCount == Main.bufferSize) {
                try {
                    System.out
                            .println(this.producerName + "is sleeping and waiting for notification form Consumer");
                    SharedObject.wait();
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
            }

            Main.itemCount++;
            System.out.println(this.producerName + " Produced the item and the item count is : " + Main.itemCount);

            if (Main.itemCount == 1) {
                SharedObject.notify();
                System.out.println("Producer Notified the cosumer to wake up");
            }

        }

        try {
            int i = rn.nextInt(100);
            Thread.sleep(i);
        } catch (Exception e) {

            e.printStackTrace();
        }

    }

}

}

Consumer Class:

package com.source;

import java.util.Random;

public class Consumer implements Runnable {

Object SharedObject = null;
String consumerName = null;
Random rn = new Random();

public Consumer(Main m, String s) {
    SharedObject = m;
    this.consumerName = s;

}

public void run() {

    while (true) {

        synchronized (SharedObject) {

            if (Main.itemCount == 0) {
                try {
                    System.out.println(this.consumerName + " is sleeping and waiting for notify from Producer");
                    SharedObject.wait();

                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
            }
            Main.itemCount--;
            System.out.println(this.consumerName + " consumed 1 item and the item Count is " + Main.itemCount);

            if (Main.itemCount == 4) {
                SharedObject.notify();
                System.out.println("Consumer notified the producer to wake up");
            }

        }

        try {
            int i = rn.nextInt(1000);
            Thread.sleep(i);
        } catch (Exception e) {

            e.printStackTrace();
        }

    }

}

}

Main Class:

package com.source;

public class Main {

static int itemCount = 0;
static int bufferSize = 5;

public static void main(String[] args) {

    Main m = new Main();
    Thread objP = new Thread(new Producer(m, "Producer1"));
    Thread objC = new Thread(new Consumer(m, "Consumer1"));
    Thread objC2 = new Thread(new Consumer(m, "Consumer2"));
    Thread objC3 = new Thread(new Consumer(m, "Consumer3"));
    Thread objP2 = new Thread(new Producer(m, "Producer2"));
    Thread objP3 = new Thread(new Producer(m, "Producer3"));

    objP.start();
    objC.start();
    objC2.start();
    objC3.start();
    objP2.start();
    objP3.start();

}

}

Once again thanks to everyone for your valuable time and suggestions.

0

Sounds like you are past your initial problem but here's some more feedback.

I believe your real problem was not because of notifyAll() but because your buffer tests were if tests instead of while loops. There are classic race conditions where a thread gets awaken but there are no elements in the buffer. See my notes here. So you code should be something like:

while (Main.itemCount == Main.bufferSize) {

and

while (Main.itemCount == 0) {

Calling notifyAll() exacerbated the problem but the race conditions still exist even with just notify(). As you add more consumers or another producer you will see more problems.

Here is some other feedback.

  • Be very careful of locks within locks. That is a bad pattern typically and one that I use very infrequently. Do you really need consume() to be synchronized?

  • Object instance names should start with a lowercase letter so it should be sharedObject.

  • Any object that you are locking on should be private final if at all possible. You wouldn't want it changing to another object.

  • Using Main. anything is a bad pattern. How about creating an object with the itemCount and bufferSize and then passing the same instance of that object to all of our producer and consumers? It would also be the object you would lock on.

  • Be careful of sprinkling your thread code with System.out.println(...) messages as others have recommended. System.out is a synchronized class so this will add locks and memory synchronization that may move or fix the problem. Yes. Debugging threaded programs is hard.

Gray
  • 115,027
  • 24
  • 293
  • 354