Wait method invoked while two locks are held

The following example is a violation.

public synchronized void method(Object obj)
{
synchronize (obj) {
obj.wait();
}
}

The wait call releases one of the locks on the monitor "obj". To release the rest of the locks held, do we call wait in a while loop? Is this the right solution?

If we don't release the rest of the locks, will it cause a deadlock? I've tried with an example, but could not produce a deadlock, instead got a warning from the FindBugs tool, that this is not a bug, but needs to be looked at.

http://findbugs.sourceforge.net/bugDescriptions.html

It depends on what obj is doing while this thread is waiting.. If obj needs to put a lock on this instance this method is in... deadlock, I think. But remember it depends on the instance since this method and the block are not using any static class level locks.

Remember by calling the synchronized method you are putting a lock on the instance of the class that method is in. (ok... <--- greenhorn... correct me if I'm wrong)

When the JVM enters the synchronized block (and the keyword is synchronized BTW) it puts a lock on the obj until wait(); is called.. however the lock on the INSTANCE OF THE METHOD IS STILL HELD.. It only releases the lock on obj.

Anyway that's my $.02...

I have moved this post to our "Threads and Synchronization" forum for you.

Find bugs will moan correctly because you wait is not in a loop, i.e. your code may not have notified when exiting the wait so you should test if you did indeed notify.

http://download.oracle.com/javase/1.5.0/docs/api/java/lang/Object.html#wait%28%29

Chris Hurst wrote:Find bugs will moan correctly because you wait is not in a loop, i.e. your code may not have notified when exiting the wait so you should test if you did indeed notify.

http://download.oracle.com/javase/1.5.0/docs/api/java/lang/Object.html#wait%28%29

Whereas this is true, it is only part of the problem. As See Furst said, you are gathering the locks on two different Objects, first the instance on which the synchronized method is called and the second obj's lock. When you call obj.wait() you only release the lock on obj, and you hold the lock on the current instance of the current class.

This is a recipe for deadlock if the thread that needs to call obj.notify() needs to acquire a lock on the same instance that the synchronized method holds the lock on. The only 'fix' for this is to avoid ever holding two locks at the same time. Here is an example of where it WILL cause a problem:

package concurrency;

/**
 * If no deadlock occurs we would expect the output:
 * Starting to Wait in one Thread
 * Pausing so Waiter is sure to be waiting...
 * Notifiying Waiter it is ok to go.
 * Okay Waiter, you can finish now!
 * Okay!  I am done!
 * All Finished, now I can exit.
 * 
 * @author steve.luke
 *
 */
public class Woopsie {

synchronized void iWillBeWaiting(Object obj) throws InterruptedException
	{
		synchronized(obj)
		{
			obj.wait();
		}
		
		System.err.println("Okay!  I am done!");
	}
	
	synchronized void iWillLetYouKnow(Object obj)
	{
		System.err.println("Okay Waiter, you can finish now!");
		synchronized(obj)
		{
			obj.notifyAll();
		}
	}
	
	public static void main(String[] args) throws InterruptedException 
	{
		final Woopsie oops = new Woopsie();
		
		final Object obj = new Object();
		
		Thread waiter = new Thread(new Runnable() 
			{
				public void run()
				{
					try {
						oops.iWillBeWaiting(obj);
					} catch (InterruptedException e) {
					}
				}
			});

Thread notifier = new Thread(new Runnable() 
		{
			public void run()
			{
				oops.iWillLetYouKnow(obj);
			}
		});
		
		System.out.println("Starting to Wait in one Thread");
		waiter.start();

System.out.println("Pausing so Waiter is sure to be waiting...");
		Thread.sleep(100);
		
		System.out.println("Notifiying Waiter it is ok to go.");
		notifier.start();

notifier.join();
		waiter.join();
		System.out.println("All Finished, now I can exit.");
	}

}

The expected output is at the top. The actual output will be:

With a hang (deadlock) because the notifier thread can't gain access to the Woopsie instance in order to call notify on obj properly. It can't do it until the waiter thread lets go of the lock, which it can't do (barring spurious notification) until the notify is called on obj.

Real world situations are not so easy to see, and may not necessarily be nested synchronized blocks on the notify side of things, but are often sequential synchronized blocks like this adjustment which still causes the deadlock:

However if you got rid of the sleep in the main thread the notifier could hit the notifyAll() call before the waiter hits the wait() call. This would also cause only partial output and the script to freeze?

Something like:

Starting to Wait in one Thread
Notifiying Waiter it is ok to go.
Okay Waiter, you can finish now!

And it never finishes because it is still waiting. Since the main thread is joined to both threads it never finishes. Does this sound plausible?

See Furst wrote:However if you got rid of the sleep in the main thread the notifier could hit the notifyAll() call before the waiter hits the wait() call. This would also cause only partial output and the script to freeze?

Something like:

Starting to Wait in one Thread
Notifiying Waiter it is ok to go.
Okay Waiter, you can finish now!

And it never finishes because it is still waiting. Since the main thread is joined to both threads it never finishes. Does this sound plausible?

Yeah, that could happen. If you take the sleep out of the main thread then as I see it you will still get a deadlock every time for the main code chunk I posted. The most likely scenario is the same thing that happens with the sleep in:
- The waiter thread gets started first so it gets the looks to the Woopsie instance.
- Then the notifier tries to get the lock and can't, so the output would be the same as with the sleep (except without the pausing line).
But, as you said, there is also the chance that the notifier code can get executed past the notifyAll call before the waiter starts waiting. In which case you will get the situation you describe above.

For that second code chunk for iWillLetYouKnow(), with the sequential synchronized blocks, it is also possible that:
- The notifier will execute first and gets through the part that is synchronized on the Woopsie instance.
- Then the waiter code could get the lock to both the Woopsie instance and the obj instance and reach the wait state
- Then the notifier has a chance to get the lock on obj.
In this case the code would execute without the deadlock sometimes.