CODEWITHSUNDEEP
javamultithreadingdeadlockwait
kenny
kenny

Reputation: 2040

Wait/Notify dead lock

I have a queue with some blocking mechanism in "Add" and "Get" methods, where first thread adds data and second thread gets data.

public synchronized MyObj getData() {               
    synchronized (myLock) {
        synchronized (this) {
            if (isEmpty()) {                
                wait(0);                    
            }
        }       


        return getData();           
    }
}

public synchronized void addData(MyObj data) {
    if (!isFull()) {
        putData(data);
        synchronized (this) {
            notify();
        }
    }
}

In the code above, if first thread tries to get data and queue is empty i put in wait via wait(0) until other thread add data to queue an release from waiting via notify().

Now I want to add another "lock" when queue is full and some one tries to add more data to it:

public synchronized MyObj getData() {               
    synchronized (myLock) {
        synchronized (this) {
            if (isEmpty()) {                
                wait(0);                    
            }
        }       

        synchronized (this) {
            notify();
        }
        return getData();           
    }
}

public synchronized void addData(MyObj data) {
    synchronized (myLock) {
        synchronized (this) {
            if (isFull()) {
                wait(0);
            }
        }
    }

    synchronized (this) {
        notify();
        }
        PutData(data);
}

The result is not what I expect , I guess that i get a dead lock cause process is stuck.

UPDATE

This is how I get data:

queueSize--;
startPointer = (startPointer + 1) % mqueueSize;
data = (String) queue[startPointer];

this is how i add data

  queueSize++;
  endPointer = (endPointer + 1) % mqueueSize;
  queue[endPointer] = data;

public synchronized boolean isEmpty() {
        return queueSize== 0;
    }

    public synchronized boolean isFull() {
        return queueSize== mqueueSize;
    }

Upvotes: 1

Views: 391

Answers (5)

Binil Thomas
Binil Thomas

Reputation: 13789

As already mentioned, your code has too many synchronized annotations. Also, the condition to wait on is checked in an if conditional, but it should ideally be checked in a while loop to avoid spurious wakeups. Here is the outline of the code that fixes these.

// _isEmpty and _getData are private unsynchronized methods
public MyData get() throws InterruptedException {
  // wait and notify should be called from a block
  // synchronized on the same lock object (here myLock)       
  synchronized (myLock) {
    // the condition should be tested in a while loop
    // to avoid issues with spurious wakeups
    while (_isEmpty()) {
      // releases the lock and wait for a notify to be called
      myLock.wait();
    }
    // when control reaches here, we know for sure that
    // the queue is not empty
    MyData data = _getData();
    // try to wake up all waiting threads - maybe some thread
    // is waiting for the queue not to be full
    myLock.notifyAll();
  }
}

// _isFull and _putData are private unsynchronized methods
public void put(MyData obj) throws InterruptedException {
  synchronized (myLock) {
    while (_isFull()) {
      myLock.wait();
    }
    _putData(obj);
    myLock.notifyAll();
  }
}

Upvotes: 0

Tudor
Tudor

Reputation: 62459

Why do you have three synchronized statements? The wait(0) only releases the lock on this, so just keep that one and dump the synchronized from the method and the synchronized(myLock).

Whenever you call wait on some object (in this case you are calling on this), the lock on that object is automatically released to allow the other thread to proceed. But you are never calling wait on myLock (and nor should you, because you are calling on this already). That part is redundant and causes the deadlock.

Consider this scenario: the thread that is supposed to add takes the lock on myLock but finds the queue full, so it waits. This wait does not release the lock on myLock. The other thread wants to take data but cannot enter the synchronized block because the first thread did not release the lock on myLock.

Conclusion: remove the synchronized(myLock) blocks.

Upvotes: 3

Costi Ciudatu
Costi Ciudatu

Reputation: 38235

Remove the synchronized keyword from your method signatures, as that implies you hold the this monitor for the whole method call -- the synchronized(this) blocks are simply redundant.

EDIT:

...Then call wait and notify on myLock rather than this. And forget completely about synchronizing on this. This is because while waiting (on this in your current code), you're not releasing the myLock lock, so the other thread is not able to get to notify().

Upvotes: 1

Piotr Praszmo
Piotr Praszmo

Reputation: 18330

Replace if with while. It won't hurt to double check, if the collection really become not empty/not full.

You don't really need two locks. Single lock will work almost as well and should be much simpler.

public synchronized T get()
{
    while(isEmpty())
        wait(0);

    notifyAll();

    return super.get();

}

public synchronized put(T t)
{

    while(isFull())
        wait(0);

    super.put(t);

    notifyAll();

}

All threads will wake up when something changes. But if they can't do their work, they will wait for next notify.

Upvotes: 0

Valchev
Valchev

Reputation: 1530

Why you don't take a look in java.util.BlockingQueue. Probably it will be useful in your situation.

Particularly take a look at java.util.LinkedBlockingQueue, where if you specify the queue's capacity in the constructor, then the queue will block.

Upvotes: 1

Related Questions