Reputation: 13882
Below is my code for implementing the Producer-Consumer problem. Everything is working using notifyAll()
, however due to performance reasons, I'd like to replace all occurrences of notifyAll()
by notify()
.
I see that replacing these calls by changing notifyAll()
to notify()
causes a deadlock to happen. However, all other attempts in replacing these calls have failed.
Is there some clever way to replace these calls with notify()
that make the code below work with a single Consumer and an arbitrary number of Producers?
public class Buffer
{
private volatile String content = "";
private volatile boolean isEmpty = true;
public synchronized void addItem(String s)
{
while(!isEmpty){
try {
wait();
} catch (InterruptedException e) {
e.printStackTrace();
}
};
content = s;
isEmpty = false;
notifyAll();
}
public synchronized String getItem()
{
while(isEmpty) {
try {
wait();
} catch (InterruptedException e) {
e.printStackTrace();
}
};
String temp = content;
isEmpty = true;
notifyAll();
return temp;
}
}
public class Producer implements Runnable
{
private String greeting;
private int repetitions;
private Buffer b;
public Producer(String aGreeting, int aRepetitions, Buffer aBuffer){
greeting = aGreeting;
repetitions = aRepetitions;
b = aBuffer;
}
public void run()
{
for(int i = 1; i <= repetitions; i++) {
b.addItem(greeting + i);
}
}
}
public class Consumer implements Runnable {
private String greeting;
private Buffer b;
public Consumer(String aGreeting, Buffer aBuffer){
greeting = aGreeting;
b = aBuffer;
}
public void run()
{
try
{
while(true){
System.out.println(greeting + b.getItem());
Thread.sleep(100);
}
}
catch(InterruptedException exception){}
}
}
Upvotes: 0
Views: 89
Reputation: 65936
For being able to use .notify()
, you need to garantee, that any possibly awaken thread will "consume" whole "reason" of notification.
E.g., in your case consumer (method .get_item
) frees space for single element in the buffer. And this is a reason for notification from consumer. Because you use single consumer model, only producer (method .add_item
) can be awaken because of this notification. And producer uses whole freed element for store information into it.
So, using .notify()
is consumer is OK.
From the other side, because you use multiple producers, it is possible that notification from one producer will awake another producer. Of course, one producer doesn't consume effect of another one.
So, using .notify()
is producer is BAD.
The most native way for resolve your problem is to use different notifications: one for consumer, and one for producer. So, notification in producer can awake only consumer, which consumes information stored by producer. Different notifications under same critical section can be achived by using Condition:
public class Buffer
{
final Lock lock = new ReentrantLock();
final Condition notFull = lock.newCondition();
final Condition notEmpty = lock.newCondition();
// `volatile` isn't needed for objects accessed under critical section
private String content = "";
private boolean isEmpty = true;
// Use lock instead of `synchronized`.
public void addItem(String s)
{
lock.lock();
try {
while(!isEmpty){
try {
notFull.await(); // Analogue for wait()
} catch (InterruptedException e) {
e.printStackTrace();
}
};
content = s;
isEmpty = false;
notEmpty.signal(); // Analogue for notify()
} finally {
lock.unlock();
}
}
// Use lock instead of `synchronized`.
public String getItem()
{
lock.lock();
try {
while(isEmpty) {
try {
notEmpty.await(); // Analogue for wait()
} catch (InterruptedException e) {
e.printStackTrace();
}
};
String temp = content;
isEmpty = true;
notFull.signal(); // Analogue for notify()
return temp;
} finally {
lock.unlock();
}
}
}
Upvotes: 1
Reputation: 1198
To brief: while notifyAll() notifies all the awaiting threads and notify() notifies any random thread, now this random thread might not be the one you need next, which can cause deadlock. Please refer this example :
The following steps lead us to deadlock. Let's set limit to 1 to keep the example brief.
E1 enqueues an item.
E2 attempts enqueue - checks wait loop - already full - waits
E3 attempts enqueue - checks wait loop - already full - waits
D1 attempts dequeue - and is executing synchronized block
D2 attempts dequeue - blocks on entry to the (synchronized) block - due to D1
D3 attempts dequeue - blocks on entry to the (synchronized) block - due to D1
D1 is executing enqueue - gets the item, calls notify, exits method
The notify happens to wake up E2 (i.e. "any waiting thread")
BUT, D2 enters sync block before E2 can (E2 must reacquire the lock), so E2 blocks on entry to the enqueue sync block
D2 checks wait loop, no more items in queue, so waits
D3 enters block after D2, but before E2, checks wait loop, no more items in queue, so waits
Now there is E3, D2, and D3 waiting!
Finally E2 acquires the lock, enqueues an item, calls notify, exits method
E2's notification wakes E3 (remember any thread can be woken)
E3 checks the wait loop condition, there is already an item in the queue, so waits.
NO MORE THREADS TO CALL NOTIFY and THREE THREADS PERMANENTLY SUSPENDED!
SOLUTION: Replace notify with notifyAll
Due reference, notify() instead of notifyAll() for blocking queue
Upvotes: 1