43

I love the MVVM Light's Messenger and its flexibility, however I'm experiencing memory leaks when I forget to explicitly unregister the recipients (in Silverlight 4).

The cause is explained here, but I'm fine with it as I believe it's a good practice to explicitly unregister the recipients anyways rather than relying on the Messenger's use of weak references. The problem is that is easier said than done.

  • ViewModels are easy: you usually have full control over their lifecycle and can just Cleanup() them when they are not needed anymore.

  • Views on the other hand are trickier because they are instantiated and destroyed via DataTemplates. For ex. you can think of an ItemsControl with MyView as DataTemplate, bound to an ObservableCollection<MyViewModel>. The MyView controls are created/collected by the binding engine and you have no good way to manually call Cleanup() on them.

I have a solution in mind but would like to know if it's a decent pattern or there are better alternatives. The idea is to send a specific message from the ViewModel to tell the associated View(s) to dispose:

public class MyViewModel : ViewModelBase
{
    ...

    public override void Cleanup()
    {
        // unregisters its own messages, so that we risk no leak
        Messenger.Default.Unregister<...>(this);

        // sends a message telling that this ViewModel is being cleaned
        Messenger.Default.Send(new ViewModelDisposingMessage(this));

        base.Cleanup();
    }
}

public class MyView : UserControl, ICleanup
{
    public MyView()
    {
         // registers to messages it actually needs
         Messenger.Default.Register<...>(this, DoSomething);

         // registers to the ViewModelDisposing message
         Messenger.Default.Register<ViewModelDisposingMessage>(this, m =>
             {
                 if (m.SenderViewModel == this.DataContext)
                     this.Cleanup();
             });
    }

    public void Cleanup()
    {
        Messenger.Default.Unregister<...>(this);
        Messenger.Default.Unregister<ViewModelDisposingMessage>(this);
    }
}

So when you call Cleanup() on a viewModel all the views that use it as DataContext will exeute their local Cleanup() as well.

What do you think? Am I missing something obvious?

