Is it okay to call notify() on a lock object before calling wait() on it?

Question

I am using wait() and notify() in my code and there is a scenario where the execution can reach notify() first before a wait is actually called on the lock object on another thread. I created a flag "isWaitCalled" in the lock object and using it to skip notify() call if wait() is not called already. Is this flag "isWaitCalled" redundant? Is it okay if notify() is called before wait in one of the scenarios? Thread A:

synchronized (syncObject) {
          try {
                if (!syncObject.isLoadComplete) {
                    syncObject.isWaitCalled = true;
                    syncObject.wait();
                }
          } catch (Exception ex) {
          }                                              
    }

ThreadB:

synchronized (syncObject) {
          try {
                if (!syncObject.isLoadComplete) {
                  syncObject.isLoadComplete =true;
                   if (syncObject.isWaitCalled) {
                       syncObject.notify();         
                   }                  
               }
          } catch (Exception ex) {
          }                                              
    }

Answer 1

Is it okay to call notify() on a lock object before calling wait() on it?

Well, it depends on the context. On the face of it, it does no harm.

However, if you code depends on the thread that calls wait() seeing the specific notification, then it won't work. Notifications are not queued up. If there was nothing waiting when the notify() call happens, the notification is discarded.

However, your code is broken for other reasons.

1. Java wait / notify can produce spurious notifications; i.e. a thread in wait() may get woken up without a specific notify() or notifyAll(). Your code assumes that loadComplete has been set following the return of the wait() in thread A. It can't assume that ... because of spurious wakeups.

2. You are assuming that only one thread could be waiting for the load to complete. If that is not true, then one thread will be woken, and the rest could be stuck forever.

3. Finally, catching and discarding Exception like that is wrong on two counts.

   • You are catching / squashing any other checked or unchecked exception (apart from Error, etc).
   • You are ignoring the one checked exception that you ought to be dealing with. If either thread A or thread B gets an interrupt at an unfortunate time, then things will break. In particular thread A could think that the code has loaded when it hasn't. If you don't intend to deal with interrupts, then treat them as a "this should never happen" case ... and throw an AssertionError, or something equally fatal.

I would advise you to use the standard pattern for implementing a condition variable that is presented in the java.lang.Object javadocs. Use a loop, and don't try to optimize the case of unnecessary notifies. And if you feel the urge to optimize, then try using one of the higher-level concurrency constructs (e.g. a "latch") in preference to implementing your own solution.

If you get "creative" you can get into trouble. And even if your code is perfect, you will cause extra work for the guys maintaining your code. If they runs into mysterious concurrency bugs, they may need to (re-)analyse your "creative" solution from first principles to determine if it is actually sound.

This is how you should implement this. As you can see, there is less code than your solution, and it is easier to understand:

ThreadA:

  synchronized (syncObject) {
      try {
          // The loop deals with spurious wakeups.
          while (!syncObject.isLoadComplete) {
              syncObject.wait();
          }
      } catch (InterruptedException ex) {
           // This is the "easy" way to do it correctly, in the case
           // where interrupts are not designed for.
           throw AssertionError("interrupted unexpectedly");
      }                                              
  }

ThreadB:

  synchronized (syncObject) {
      if (!syncObject.isLoadComplete) {
          syncObject.isLoadComplete = true;
          syncObject.notifyAll();  // use `notify()` only if threadA 
                                   // is guaranteed to be the only waiter.
      }                                              
  }

Answer 2

When you find yourself using the

wait()

method, and that method call is not in a loop, you are almost certainly doing something wrong.

the loop should be of the form

while (_whatever_im_waiting_for_to_happen_has_not_happened_)
    wait();
}

Answer 3

There is no special problem with invoking notify() or notifyAll() on an object on which no thread is waiting. In particular, it will not block, and in this sense the isWaitCalled variable serves no purpose.

Be aware, however, that notifications have effect only on threads that are already waiting when the notification is delivered. In particular, a thread that invokes the object's wait() method after the notify() will block until the next notify. For this reason, the standard wait / notify paradigm requires threads to check before waiting whether they need to wait at all.

In its simplest form, threads would check the value of some shared variable to determine whether to wait and whether, after returning from from waiting, to resume waiting. The notifying thread, for its part, is then responsible for modifying that variable appropriately before sending its notification. That's more or less the reverse of what you present.

Answer 4

Somehow, you have to know whether there is something to wait for or not. Otherwise, you have no way to know whether to call wait. This thing that you're waiting for is called the "predicate". In your case, isLoadComplete is already the predicate, so having another flag that tracks whether or not you need to wait is redundant.

You need to wait if, and only if, isLoadComplete is false. You call notify when, and only when, you've set isLoadComplete to true. What makes you think you need more than that?

Calling notify when no thread is waiting is harmless.