notify listener inside or outside inner synchronization

Question

I am struggling with a decision. I am writing a thread-safe library/API. Listeners can be registered, so the client is notified when something interesting happens. Which of the two implementations is most common?

class MyModule {
   protected Listener listener; 

   protected void somethingHappens() {
        synchronized(this) {
             ... do useful stuff ...
             listener.notify();
        }
    }
}

or

class MyModule {
   protected Listener listener; 

   protected void somethingHappens() {
        Listener l = null;

        synchronized(this) {
             ... do useful stuff ...
             l = listener;
        }
        l.notify();
    }
}

In the first implementation, the listener is notified inside the synchronization. In the second implementation, this is done outside the synchronization.

I feel that the second one is advised, as it makes less room for potential deadlocks. But I am having trouble to convince myself.

A downside of the second imlementation is that the client might receive 'incorrect' notifications, which happens if it accessed and changed the module prior to the l.notify() statement. For example, if it asked the module to stop sending notifications, this notificaiton is sent anyway. This is not the case in the first implementation.

thanks a lot

Answer 1

It depends on where you are getting listener in your method, how many listeners you have, how the listener subscribes/unsubscribes

Assuming from your example, you have only one listener then you might be better to use critical sections (or monitors) for different parts of the class rather than locking the entire object.

You could have one lock for performing tasks within the method that are specific to the object/task at hand, and one for the listener subscribe/unsubscribe/notify (that is to ensure that the listener is not changed during a notification).

I would also use a ReadWriteLock protecting you listener references (either single or list of listeners)

Answering you comment:

I think that you should notify the listener after you have unlocked the class. This is because, the result of that notification could result in a different thread trying to gain access to the class, which it may not be able to do, under certain circumstances, leading to deadlock.

Notifying a listener (if protected like I have described) should not hold up any other thread that requires the facilities of the class. The best strategy is to create locks that are specific to the state of the class and locks that are specific to safe notification.

If you take your example of suspending notifications, this could be covered by the lock that governs notifications, so if a different thread 'suspends' notifications, either the suspend will be processed or the current notification complete, if the other thread suspends notification between the task being processed and the notification happening, the l.notify() will not happen.

Listener l = null;

synchronised(processLock_) {
   ... do stuff....
   synchronised(notifyLock_) {
      l = listener;
   }
} 
//
// current thread preempted by other thread that suspends notification here.
//

synchronised(notifyLock_) { // ideally use a readwritelock here...
   l = allowNotify_ ? l: null;
}
if(l)
   l.notify();