Unsubscribing from events - unsubscription or null?

Question

Is this approach correct? I want only one subscription to this events. When I try using unsubscription -=, for some reason events are actually adding and every time it's more.PresentationViewModel is not disposed when closing associated window so I can't rely on IDisposable this time yet it is implemented and events are disposed there also - maybe an overkill.

I want to know if assigning null is going to bring any side effect I might not want.

Thank you.

public PresentationViewModel ViewModel
{
    get
    {
        if (_viewModel != null) return _viewModel;

        _viewModel = (PresentationViewModel) DataContext;
        _viewModel.OnNavigatedToHandler = null; // <-------------------
        _viewModel.OnNavigatedToHandler += OnNavigatedToHandler;
        _viewModel.OnNavigatedFromHandler = null; // <-------------------
        _viewModel.OnNavigatedFromHandler += OnNavigatedFromHandler;

        return _viewModel;
    }
}

And previously I added unsubscription this way which resulted in many calls of this methods:

_viewModel.OnNavigatedToHandler -= OnNavigatedToHandler;
_viewModel.OnNavigatedToHandler += OnNavigatedToHandler;
_viewModel.OnNavigatedFromHandler -= OnNavigatedFromHandler;
_viewModel.OnNavigatedFromHandler += OnNavigatedFromHandler;

It is not subtraction. :D Call it unhook or unsubcribe operator only. — Tanveer Badar, Jan 14 '20 at 12:34
(1) Don't assign null to an event handler like that - it will unsubscribe ALL handlers, even ones that your code didn't subscribe. (2) You shouldn't be subscribing/unsubscribing in a property getter! That's the wrong place to do it. You should be doing it only when a new DataContext is obtained (e.g. in response to `DataContextChanged`) — Matthew Watson, Jan 14 '20 at 12:48
@MatthewWatson, thank you for answer. 1) Yes it will but I need only one instance for a lifetime of this object. It is the easiest way so I suppouse that it might not be the best approach - I would be pleased if you could guide me more in int. 2) This property getter makes sure _viewModel is assigned only once per lifetime. If statement at beggining checks if it is assigned already. Should I check if DataContext is differenc also? — nelaed, Jan 14 '20 at 12:58

Trevor · Accepted Answer · 2020-01-14T13:48:54.480

Is this approach correct ... for some reason events are actually adding...

To prevent your event handler from being invoked when the event is raised, you need to unsubscribe from the event. In order to help prevent any resource leaks, you should unsubscribe from events, read more here about this.

As for your comment about, for some reason events are actually adding, the reason for this is you are not unsubscribing and therefore the object is kept alive by this reference. In return, doing this without unsubscribing would create a memory leak.

As already mention in the comments, do not assign null to the event as, "it will unsubscribe ALL handlers, even ones that your code didn't subscribe." as Matthew Watson has pointed out. Jon Skeet notes, it'll effectively clear the list of subscribers, which means no handlers whatsoever are called at this point, Matthew Watson also mentions this in his comments.

With all this in mind, -= is the proper way of unsubscribing an event and should be used.

References:

How to subscribe to and unsubscribe from events c#

Should I unsubscribe from events

Unsubscribing from events - unsubscription or null?

1 Answers1