[ FIXED ] v1.7 removeMediator: removed mediator still recieves notifications

[shauno]

Hello!

I updated PureMVC from v1.6 to v1.7 on a project I am working on, and immediately after, my navigation system broke down. From what I can see it boils down to old (removed) mediators still receiving notifications.

In my app, when a new section is requested by the user, a REQUEST_SECTION notification is broadcast triggering a RequestSectionCommand. This command checks to see if a section is currently open. If a section is open, it broadcasts a CHANGE_SECTION notification. The section will respond by tweening out and broadcasting a REMOVE_SECTION notification when the tween is complete. The REMOVE_SECTION notification triggers a RemoveSectionCommand that does some house cleaning and fires a SECTION_REMOVED notification (followed by the initial REQUEST_SECTION notification). The section's Mediator responds to the SECTION_REMOVED notification by nulling it's ViewComponent and calling facade.removeMediator on itself.

Unfortunately, even though the Mediator unregisters itself, it still receives notifications somehow. Is this possible?? Or, is it more likely that I'm doing something silly. I'll try to build a quick test app to see if I can replicate the issue...

... ok, i could, whew! what's really weird is that it's only certain notifications that get through?? arg..

I have uploaded my test here if anyone has a moment to check it out: http://shaun.boyblack.co.za/pmvcTest.zip

For now, I think I'll just have to roll back to 1.6 for this particular app.

Cheers,
Shaun.

[Rhysyngsun]

shauno, downloaded your zip file and I think part of your problem is that you should:

1) Don't try to remove the mediator from within itself. Generally in Mediators you should only be dispatching/responding to notifications and working with your view You should instead create a command for this and send a notification for the command.

2) Your CreateTestCommand is doing a lot of stuff. I simplified it down into 3 seperate commands:

• CreateTestCommand (instaniates a new mediator and adds it via the facade)
• RunTestCommand (simply send a new notification for POKE_TEST)
• RemoveTestCommand ( calls facade.removeMediator )

I modified your code and ran it as a flex application with three buttons that fired off each command respectively. The mediator never responded to poke after it had been deleted.

I zipped up the code and threw it on  my server so you can take a look at it:

http://nathanlevesque.com/codehelp/test_src_solution.zip

(the mxml file simply does the same as your Main.as script did, plus contains the three buttons which call facade.notifyObservers when clicked for the events that the above commands respond to)

Also, if you haven't already, it would be good take browse through the Best Practices:

http://puremvc.org/component/option,com_wrapper/Itemid,30/

Good luck, and if you need any more help, let us know!

[puremvc]

Nathan,

Good catch! You're on your toes, man. This will be a good one to turn into an anti-pattern tutorial at some point.

Shano,

Here is why the mediator instance didn't go away:

This is your CreateTestCommand:

   public class CreateTestCommand extends SimpleCommand implements ICommand
   {
      override public function execute (notification:INotification) : void
      {
         trace('CreateTestCommand');
         var spr:Sprite = new Sprite();
         var test:TestMediator = new TestMediator(spr);
         facade.registerMediator(test);
         
         sendNotification(ApplicationFacade.POKE_TEST);
         sendNotification(ApplicationFacade.REMOVE_TEST);
         sendNotification(ApplicationFacade.POKE_TEST);
         
         var test2:TestMediator = new TestMediator(spr);
         facade.registerMediator(test2);
         
         sendNotification(ApplicationFacade.POKE_TEST);
         sendNotification(ApplicationFacade.REMOVE_TEST);
         sendNotification(ApplicationFacade.POKE_TEST);
         sendNotification(ApplicationFacade.REMOVE_TEST);
      }
      
   }

The problem is that you create the Mediators within the same scope where you remove them, and hang onto a reference.

The removeMediator method removes the reference to the Mediator that is in the View's mediatorMap. It also removes all Observers that have a reference to the Mediator by combing through the View's observerMap.

Inside the scope of the above command, you create and have a local reference to both the mediators. You send notifications that cause the removeMediator method to be run, and all the above occurs for both Mediators, but there are still references in scope: the variables test and test2.

In the unit test (http://puremvc.org/component/option,com_wrapper/Itemid,122/), which runs correctly, I do the creation of the Mediators to be registered in the method calls rather than creating a reference to a new instance and passing to the method.

        /**
         * Tests registering a Mediator for 3 different notifications, removing the
         * Mediator from the View, and seeing that neither notification causes the
         * Mediator to be notified. Added for the fix deployed in version 1.7
         */
        public function testRemoveMediatorAndSubsequentNotify():void {
           
              // Get the Singleton View instance
              var view:IView = View.getInstance();
           
            // Create and register the test mediator to be removed.
            view.registerMediator( new ViewTestMediator2( this ) );
           
            // Create and register the Mediator to remain
            view.registerMediator( new ViewTestMediator( this ) );
           
           
            // test that notifications work
               view.notifyObservers( new Notification(NOTE1) );
               assertTrue( "Expecting lastNotification == NOTE1",
                       lastNotification == NOTE1);

               view.notifyObservers( new Notification(NOTE2) );
               assertTrue( "Expecting lastNotification == NOTE2",
                       lastNotification == NOTE2);

               view.notifyObservers( new Notification(NOTE3) );
               assertTrue( "Expecting lastNotification == NOTE3",
                       lastNotification == NOTE3);
                       
            // Remove the Mediator
            view.removeMediator( ViewTestMediator2.NAME );

            // test that retrieving it now returns null           
               assertTrue( "Expecting view.retrieveMediator( ViewTestMediator2.NAME ) == null",
               view.retrieveMediator( ViewTestMediator2.NAME ) == null );


            // test that notifications no longer work
            // (ViewTestMediator2 is the one that sets lastNotification
            // on this component, and ViewTestMediator)
            lastNotification = null;
           
               view.notifyObservers( new Notification(NOTE1) );
               assertTrue( "Expecting lastNotification != NOTE1",
                       lastNotification != NOTE1);

               view.notifyObservers( new Notification(NOTE2) );
               assertTrue( "Expecting lastNotification != NOTE2",
                       lastNotification != NOTE2);

               view.notifyObservers( new Notification(NOTE3) );
               assertTrue( "Expecting lastNotification != NOTE3",
                       lastNotification != NOTE3);

            cleanup();                                     
        }

So, the long and short of it is, be careful with your references, and don't try to have a Mediator remove itself.

Although further discussion from anyone doing lots of removeMediator calls is certainly open, I'll mark this one defused for now. It seems to be working as expected.

-=Cliff>

[shauno]

Hi Guys!

Thanks for taking a peek at this. Believe me, I know how ugly that test app is - I'm well aware that it's a bad idea for Mediators to remove themselves. Regardless, I'm not sure why those local references would have any effect on the test at hand: yes, the Mediators will still exist in memory, but that has nothing to do with them still receiving notifications after they have been unregistered. I think the messiness of my test got in the way of what I was trying to demonstrate (unless I'm still completely missing the point - very possible!!).

