views:

31

answers:

1

Hello all,

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

A: 

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();
Adrian Regan
Thanks for your answer. I intentionally dot not mention the subscribing/unsubscribing part. I want to assume there is one fixed listener.My question is rather if it is wise to expose the synchronization outside the class or not.
Jary Zeels
@Jary Zeels, see additions to answer
Adrian Regan
Thanks Adrian for your clarification, I understand it now. The situation with one listener is clear to me. I'm going to think about the situation when more listeners can be registered, because then, re-entrancycan be a problem. You don't want to have the events delivered in the wrong order when for example one of the notified listeners does something which triggers a new notification. I'll think about it and come back with more questions if needed. Thanks again.
Jary Zeels
In your example, why do you call notify() on the listener inside the notifyLock_ synchronization? This means you expose the lock to the outside world, which might result in a deadlock occuring (which happens if the notification triggers the adding or removal of a listener by the another thread and this thread waits for it to be added or removed). As far as I can see, this case is very exceptional.
Jary Zeels
Very true, in your case you should lock notifyLock_ assign temp Listener to l end lock and call notify. If it is a list then lock the notifyLock_ copy the list end lock and call notify. I will update the answer.
Adrian Regan
I think it's a never-ending cycle. After your change, it is again possible to notify a listener which was removed before. I believe it's a trade-off betweenexposing the lock to the outside world and introducing a potential deadlock risk on one side, or be deadlock free but having the possibility to notify 'old' listeners on the other side. Am I seeing this wrong?
Jary Zeels