1

I have the follow method foo which takes an Observer and puts the current thread to sleep until the Observer wakes it up.

For some reason, I keep getting java.lang.IllegalMonitorStateException exception in foo

public void foo(Observer o)
{
    Thread currentThread = Thread.currentThread();
    o.setThread(currentThread);
    // sleep until the observer wakes it
    currentThread.wait(2000);   // <<<<< Exception happens here
}

The Observer object would call currentThread.notifyAll() sometime later when its Observable calls update on it.

public class Observer
{
    private volatile Thread currentThread;
    // ... other code  ....

   public void setThread(Thread t)
   {
       currentThread = t;
   }

   public void update(Observable o)
   {
        currentThread.notify();
   }
}

Any idea what is wrong here?

One Two Three
  • 22,327
  • 24
  • 73
  • 114
  • possible duplicate of [IllegalMonitorStateException on wait() call](http://stackoverflow.com/questions/1537116/illegalmonitorstateexception-on-wait-call) – Gray Apr 08 '13 at 19:08
  • http://stackoverflow.com/questions/1553886/java-lang-illegalmonitorstateexception-m-null-failed-to-get-monitor-for seems pertinent. – Ben Barden Apr 08 '13 at 19:08
  • I'm not sure if it's of any help, but have a look at: http://stackoverflow.com/questions/1537116/illegalmonitorstateexception-on-wait-call and http://stackoverflow.com/questions/886722/how-to-use-wait-and-notify-in-java – PM 77-1 Apr 08 '13 at 19:16
  • That's is NOT the same as this question. I cannot put `synchronized` in `foo` because that would cause the thread to sleep forever. (See my comment below in one of the question! – One Two Three Apr 08 '13 at 19:19

5 Answers5

3

Whenever you call wait(long) or notify() method of object , that thread must own the monitor of that object. So you should declare your block of code calling wait() on the object to be synchronized . So your method 

public void foo(Observer o)

should be defined in following way:

public void foo(Observer o)
{
    Thread currentThread = Thread.currentThread();
    o.setThread(currentThread);
    // sleep until the observer wakes it
    synchronized(currentThread)
    {
      currentThread.wait(2000);   
    }
 }

UPDATE:
Going by your requirement I suggest you that you should call wait on Observer object . So your code for foo should be something like this:

public void foo(Observer o)
{
    synchronized(o)
    {
      o.wait();//Don't pass time as parameter. Let only the Observer object to wake it up.
    }
 }

And your Observer class should be defined in this way:

public class Observer
{
    // ... other code  ....

   /*public void setThread(Thread t)
   {
       currentThread = t;
   }*/
   public synchronized void update(Observable o)
   {
        notify();//Will wake up the Thread waiting on the current Object of Observer
   }
}
Gray
  • 115,027
  • 24
  • 293
  • 354
Vishal K
  • 12,976
  • 2
  • 27
  • 38
  • Sorry, what object should I call? I don't really have an object for sharing. All the I want to do is for the function `foo` to put the current executing thread to sleep and let the `Observer` wake it. – One Two Three Apr 08 '13 at 19:08
  • Why would `foo` be declared as synchronized? I can guarantee that it is not going to be called concurrently. – One Two Three Apr 08 '13 at 19:08
  • Yes you are right..`foo` should not be synchronized, instead `synchronized` should be called on object `currentThread` for the block of code that is calling `wait` method on `currentThread` – Vishal K Apr 08 '13 at 19:15
  • If i add synchronized(currentThread) in foo, the Observer.update would not be able to get access to currentThread because the Observer.update will be called in a different thread from the one foo is executing, which means foo will be sleeping forever, because it is holding currentThread while it sleeps, so update would not be able to wake it) – One Two Three Apr 08 '13 at 19:20
  • `foo` will not be sleeping forever as you have mentioned the time(`2 s`) for which the Thread will wait . See my updated Post.. – Vishal K Apr 08 '13 at 19:50
2

That's is NOT the same as this question. I cannot put synchronized in foo because that would cause the thread to sleep forever.

I don't think you are understanding how wait() and notify() work. You do not wait and notify on the threads, you do it on the same object. When your code does:

currentThread.wait(2000);

it is actually causing the current thread to wait on it's own Thread object. The way to notify() that thread would then be something like:

