RelayCommand and WeakActions. Why?

Apr 27, 2012 at 8:17 AM
Edited Apr 29, 2012 at 4:39 AM

Hi Laurent.

I was trying to understand the reasoning behind using WeakActions in RelayToCommand.
Does this try to target memory leak scenarios like the EventToCommand one?
If so then i have to disagree with the solution for the following reasons:

1) Commands ARE supposed to hold on to the ViewModel.

Its just the natural way of things.There is no point in weakly connecting them.If one goes away
then the other will as well.There is no point in the Command outliving its container ViewModel.
Its like adding a UserControl eventhandler on some control inside its visual tree. There is no
leak there because they both hold on to each other ,as they are meant to do, and they die
together.

If the fear is that anyone that keeps the Command also keeps the ViewModel then that is
reasonable and expected as well. If i need the command then i obviously need the ViewModel it
acts upon.

The Command anyways will be held through a databinding (DP) and there's built in support to be held weakly.

2) It forces a totally unnecessary restriction on the user of the API. Why o why should my already
encapsulated command logic be forced to be public or static??

This is the most trivial correct way of using commands from its inception : 

 

public ICommand AddCommand
{
   get { return new DelegateCommand(this.CanAdd, this.Add); }
}

private bool CanAdd() { return true; }

private void Add() {}

 

Does that mean that my private methods now HAVE to be public? If yes then *why*?

3) In trying to solve memory leaks it unfortunately focuses on the wrong link in the chain.

In the case of EventToCommand the problem is in the fact that "the Command holds on
to the EventToCommand" when it shouldn't.

That connection is the right one to be solved and made weak with a WeakEventListener.

So to recap :  

- Commands are meant to strongly reference their ViewModel.
- Commands should not reference elements strongly when there is a reason.Its the responsibility of
 the user of the Command (and not the Command itself) though to know that reason and provide an ad-hoc
solution like WeakEventListener (see EventToCommand)
- Command callbacks should be instance private if they want! 

 

Looking forward to your thoughts on this.

 

Thanx

I have blogged about the issue here

Coordinator
Apr 29, 2012 at 12:01 PM

Hi,

My concern here was about the scenario where a RelayCommand’s Execute method is defined on another object. In that case the RelayCommand would hold a reference to that other object and there would be a potential leak. I do agree that it is not a common scenario, but it is one for which I got a bug report earlier.

The issue with the privacy of methods is a side effect of Silverlight’s sandbox system (this issue does not exist in .NET for instance, the .NET implementation can handle private delegates too). That said, note that you can continue to use private methods for your commands. The WeakAction implementation detects if a private method is used in Silverlight, in which case it will use the old implementation. So if you use a private method, it works but there is a tiny risk of memory leak. If you use a public or static method, it works too and this is a true weak event implementation.

Do you think that makes sense?

Regarding the EventToCommand, I saw your blog post, and this is definitely something I want to improve. However I want to release V4 first because it solves other issues that are quite important. I think that the EventToCommand leak is going a good feature for V4.1.

Cheers,

Laurent

From: atomaras [email removed]
Sent: Friday, April 27, 2012 9:17 AM
To: [email removed]
Subject: RelayCommand and WeakActions. Why? [mvvmlight:353774]

From: atomaras

Hi Laurent.

I was trying to understand the reasoning behind using WeakActions in RelayToCommand.
Does this try to target memory leak scenarios like the EventToCommand one?
If so then i have to disagree with the solution for the following reasons:

1) Commands ARE supposed to hold on to the ViewModel.

Its just the natural way of things.There is no point in weakly connecting them.If one goes away
then the other will as well.There is no point in the Command outliving its container ViewModel.
Its like adding a UserControl eventhandler on some control inside its visual tree. There is no
leak there.The control can die at any point cause IT holds the UserControl. The UserControl
can die because IT holds the control and they die together.

If the fear is that anyone that keeps the Command also keeps the ViewModel then that is
reasonable and expected as well. If i need the command then i obviously need the ViewModel it
acts upon.

The Command anyways will be held through a databinding (DP) and there's built in support to be held weakly.

2) It forces a totally unnecessary restriction on the user of the API. Why o why should my already
encapsulated command logic be forced to be public or static??

This is the most trivial correct way of using commands from its inception :

public ICommand AddCommand

{

get { return new DelegateCommand(this.CanAdd, this.Add); }

}

private bool CanAdd() { return true; }

private void Add() {}

Does that mean that my private methods now HAVE to be public? If yes then *why*?

3) In trying to solve memory leaks it unfortunately focuses on the wrong link in the chain.

In the case of EventToCommand the problem is in the fact that "the Command holds on
to the EventToCommand" when it shouldn't.

That connection is the right one to be solved and made weak with a WeakEventListener.

So to recap :

- Commands are meant to reference strongly their ViewModel.
- Command should not reference elements strongly when there is a reason.Its the responsibility of
the user of the Command (and not the Command itself) to know that reason and provide an ad-hoc
solution like WeakEventListener (see EventToCommand)
- Command callbacks should be instance private if they want!

Looking forward to your thoughts on this.

Thanx

I have blogged about the issue
here

Oct 4, 2013 at 9:33 AM
Hi, Laurent
I noted, now there's 4.1.27 in Nuget.
Is this issue with memory leaks solved already?