I have created another test. This time, only one Mediator is created, and it does not try to unregister itself. Instead, one command creates the Mediator, another removes the Mediator, and another performs the test (by broadcasting 4 notifications). You'll notice that the Mediator stills responds, but for some reason, not to the first Notification. You can get my (hopefully better) test here:

http://shaun.boyblack.co.za/pmvcTest2.zip

I also took a peek at your Test Flex app Nathan (thanks for putting it together), and managed to get it to demonstrate the same quirky behaviour by adding a couple more notifications into RunTestCommand (POKE_TEST2 and POKE_TEST3). After being removed, the Mediator still responds to 2 of the 3 notifications. I have uploaded it here: http://shaun.boyblack.co.za/test_src_solution2.zip

Sorry if I'm still doing something completely silly, but if I am I'm struggling to see it at the mo! And thanks again for your help

Cheers,
Shaun

[puremvc]

Shauno,

Debugger up, blinders on, flaps set; ten degrees. We're off into the Bemediator Triangle. If you don't hear back from us in 24 hours, send a search party...

Everyone else, if you're calling removeMediator (not very common, but if you are) hold off on the 1.7 upgrade until we're sure what's going on here.

The unit tests seem to show that the method works as expected, removing the mediator and its observers successfully. However Shauno's tests seem to have uncovered the unexpected behavior of removed mediators continuing to respond.

-=Cliff>

[puremvc]

Shauno,

Ok, I found the problem. And the reason why the test in the test suite worked and why yours failed.

The problem was this break in removeMediator:

                if ( observers.length == 0 ) {
                    delete observerMap[ notificationName ];                               
                    break;
                }

It was left over from the previous refactor, and since it was no longer nested in the loop that modified the array, it wasn't necessary. Still being several indents deep, pure code blindness caused me to leave it in.

But why didn't the new unit test catch it? That's wierder.

Inside the unit tests, its pretty difficult to test singletons properly, since they don't have a reset switch (perhaps they should... a 2.0 enhancement perhaps). They accumulate stuff, and your tests have to ignore the other stuff (mediators, observers) that may be cached in the singleton under test. ***

Anyway, there was already a TestMediator being registered for some other notifications, so I created a TestMedator2, and added some new notifications.

The test (ViewTest.testRemoveMediatorAndSubsequentNotify) registers the TestMediator2 with the test itself as the view component, but not holding a reference to the Mediator. The purpose of giving the Mediator a reference to the Test as a view component is so that the mediator can set a property on the test when it receives a notification. Then the test simply sends the notification, and checks its property to see if it was set by the Mediator, QED.

So that's fine, except that just to be sure I wasn't just dealing with observer lists that were just for this mediator, I also subscribed TestMediator to the NOTE1, 2 and 3 notifications. That was the rub.

The break statement in the code above only executes when the observer list falls to zero for a given notification. Since TestMediator was also subscribing to those notifications, the observer list never fell to zero, so the break never happened and everything worked fine.

So with the 1.7 code and unit tests, removing the subscription of those notifications from TestMediator (not TestMediator2) will in fact cause the unit test method to break as it should have. Complicating the issue with that was just wrong. I should've done one test with other subscribers, one without. That's what I'll do next.

Taking out the offending break statement in removeMediator caused your app to work fine. I bolted it onto a Flex app so that I could use the debugger, but that's really of no consequence to the test. Since you're doing this with Flash and are including the PureMVC source code, you can remove the break statement and should see it work fine.

Look for 1.7.1 shortly. that will be the only change.

Thanks for your tenacious work in helping to get this bug ironed out.
-=Cliff>.

*** Ugly true, but the nature of FlexUnit and Singletons. A friend of mine is working on a nice framework called FUnit (http://blog.funit.org/), and I'm thinking of looking into it since it's way more elegant than FlexUnit, but I'm not sure it solves the Singleton testing issue.

[shauno]

Hi Cliff,

Nice one! Glad you got this sorted - and only one sneaky break statement at that! 

Thanks again,
cheers,
Shaun

[puremvc]

Maybe subconsciously I was saying:

Gimme a break with this removeMediator thing already!


-=Cliff>

[puremvc]

Ok, folks. The docs, unit tests, and new downloads for 1.7.1 are up. Again, thanks to Nathan (owner of the Python port) for diving in and doing some early analysis on this one, and to Shauno for reporting it and diligently clarifying his test and pushing the issue along.

The truth is out there...
-=Cliff>

[Rhysyngsun]

Thanks Cliff, glad to be of service!