Francesco De Vittori
  • 9,100
  • 6
  • 33
  • 43
  • 8
    There is also a pattern for this Cleanup() behaviour of yours. Implement IDisposable instead and use a known pattern. This won't solve your problem but at least other devs will know what your intentions are with Dispose() – Ray Booysen Mar 10 '11 at 08:14
  • 1
    Also, what is the reason that your view needs to register for anything with Messenger. Surely everything will be done via binding? – Ray Booysen Mar 10 '11 at 08:15
  • 1
    @Ray: the view registers to messages for the few viemModel-started actions that cannot be done with bindings, for ex. starting animations, setting a scroll offset (there is no two-way property for that), etc. It's not too frequent, but it happens. – Francesco De Vittori Mar 10 '11 at 08:37
  • 4
    Also, I'm using ICleanup instead of IDisposable because that's what MVVM-Light's ViewModelBase uses. There is a discussion somewhere here where it's explained the reason behind the choice. – Francesco De Vittori Mar 10 '11 at 08:39
  • The "downside" of WPF is that you never know whether a control is unloaded for the last time (it is unloaded every time it is removed from the visual tree, and it might be the last time every time). I am one of the developers of Catel (also an MVVM framework), and our framework takes care of this for you. It has a ViewModelManager and automatically unsubscribes and cleans up all event handling stuff. We noticed that WPF holds a LOT of references by default, so you have to be really careful to clean all garbage. – Geert van Horrik Mar 10 '11 at 10:04
  • I very much like the idea you are proposing, It solves the problem of informing the view that the view model is disposing quite ellegantly! V. 4 SP 1 does implement IDisposable on the ViewModelBase class, so maybe using the "standard way" is not a bad idea, ViewModelBase.Dispose calls Cleanup internally. Maybe, propose this solution to Laurent Bungion for "standarized" inclusion into the view model. – AxelEckenberger Mar 12 '11 at 13:35
  • Thanks Obalix, I'll talk soon to Laurent. – Francesco De Vittori Mar 14 '11 at 08:46
  • 1
    I tend to move away from IDisposable for this. IDisposable implies more than a simple unregistration, it implies that the ViewModel will be disposed after the Dispose method is called, i.e. that it is ready for garbage collection. However, this is not always the case. This is why I am considering removing the IDisposable interface from the ViewModelBase class. It does not mean that I am against using IDisposable, but just that using it by default is not a great idea, which is why I include ICleanup instead. Would be interested to read comments about this. Cheers! – LBugnion Mar 15 '11 at 08:39
  • @LBugnion: Personally I believe that both, the ICleanup, and IDisposable are important. IDisposable especially becomes important when the VMs are created using Unity with a container controlled live time. Using IDisposable ensure a propper cleanup here. For the other cases I can see the case for ICleanup. – AxelEckenberger Mar 15 '11 at 15:16
  • @Obalix: To be clear, I didn't mean that IDisposable was not important. I just decided to remove it from the default implementation of ViewModelBase because it was confusing the concept of cleaning up vs disposing. In V4, ViewModelBase will not implement IDisposable anymore (in fact, this interface had been marked obsolete in V3 already). The only thing it means is that, if you feel that your VM should implement IDisposable, they should do so explicitly. Makes sense? – LBugnion Mar 17 '11 at 11:33
  • @LBugnion: As I said, when the VM is used with Unity the V3 implementation ensures that Cleanup is called when the object is disposed. If this is not present it requires additional work and consideration on the side of the developer, therefore, I think it _should_stay_ as it is in V3, i.e. both ways should be present in the ViewModelBase. – AxelEckenberger Mar 18 '11 at 10:57
  • why don't Unregister all Messengers in one place? Say in App_close()method? this would be much easier then unregister them in the same classes where they have been registered. – Arseny May 03 '11 at 12:29
  • 2
    @Arseny: I prefer to unregister the views immediately when they are disposed otherwise they may respond to messages even if they should be "dead". Also with weak references, the object can still be alive until the GC has collected it (which is not under my control). – Francesco De Vittori May 04 '11 at 09:02
  • Just a note on your "two-way property" comment for scroll offsets. You can achieve this using attached behaviours. Plenty of examples out there. Try this: http://www.scottlogic.co.uk/blog/colin/2010/07/exposing-and-binding-to-a-silverlight-scrollviewer%E2%80%99s-scrollbars/ – Chris Ward Jun 26 '11 at 08:29
  • The problems you are trying to address give me some more confidence in a mediator scoping pattern I've been rolling around in my head for a while. When I implement our mediator I'll consider having scopes that automatically dispose of anything registered within that scope. I'll post on my blog about it when I get around to implementing it. http://jefuri.wordpress.com/ – jpierson Aug 22 '12 at 21:22

3 Answers3

7

The ViewModelLocator class helps keep a centralized store for your viewmodels. You can use this class to help manage new versions and clean up old ones. I always reference my viewmodel from view via the locator, so I always have code I can run to manage these things. You could try that.

As well, I use the Cleanup method to call Messenger.Unregister(this), which cleans up all references from the messenger for that object. You have to call .Cleanup() every time, but such is life :)

Camron B
  • 1,650
  • 2
  • 14
  • 30
  • can you give a code example of viewmodellocator managing versions and doing the cleanups? I'm struggling to understand that... :| – sexta13 Feb 28 '14 at 11:19
1

I've not used MVVM Light (though I've heard great things), but if you want a Messenger implementation that uses WeakReferences, checkout the Messenger included here http://mvvmfoundation.codeplex.com/.

BrandonZeider
  • 8,014
  • 2
  • 23
  • 20
-1

MVVM Light Messenger is using WeakAction(WeakReference). So you don't need to Unregister explicitly.

wafe
  • 382
  • 2
  • 3