views:

393

answers:

2

Hello all:

I have a following program:

import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;


public class SimpleWaitNotify implements Runnable {

final static Object obj = new Object();
static boolean value = true;

public synchronized void flag()  {
    System.out.println("Before Wait");
    try {
        obj.wait();
    } catch (InterruptedException e) {
        System.out.println("Thread interrupted");
    }
    System.out.println("After Being Notified");
}

public synchronized void unflag() {
    System.out.println("Before Notify All");
    obj.notifyAll();
    System.out.println("After Notify All Method Call");
}

public void run() {
    if (value) {
        flag();
    } else {
        unflag();
    }
}

public static void main(String[] args) throws InterruptedException {
    ExecutorService pool = Executors.newFixedThreadPool(4);
    SimpleWaitNotify sWait = new SimpleWaitNotify();
    pool.execute(sWait);
    SimpleWaitNotify.value = false;
    SimpleWaitNotify sNotify = new SimpleWaitNotify();
    pool.execute(sNotify);
    pool.shutdown();

}

}

When I wait on obj, I get the following exception Exception in thread "pool-1-thread-1" java.lang.IllegalMonitorStateException: current thread not owner for each of the two threads.

But if I use SimpleWaitNotify's monitor then the program execution is suspended. In other words, I think it suspends current execution thread and in turn the executor. Any help towards understanding what's going on would be duly appreciated.

This is an area1 where the theory and javadoc seem straightforward, and since there aren't many examples, conceptually left a big gap in me.

+2  A: 

You're calling wait and notifyAll on obj, but you're synchronizing on this (because you've got synchronized methods).

In order to wait or notify, you need to "own" the monitor first. Unsynchronize the methods, and synchronize on obj instead:

public void flag()  {
    System.out.println("Before Wait");
    synchronized (obj) {
        try {
            obj.wait();
        } catch (InterruptedException e) {
            System.out.println("Thread interrupted");
        }
    }
    System.out.println("After Being Notified");
}

public void unflag() {
    System.out.println("Before Notify All");
    synchronized (obj) {
        obj.notifyAll();
    }
    System.out.println("After Notify All Method Call");
}
Jon Skeet
Great. Can do achieve the same with getting rid of composite object `obj` altogether?
Maddy
@Maddy: You should be able to wait and notify on "this" - but it's generally a bad idea, as "this" is typically a reference which other code has access to, effectively. Keeping a private reference to an object which only your code knows about is good practice.
Jon Skeet
Good point. Agreed.
Maddy
+2  A: 

Either synchronize on obj, or call wait and notify on this. The calling thread must hold the monitor of the same object on which these methods are called.

For example,

synchronized void flag() {
  System.out.println("Before Wait");
  try {
    wait();
  } catch (InterruptedException e) {
    System.out.println("Thread interrupted");
  }
  System.out.println("After Being Notified");
}

In this example, the lock is held on this (when the modifier synchronized is used on a instance method, the monitor of the instance is acquired). So, the wait() method may be invoked on the implied instance this.


In order to coordinate the two threads, they need to share the same lock. The original version had a static obj that could be used as a lock, but it wasn't used in the synchronized blocks. Here is a better example:

class SimpleWaitNotify implements Runnable {

  private final Object lock;
  private final boolean wait;

  SimpleWaitNotify(Object lock, boolean wait) {
    this.lock = lock;
    this.wait = wait;
  }

  public void flag()  {
    synchronized (lock) {
      System.out.println("Before Wait");
      try {
        lock.wait();
        System.out.println("After Being Notified");
      } catch (InterruptedException ex) {
        System.out.println("Thread interrupted");
      }
    }
  }

  public void unflag() {
    synchronized(lock) {
      System.out.println("Before Notify All");
      lock.notifyAll();
      System.out.println("After Notify All Method Call");
    }
  }

  public void run() {
    if (wait) {
      flag();
    } else {
      unflag();
    }
  }

  public static void main(String[] argv) throws Exception {
    ExecutorService pool = Executors.newFixedThreadPool(4);
    Object shared = new Object();
    SimpleWaitNotify sWait = new SimpleWaitNotify(shared, true);
    pool.execute(sWait);
    SimpleWaitNotify sNotify = new SimpleWaitNotify(shared, false);
    pool.execute(sNotify);
    pool.shutdown();
  }

}
erickson
I changed instead to wait on SimpleWait object itself. However, in this case, the program does not halt. Rather the suspended thread is not notified. I get the following printed. Before Wait Before Notify All After Notify All Method Call
Maddy
@Maddy okay, I see the problem. Please check out my update.
erickson