[ FIXED ] If observer list changes during notification some observers are missed

[mrichard1]

hi,

I have a problem with the notifyObservers method in the class View.

I explain, 2 mediators are attach to a same Notification 'CloseView' and when I send it, only one mediator receive it...

It's 'normal' when you analyse the notifyObservers method. When the 'CloseView' notification is received my Mediator is removed from the Facade and by consequence it removed from the observers attribut list (by reference). the var i = 1 and when the next step of 'for' is call observers.length = 1 not 2 (not such as previously) and the second observer is never notified....

//current method
if( observerMap[ notification.getName() ] != null ) {
   var observers:Array = observerMap[ notification.getName() ] as Array;
   for (var i:Number = 0; i < observers.length; i++) {
      var observer:IObserver = observers[ i ] as IObserver;
      observer.notifyObserver( notification );
   }
}

my idea to solve this problen is to clone (deep copy) the observers to unlink the memory reference temporaly.

Someone have an idea about this problem ? And how create a deep copy because the Observer are not correctly copy by the method ObjectUtil.copy(observers);

Best regards,
Marc

[mrichard1]

I found a solution which work correctly :

( observerMap[ notification.getName() ] != null ) {
   var observers_ref:Array = observerMap[ notification.getName() ] as Array;
   var observers:Array = new Array(); //tempory copy (not deep copy, shallow copy... work fine)
   var observer:IObserver;
   
   for (var i:Number = 0; i < observers_ref.length; i++) { //copy the reference
      observer = observers_ref[ i ] as IObserver;
      observers.push(observer);
   }
   
   for (i = 0; i < observers.length; i++) { //notify the observer
      observer = observers[ i ] as IObserver;
      observer.notifyObserver( notification );
   }
}

Marc

[puremvc]

Good enough. Seems likely the problem will crop up, and that this solution will fix it. I'll test both to make sure it all holds water, and fold this change into the next minor rev.

In the meantime, as a workaround (and actually probably the better thing to do anyway), have a single Command remove the Mediators rather than having them remove themselves. Have the Mediators null the references to their viewComponents in their onRemove methods.

-=Cliff>

[mrichard1]

Thank for your other solution.

Marc

[garkpit]

I have a number of ViewMediators which listen for a LOGOUT notification and remove themselves by using facade.removeMediator. Only half of them are removed. I traced the problem to notifyObservers in View.as. The for loop increments as the observers array decrements, which means you skip every other observer. My solution, after a bit of head scratching, is very simple: reverse the for loop. This fixed it for me:

public function notifyObservers( notification:INotification ) : void
{
   if( observerMap[ notification.getName() ] != null ) {
      var observers:Array = observerMap[ notification.getName() ] as Array;
      // BAD: because observers that remove themselves decrement the array while i increments
      // for (var i:Number = 0; i < observers.length; i++) {
      // GOOD:
      for (var i:Number = observers.length - 1; i >= 0; i--) {
         var observer:IObserver = observers[ i ] as IObserver;
         observer.notifyObserver( notification );
      }
   }
}

I hope others find this helpful. I would suggest adding this fix to the next release.

Jay

[andres]

I came across this very same problem a while back while working on a shooter game. My solution was a bit nastier though.

Although this is so much more simpler and cleaner, I have 2 issues with it:

1. It notifies all registered observers for a particular Notification name in reverse order which is something that I can not have since I've come to rely on the order in which observers are registered in most of my projects. I'm not sure if that's something the framework was designed to guarantee, but I definitely need it to stay that way.

2. It seems to only work in situations where the observer removes itself (only one observer is removed each iteration). What if you have, say a parent Mediator removing MULTIPLE child Mediators registered to the same Notification name as the parent. You'll end up skipping Observers as well.

But again, your solution looks like it definitely works for situations in which the current observer removes itself.


The following should fix the problem when removing multiple observers registered to the same notification name (see below for version with comments)


public function notifyObservers( notification:INotification ):void {

   if ( observerMap[ notification.getName() ] != null ) {
      var observers:Array = observerMap[ notification.getName() ] as Array;
               
        var prevLength:int;
        var removedObservers:int = 0;
        var traversedList:Array = new Array();
               
      for (var i:Number = 0; i < observers.length; i++) {
         var observer:IObserver = observers[ i ] as IObserver;


           traversedList.push(observer);
           prevLength = observers.length;

         observer.notifyObserver( notification );


           if (observers.length < prevLength) {
            
              for (var j:int = 0; j < traversedList.length; j++) {
               
                 if (observers.indexOf(traversedList[j]) == -1) {
                    traversedList.splice(j, 1);
                    j--;

                    removedObservers++;
                 }
              }
           }

           i -= removedObservers;
           removedObservers = 0;

      }
   }
}


