3

I've got a Window, which gets a viewmodel instance injected into its constructor. The window sets its DataContext to that viewmodel. The viewmodel contains a command to "submit" the window and a command to "cancel" the window.

The commands are attached to buttons in the window (via a Binding expression in XAML). The viewmodel also contains two events: Submitted and Cancelled. The events are invoked whenever the corresponding command of the viewmodel is executed.

I use events on the viewmodel to be able to know from within the window's codebehind if a command was executed on the viewmodel. The window should set its DialogResult, and should Close() itself whenever the viewmodel's submit/cancel command is executed.

The window is always shown using ShowDialog().

The viewmodel instance that is passed into the window constructor, has a lifetime equal to the total lifetime of the application. So, the viewmodel lifetime is longer than the lifetime of the window. While the window itself will go out of scope and will be created again multiple times. So, what happens:

  1. Window is created, viewmodel is put into constructor
  2. Window subscribes to viewmodel events.
  3. Viewmodel command is executed.
  4. Viewmodel invokes an event.
  5. Window reacts to viewmodel event and closes itself (and ShowDialog() returns)
  6. The window goes out of scope.

When above process is executed multiple times, the window instances are actually not garbage collected, because they subscribed to the (longer living) viewmodel events, and internally events keep a reference to their subscribers. This is actually a well-known memory leak mistake in C#.

But I don't know how to solve this issue. I was thinking to implement the IDisposable interface on the window, and unsubscrive from the events in the Dispose method. But some sources say this is not a good idea. Using a destructor (finalizer) on the window doesn't seem to be a good solution either.

What would be best practise?

1
  • Why do you close the window in the first place? it is a fairly common practice to just hide windows that are not needed for the moment.
    – JonasH
    Nov 12, 2021 at 10:50

4 Answers 4

6

Why not unsubscribe when window closes? Thats normally how event aggregation works. You can also use weak references. Thats how I did in this event aggregator for a open source application

public class EventAggregator : IEventAggregator
{
    private readonly WeakReferenceList<object> subscribers = new WeakReferenceList<object>();

    public void Subscribe(object subsriber)
    {
        subscribers.Add(subsriber);
    }

    public void Publish<T>(T message) where T : class
    {
        subscribers
            .OfType<IHandle<T>>()
            .ForEach(s => s.Handle(message));
    }
}

https://github.com/AndersMalmgren/FreePIE/tree/master/FreePIE.Core/Common/Events

3

The window already implements IDispose and has an overridden Dispose(bool) method. Just add your unsubscibes to it. You may need to move the Dispose(bool) override from the .Designer.cs part of your form to the regular .cs part.

0

Proper ownership of resources is something you generally have to think about even with a garbage collector in hand for long-lived applications where avoiding memory leaks (as perceived and defined by customers) is highly desired.

One of the problems I find with logical leaks of this sort (or space leaks or whatever we want to call them) is that they are like stealth fighter bugs. They're very difficult to detect post-introduction and not something you can effectively catch in your automated tests. They might not be as show-stopping as a dangling pointer crash in C, but they are also much harder to detect especially when looking down at a very large-scale codebase modified constantly by busy team members with customers complaining about how they have to restart the application every 30 mins to get it to perform reasonably. Then you might well find, to your horror, after millions of lines of code were introduced, that practically everything in your application state is failing to be freed from memory at the time it should be with all kinds of cross-references extending their lifetimes all the way to shutdown.

So I wholeheartedly echo the recommendation for weak references. They should actually be a fundamental part of the engineering standards for a team in long-lived applications where such leaks ruin the user experience. Your view model doesn't necessarily conceptually own the objects observing it, like your window. It just references it, and that's where weak references come in handy; they cannot prolong the lifetime of the objects they reference for needlessly long times (ex: all the way until your software is shut down). Using them might lead to some exceptions trying to access weak references if you fail to handle removal requests properly, but at least that's something you can easily detect if you failed to unsubscribe, e.g. as opposed to leaks. In domains where such leaks are highly undesirable, I recommend using the weak reference liberally as a way to make these kinds of programmer mistakes far easier to detect (ex: something you can actually detect in your automated unit and integration tests). You want to reduce the number of strong references in those cases to the minimum to things that genuinely do conceptually own such resources and should extend their lifetimes.

Actually, after fixing dozens of hard-to-detect leaks of this sort introduced by other team members and getting a whole bunch of new gray hairs from it, I might have become a little bit too dogmatic but I actually think it would be beneficial if languages made weak references the default reference with the simplest syntax (just T, e.g.) for member objects, and made strong references the thing for which we have to use a more verbose syntax (StrongReference<T>). I think the language could still default to strong references for function parameters and return values, but default to weak references for members of a class and containers so that we explicitly have to say that this class/container should share ownership in the objects it is referencing.

Everyone's mileage will likely vary, but working in real-time applications where maintaining consistent and high frame rates is critical, these leaks are a nightmare to detect and fix but still need to be fixed to avoid angry customers. And I find it horrific a bit that strong references are the default type of references used in containers and members of other objects. So anyway, TLDR but please consider weak references more if you are tripping over such bugs.

-1

An alternative to the very annoying "remember to unsubscribe from all the events" is to keep the window around and reuse it. Instead of closing, Hide it.

1
  • 1
    One has to be very careful with this approach not to have a hidden Window in the background still reacting to events it shouldn't.
    – Doc Brown
    Nov 8, 2021 at 6:43

Not the answer you're looking for? Browse other questions tagged or ask your own question.