Thread thread = new Thread(myRunnable);
...
thread.notify();

This is a very strange pattern and is most likely not what you want to do. It shouldn't matter which thread is running the foo() method. If you were using a thread-pool you wouldn't even know which thread was running it.

If you want your Observer thread to notify the thread waiting in foo() then they both need to be working with the same lock object. Something like:

class MyClass {
    ...
    public synchronized void foo() {
        // this is waiting on the current instance of MyClass
        wait(2000);
    }
}

public class Observer {
     ...
     // to wake up the other thread, we lock the same myClass instance
     synchronized (myClass) {
         // and notify that object
         myClass.notify();
     }
}

Or you could create a lock object that they both should share.

final Object lockObject = new Object();
MyClass c1 = new MyClass(lockObject);
Observer obs = new Observer(lockObject();
...
class MyClass {
    private final Object lockObject;
    public MyClass(Object lockObject) {
        this.lockObject = lockObject;
    }
    ...
    public void foo() {
        // this is waiting on the current instance of MyClass
        synchronized (lockObject) {
            lockObject.wait(2000);
        }
    }
}
...
public class Observer {
    private final Object lockObject;
    public Observer(Object lockObject) {
        this.lockObject = lockObject;
    }
    public void update(Observable o) {
        synchronized (lockObject) {
            lockObject.notify();
        }
    }
}

FYI: Lock objects should almost always be final so they can't change their reference. You never want to lock on something that changes like an Integer or a Boolean.

Gray
  • 115,027
  • 24
  • 293
  • 354
  • Ok, firstly I agreed with you that I misunderstood the meaning of `wait` and `sleep`. But I don't think your suggestions (especially the second one) would solve my problem (, though I might be wrong (again!)). So let me clarify what I am try to do. (in the second comment, since it'd be too long here) – One Two Three Apr 08 '13 at 19:53
  • I don't have an object that I need to be 'locked on'. All I want to do is for `foo` to put whatever thread currently calling `foo` to sleep, until the given `observer` (the argument to `foo`) wakes it up. The `observer` would be observing some background task elsewhere. It needs to wake the thread calling `foo` up after the background task that it is observing has finished. (`foo` would actually be `static`, by the way.) – One Two Three Apr 08 '13 at 19:53
  • Is there one instance of the object that has `foo()` in it or multiple instances? You can't call `notify()` on that object? – Gray Apr 08 '13 at 19:57
  • As I said, `foo` is `static`, so it is some sort of library function that anyone can call to wait until the `observer` says some background task has finished. – One Two Three Apr 08 '13 at 19:59
  • The only way to do that @OneTwoThree is to have the library function and the `Observer` share the same object. It could be a `BlockingQueue` or something instead of a lock-object but there is no other way to accomplish what you want unless you want to do a socket connection to an agreed upon port or something. – Gray Apr 08 '13 at 20:04
1

I would prefer not yo use wait or notify as it is low level and gets dirty quickly if not well implemented. It can be solved through binary semaphore.

Semaphore sem = new Semaphore(0);
public void foo(Semaphore f){
    f.acquire();
}

Other thread, can later call f.release which will unblock the other thread

Jatin
  • 31,116
  • 15
  • 98
  • 163
0

To use java.lang.Object.wait() you have to own the monitor, it's usually done by a synchronized block:

public void foo( Observer o ) throws InterruptedException
{
   Thread currentThread = Thread.currentThread();
   o.setThread( currentThread );
   synchronized( currentThread ) {
      currentThread.wait( 2000 );
   }
}

Add the same mechanism around notify()

public void update( Observable o ) throws InterruptedException {
   synchronized( currentThread ) {
      currentThread.notify();
   }

}

Aubin
  • 14,617
  • 9
  • 61
  • 84
  • If i add `synchronized(currentThread)` in foo, the `Observer.update` would not be able to get access to `currentThread` because the `Observer.update` will be called in a different thread from the one `foo` is executing, which means `foo` will be sleeping forever, because it is holding `currentThread` while it sleeps, so `update` would not be able to wake it) – One Two Three Apr 08 '13 at 19:10
0

You can use a synchronized. When you will call currentThread.wait(), the thread will sleep and will free the monitor (currentThread). This thread will after be waiting to be waked up by another thread with a notify() on the same monitor.

Charly
  • 193
  • 1
  • 8