//WITH COMMENTS

public function notifyObservers( notification:INotification ):void {

   if ( observerMap[ notification.getName() ] != null ) {
      var observers:Array = observerMap[ notification.getName() ] as Array;
      var prevLength:int;
      var removedObservers:int = 0;
      var traversedList:Array = new Array();
      for (var i:Number = 0; i < observers.length; i++) {
         var observer:IObserver = observers[ i ] as IObserver;

         traversedList.push(observer);
         prevLength = observers.length;

         observer.notifyObserver( notification );

         //If observers.length has changed during execution of the observer
         //then observer(s) were removed from the list and we must move back
         //the i counter so that we resume with the observer after the observer
         //we left off at.
         //Counter will be rolled back only when observers before the current
         //observer are removed (ex. i is at 4 and observers at i=0 and i=1 are removed)
         if (observers.length < prevLength) {
            //traversedList holds list of observers we've already traversed through
            for (var j:int = 0; j < traversedList.length; j++) {
               //Determine the number of observers that were removed by checking
               //if any of the observers in the traversedList no longer exist in
               //the observers list
               if (observers.indexOf(traversedList[j]) == -1) {
                  traversedList.splice(j, 1);
                  j--;

                  removedObservers++;
               }
            }
         }
         //rollback i
         i -= removedObservers;
         removedObservers = 0;
      }
   }
}

[garkpit]

1. I am fairly certain the framework does not guarantee the order Notifications are received in. If I were younger I might remember where I read that!

2. You are correct. My fix does not handle "mediators removing other mediators listening to the same notification" and so is incomplete. Assuming yours works andres I would hope to see it in the next release. Well done!

Jay

[puremvc]

While the framework makes no guarantees about the order of notification, suddenly reversing the order might introduce bad behavior in apps where the developer either relied upon that order conciously or simply expected it as sensible behavior.

-=Cliff>

[puremvc]

Also, now that I'm looking at it, this is the same bug described here:
http://forums.puremvc.org/index.php?topic=490

And that post has the workaround. I'll merge these topics when I'm at my computer, the wap browser on my phone doesn't allow it.

-=Cliff>

[puremvc]

Ok folks.

Looking at these topics merged, we have 3 proposed solutions that should work:

1) Go backwards through the notification list instead of forwards so that removals from the list don't matter.

2) Keep track of the observers traversed so far in a separate array. After each notification pass through the traversed list and see if any are now missing from the observer list and if so drop them from the traversed list and decrement the traversed list pointer and keep going, counting the total we discover to have been removed. Then decrement the outer notification loop by the total removed and continue notification.

3) Copy all the observers to a new array and notify by iterating over the copy. So removals from the actual observer list don't affect the list we're iterating over at all.

My thoughts:

#1 - Doesn't add any logic or extra cycles, but might cause undesired side effects and is somewhat counter-intuitive to how you would expect notification to work. It's short, but not so sweet if you were depending on notification following the order of observer registration.

#2 - Is a valiant effort which really appears to hold water, but monkeying with nested iterators is not pretty to debug, and it would eat up an awful lot of extra cycles for this relative boundary case on every notification.

#3 - Is an elegant approach that really cuts to the heart of the problem - the iterated array may change mid-traverse, so don't work with a reference at all. It'll add the overhead of an extra loop to copy the list each time, but it we're shielded from changes to the original list and don't have to add any logic to the notification loop.

#3 looks like the right approach to me.

-=Cliff>

[andres]

Ok folks.

Looking at these topics merged, we have 3 proposed solutions that should work:

#3 looks like the right approach to me.

-=Cliff>

But this comes back to the 2nd point I made. Solution #3 looks like it will only work when you have a Mediator removing itself. What about a situation where a Mediator is removing Mediators further down in the observers array? The Mediator will get removed from the original observers array, but will continue to exist in its copy, which will obviously be disastrous once the second loop reaches those Mediators that are no longer supposed to exist.

[puremvc]

What about a situation where a Mediator is removing Mediators further down in the observers array? The Mediator will get removed from the original observers array, but will continue to exist in its copy, which will obviously be disastrous once the second loop reaches those Mediators that are no longer supposed to exist.

Now we've drifted into the land of silliness, otherwise known as antipattern.

Think about that scenario. Why would you ever have multiple mediators registered to a notification and one of them responds by removing some of the others?

Ths territory was covered before:

http://forums.puremvc.org/index.php?topic=246

-=Cliff>

[puremvc]

Fixed in 2.0.4. A new unit test proves the fix, and will fail on prior versions of the framework.

See the release notes here: http://trac.puremvc.org/PureMVC_AS3/wiki/ReleaseNotes

-=Cliff>