6
Vote

Closures not working with RelayCommand

description

I'm using RelayCommand with a WPF project (.NET 4.6.1), and if the provided action contains local variables (closures), the command fails silently (the action is not executed and no exception is thrown).

This code fails:
public ICommand GenerateCommand { get; private set; }

public MainWindowViewModel(FileDialogPresenter fileDialogPresenter, TextFileProcessor textFileProcessor)
{
    GenerateCommand = new RelayCommand(() => GenerateJsonFiles(fileDialogPresenter, textFileProcessor));
}

private void GenerateJsonFiles(FileDialogPresenter fileDialogPresenter, TextFileProcessor textFileProcessor)
{
    // do work
}
This also fails:
public ICommand GenerateCommand { get; private set; }

public MainWindowViewModel(FileDialogPresenter fileDialogPresenter, TextFileProcessor textFileProcessor)
{
    Action generateAction = () => GenerateJsonFiles(fileDialogPresenter, textFileProcessor);
    GenerateCommand = new RelayCommand(generateAction);
}

private void GenerateJsonFiles(FileDialogPresenter fileDialogPresenter, TextFileProcessor textFileProcessor)
{
    // do work
}
This works:
private readonly FileDialogPresenter _fileDialogPresenter;
private readonly TextFileProcessor _textFileProcessor;

public ICommand GenerateCommand { get; private set; }

public MainWindowViewModel(FileDialogPresenter fileDialogPresenter, TextFileProcessor textFileProcessor)
{
        _fileDialogPresenter = fileDialogPresenter;
        _textFileProcessor = textFileProcessor;
        GenerateCommand = new RelayCommand(GenerateJsonFiles); // no closures
}

private void GenerateJsonFiles()
{
    // do work (uses fields instead of arguments)
}

comments

lbugnion wrote Mar 1, 2016 at 8:38 AM

Hi,

You are witnessing an unfortunate side effect of using Weak actions, which are dehydrating / rehydrating the content of the action. Closures are not included in that. I will try to find a way to solve that but to be honest I don't know if it's even possible...

Laurent

danthman wrote Mar 10, 2016 at 12:14 AM

Thanks for your reply, Laurent.

I'm sure you've seen this already, but here's a relevant StackOverflow question on the topic.

If the solutions proposed in the answers aren't feasible to implement, it might help to at least mention the limitations on the action in the XML documentation comments for the RelayCommand constructor:

Suggestion:
        /// <summary>
        /// Initializes a new instance of the RelayCommand class that 
        /// can always execute.
        /// </summary>
        /// <param name="execute">The execution logic. Note that closures are not supported due to the use of WeakActions (see http://stackoverflow.com/questions/25730530/).</param>
        /// <exception cref="ArgumentNullException">If the execute argument is null.</exception>
        public RelayCommand(Action execute)
            : this(execute, null)
        {
        }

simonbhlr wrote Mar 15, 2016 at 4:06 PM

Hi,

this also hit me bad from behind when trying to bind Relaycommand to the Stateless State machine with an extension method:
 internal static class CommandExt
    {
        public static ICommand CreateCommand<TState, TTrigger>(
            this StateMachine<TState, TTrigger> stateMachine, TTrigger trigger)
        {
            return new RelayCommand
                (
                () => stateMachine.Fire(trigger),
                () => stateMachine.CanFire(trigger)
                );
        }
    }
this works sometimes and sometimes not..argh..

it allowed me to use
ICommand SomeCommand = _stateDefinition.Machine.CreateCommand(Trigger.SomeTrigger)
now i have to use a very ugly verbose style like
SomeCommand = new GalaSoft.MvvmLight.CommandWpf.RelayCommand(Target1, CanExecute1);

private bool CanExecute1()
    {
      return  _stateDefinition.Machine.CanFire(Trigger.SomeTrigger);
    }
private void Target1()
    {
        _stateDefinition.Machine.Fire(Trigger.SomeTrigger);
    }
