October, 2009 - Brian Noyes' Blog

Avoiding Memory Leaks with CompositeCommands

I had some good follow on discussions from my recent post on Graceful Shutdown with Ward Bell and Jeremy Miller about how “graceful” my shutdown example really was. They argued a point that I agreed was a big weakness with the approach: sending information back to the shell about whether to shutdown through a command parameter is not a very clean path of communication. Also, my arguments about using a command vs a pub-sub event were difficult to defend. Either one would work fine, but both suffer this same problem of obscure communication paths.

Jeremy also pointed out that the use of Prism CompositeCommands is always a little dirty and hazardous for two reasons. First, they are typically used as globals – a static singleton instance of the CompositeCommand that any control can source and any other part of the app can hook up to through the CompositeCommand.RegisterCommand method. And we all know that globals are evil.

The other problem with CompositeCommands is that it is too easy to have some chunk of code register a child command with the RegisterCommand method and then forget that now the CompositeCommand holds a reference chain back to the child command, the object on which the child command is a member, and the object on which the child command handler method exists.

In my simple example, there was the smell of a potential memory leak – an app that lets you open multiple documents, each document gets its own view and view model, and the view models are the command handlers for composite commands. Since those views are being dynamically created (in this case in response to another composite command for “New”), the registration is being done on the fly as their view models are initialized. So this begs the question of when do they get unregistered so that you don’t leak memory (in the form of residual command references into view models that are no longer being used). Prism commands do not currently use weak references like the Prism events do, although you could modify them to do so without too much difficulty.

You can get the updated code for this post here.

With the original sample code from my last post, it turned out this was not really a problem because the sample provided no way to close a view once opened. But it was just a sample focusing on the communication path for the Closing event after all. But say it was a real app and you were going to allow the user to close a document. I’ve modified the sample to allow closing a document through an X button on the tabs that the documents get opened in.

I didn’t take the time to beautifully style it, so forgive me my artistic ineptitude. But now that there is a mechanism to close the views, presumably the references to the views and view models are released when their document is closed, but if we don’t make sure we call UnregisterCommand in that process, we will have an effective memory leak – the CompositeCommands for Shutdown and Save will be holding references to the view models keeping them from being garbage collected. Each new document we open gets a new view and ViewModel instance, registers it, and over time we would see the memory of this app grow and grow.

So the direct solution is to ensure through whatever means practical to call UnregisterCommand before all refs to the ViewModel go away. Lets take a quick look at the code for this sample to get that done.

I added a data template to the shell where the TabControl is defined to add the X buttons into the tab headers:

<DataTemplate x:Key="ButtonTabItem">
    <DockPanel>
        <Button DockPanel.Dock="Right" 
          Command="{Binding Path=Content.DataContext.CloseCommand, 
          RelativeSource={RelativeSource AncestorType=TabItem}}">X
        </Button>
        <TextBlock Text="{Binding Path=Content.DataContext.DocumentTitle,
           RelativeSource={RelativeSource AncestorType=TabItem}}" VerticalAlignment="Center"/>
    </DockPanel>
</DataTemplate>

You can see the binding code is expecting a CloseCommand to be exposed from the ViewModel which is the DataContext of the view, and the view is the Content of the TabItem. The DocumentEditorView did not have to be modified at all, but the CloseCommand was added to the ViewModel as a DelegateCommand, as well as a Closed event to notify the controller:

public DocumentEditorViewModel()
{
    SaveCommand = new DelegateCommand<object>(OnSave, OnCanSave);
    SaveCommand.IsActive = true;
    Commands.SaveCommand.RegisterCommand(SaveCommand);

    ShutdownCommand = new DelegateCommand<CancelEventArgs>(OnShutdown);
    Commands.ShutdownCommand.RegisterCommand(ShutdownCommand);

     ...
    CloseCommand = new DelegateCommand<object>(OnClose);
 }

public DelegateCommand<object> CloseCommand { get; set; }
public event Action<DocumentEditorViewModel> Closed = delegate { };

private void OnClose(object obj)
{
    // Prompt to save, save document, etc.
    Commands.ShutdownCommand.UnregisterCommand(ShutdownCommand);
    Commands.ShutdownCommand.UnregisterCommand(SaveCommand);
    Closed(this); // Notify controller through an event, could also use a Prism event if others might care...
}

In this case, since the command handling for closing can be collocated with the code that did the registration, it is easy to make sure that we UnregisterCommand as part of the closing process. The controller is the one that takes care of presenting the views in this case, so the event is fired so the controller can choose to do what is appropriate, which in this case is to just remove the view from the region. That takes a bit of convoluted code to locate the view that corresponds to the ViewModel since the two are loosely coupled.

private void OnViewClosed(DocumentEditorViewModel vm)
{
    vm.Closed -= OnViewClosed;
    IRegion region = m_RegionManager.Regions[Constants.DOCUMENTREGION];
    DocumentEditorView viewToRemove = null;
    foreach (var view in region.Views)
    {
        DocumentEditorView docView = view as DocumentEditorView;
        if (docView == null) continue;
        if (docView.DataContext == vm)
        {
            viewToRemove = docView;
            break;
        }
    }
    if (viewToRemove != null)
        region.Remove(viewToRemove);
}

So for this simple example, it was not too terribly hard to make sure a memory leak did not happen with the use of the CompositeCommand once I added the functionality to close a view. However, what you can see emerging here is that the complexity is growing disproportionate to the work that this little sample is really doing. With a large app with many views/ViewModels, lots of commands and cross module communications, etc. this could get out of control pretty quick, and it would be easy to miss a place where you are not unregistering correctly.

The net result of all this is that the use of CompositeCommand works, but is not completely satisfying here, and especially not in a large complex app. Modifying CompositeCommand to use weak references would be one choice, or switching to Prism events for the communications would be another choice.

Coupled with this challenge of potential memory leaks is the scenario of the previous post – graceful shutdown communications – is a another responsibility relative to view management that really cries out for some centralized management. Then you have the view activation/deactivation that I will talk about in my next post that also requires some common handling, and it turns out that what you really need is a pattern/abstraction that Prism does not yet really provide – you need a screen conductor / director of some sort that keeps track of what views are being presented, when they are activated/deactivated, whether they can close, etc.

Jeremy is working on a chapter for his upcoming book that will cover an approach to this problem. I and others are trying to wrap our heads around the best way to tackle this problem. Ward Bell has suggested a “Close Service” that is responsible for coordinating the shutdown and he spiked an implementation of this that definitely felt cleaner than doing the back-channel communications through command or event arguments. I’ll be sure to blog more about this as my thoughts gel on it and if I can get a decent implementation put together, or will link to whoever beats me to it publicly first.

But before I get there, I do want to explain the IActiveAware mechanism of Prism, since it was important in getting the application to work correctly as well and is severely under-documented in the Prism docs. So that will be my next post, stay tuned…

You can get the updated code for this post here.

Monday, October 5, 2009 2:22:17 PM (GMT Daylight Time, UTC+01:00)

Comments [0] |