``` for each single Command ... is there way to get this working again?

Togakangaroo wrote Apr 20, 2016 at 2:21 PM

Goddamit, just spent a day and a half on this.

Not so much with RelayCommand but with IMessenger. Register.

I had to trace through tons of source code and of course the results are non-deterministic so sometimes it worked and sometimes it didn't.

This is really untenable. I would argue that most of the usage of things like this is via lambdas as they give you one of the more useful constructs for scope management.

As a bare minimum there needs to be an FAQ and doc comments on IMessenger.Register to highlight this issue. Otherwise people are blowing tons of time trying to hunt down why their events sometimes not arriving (or worse, it doesn't manifest until production). If this can't be fixed I would recommend removing the messaging subsystem entirely and asking people to plug in their own as that would at least clarify this limitation.

If WeakAction is absolutely required and this otherwise can't be made workable, (and assuming I understand the issue) how about providing an error-checking mode that can be toggled via code or a config file. In this mode, when you register an event callback it would check Action.Target versus the stacktrace and issue an exception if the target class reference is not found in the callstack.

lbugnion wrote Apr 20, 2016 at 3:24 PM

Hi,

While I understand your frustration, I must underline that almost no one complained about the lack of support for closure in the Messenger, which has been published about 9 years ago. So I would say that most of the usage of lambdas is probably not what you think it is. It seems that more people are using closures with relaycommands, which sounds plausible to me.

That said. I understand the issue and I will think of a way to circumvent it. The biggest issue is that closures are not captured by the WeakAction and cannot be dehydrated / rehydrated. So in essence, the compromise we could have is that, if you want to use closure, you would lose the WeakAction altogether (which would be an opt-in scenario).

Agreed on documenting it. I will add a note in the Messenger's Register method's XML comments now, and in the RelayCommand. This is a temporary measure, and hopefully I can find a better way to handle this.

I remember the days when people were super afraid to use closure because no one really understood how they worked. Seems we came a long way and no with lambdas closures are more popular than ever before (which is actually a good thing, apart from the headache they are causing me now).

Take care
Laurent

Togakangaroo wrote Apr 20, 2016 at 6:26 PM

Thanks for the attention.

I suppose the lack of complaints might have to do with the flow of developers. Previously wpf developers seem to have been flowing from oldschool winforms development where - although lambdas are completely possible and have been encouraged for a while - they are simply not a part of the Microsoft codesamples. Nowadays, however you have a lot more people who are coming from front-end frameworks where lightweight anonymous functions are very common.

Also keep in mind that for every person who reports an issue there are dozens who have had it and said nothing. I suspect with codeplex the number is actually a lot higher as you're also cutting the pool of potential issue reporters by individuals who either have a codeplex account or are willing to create one.

lbugnion wrote Apr 20, 2016 at 6:53 PM

I don't disagree. I just meant in proportion to other issues in this backlog.

For now, I added a comment in the XML doc of all places where this can happen. I will consult with a few .NET people about possible ways to go around this (if there is one, I know who would know about it, and in fact it so happens that I will meet him in person in 2 weeks). If there isn't a way to handle the closure in the WeakAction, I am thinking of proposing a way to opt out of the WeakAction (in which case you would have to make sure to Unregister the Messenger (or discard the RelayCommand) to allow for garbage collection.

Should be a really interesting issue to solve. In the mean time, the only way around this is to avoid closures...

Thanks again for the discussion,
Laurent

schumm wrote Nov 23, 2016 at 3:52 AM

I got bit by this bug when using the Messenger, nothing like spending a few housr trying to figure out why this isn't working. If proper closure support isn't going to be supported, then you should set MVVM Light to end of life. Closures are major part of C# now, not properly supporting an essential language feature just makes MVVM Light not fit for purpose.

lbugnion wrote Nov 23, 2016 at 9:19 AM

Hi,

I understand the frustration. Unfortunately as described in the Stackoverflow reference above, this problem just cannot be solved in an elegant manner.

In the next version of MVVM Light, I will give the user the possibility to opt-in into a behavior where the reference to the anonymous class created by the closure will be kept no matter what. This will prevent the garbage collector to collect this instance (which is the reason why the WeakAction fails when you use closures).

To be clear, this will cause a (small) potential memory leak to appear. By opting into the behavior (concretely, by passing "true" to an optional parameter in the RelayCommand constructor and in the Register method of the Messenger), you will acknowledge that this is indeed the behavior that you prefer. If you don't want this, you can always use the default behavior, and avoid using a closure.

Hopefully this will solve the problem in a satisfactory, non-breaking manner.
I am planning to push this version as a preview this weekend. Please stay tuned.

Laurent

lbugnion wrote Nov 28, 2016 at 10:00 AM

Quick update after the weekend: I have a proposed implementation which would solve this issue but I am still looking for the best way to change the API (or not). I am contacting the C# team to run a few questions by them. If all goes well, I should be able to propose a poll on the change within a few days.

Laurent

lbugnion wrote Dec 4, 2016 at 4:42 PM

Hi,

I just published an alpha version of V5.4.0 to Nuget. This version proposes a fix to this issue. This requires setting a new parameter called keepTargetAlive to true when you call Register. Note that while this allows using a closure, this also risks causing a (small) memory leak.

I have preliminary documentation here: [http://www.mvvmlight.net/doc/weakaction.cshtml]. A more detailed article will be published as soon as I have time to finish it..

Hopefully this helps. This is an alpha, which means I am interested on your feedback.

Thanks!
Laurent