Problem with loop optimization or closure of lambda?

Question

In the following method I'm sending an enumeration of actions and want an array of ICommands back that call Action<object> that wrap those actions (needed for the relayCommand).

The problem is that if I do this inside the for each (or even a for loop) I get commands that always execute the first action passed in the parameters.

    public static ICommand[] CreateCommands(IEnumerable<Action> actions)
    {
        List<ICommand> commands = new List<ICommand>();

        Action[] actionArray = actions.ToArray();

        // works
        //commands.Add(new RelayCommand(o => { actionArray[0](); }));  // (_execute = {Method = {Void <CreateCommands>b__0(System.Object)}})
        //commands.Add(new RelayCommand(o => { actionArray[1](); }));  // (_execute = {Method = {Void <CreateCommands>b__1(System.Object)}})

        foreach (var action in actionArray)
        {
            // always add the same _execute member for each RelayCommand (_execute = {Method = {Void <CreateCommands>b__0(System.Object)}})
            commands.Add(new RelayCommand(o => { action(); }));
        }

        return commands.ToArray();
    }

It seems that the lambda is always reused inside the loop thinking it does the same, but it does not.

How do I overcome this situation? How can i force the loop to threat o => { action(); } always as a new one?

Thanks!

What I tried as per suggestions, but did not help:

        foreach (var action in actionArray)
        {
            Action<object> executeHandler = o => { action(); };
            commands.Add(new RelayCommand(executeHandler));
        }

What seems to work for me is:

    class RelayExecuteWrapper
    {
        Action _action;

        public RelayExecuteWrapper(Action action)
        {
            _action = action;
        }

        public void Execute(object o) 
        {
            _action();
        }
    }

/// ...
    foreach (var action in actionArray)
    {
        RelayExecuteWrapper rxw = new RelayExecuteWrapper(action);
        commands.Add(new RelayCommand(rxw.Execute));
    }

Code of RelayCommand:

/// <summary>
/// A command whose sole purpose is to 
/// relay its functionality to other
/// objects by invoking delegates. The
/// default return value for the CanExecute
/// method is 'true'.
/// </summary>
public class RelayCommand : ICommand
{
    #region Fields

    readonly Action<object> _execute;
    readonly Predicate<object> _canExecute;        

    #endregion // Fields

    #region Constructors

    /// <summary>
    /// Creates a new command that can always execute.
    /// </summary>
    /// <param name="execute">The execution logic.</param>
    public RelayCommand(Action<object> execute)
        : this(execute, null)
    {
    }

    /// <summary>
    /// Creates a new command.
    /// </summary>
    /// <param name="execute">The execution logic.</param>
    /// <param name="canExecute">The execution status logic.</param>
    public RelayCommand(Action<object> execute, Predicate<object> canExecute)
    {
        if (execute == null)
            throw new ArgumentNullException("execute");

        _execute = execute;
        _canExecute = canExecute;           
    }

    #endregion // Constructors

    #region ICommand Members

    [DebuggerStepThrough]
    public bool CanExecute(object parameter)
    {
        return _canExecute == null ? true : _canExecute(parameter);
    }

    public event EventHandler CanExecuteChanged
    {
        add { CommandManager.RequerySuggested += value; }
        remove { CommandManager.RequerySuggested -= value; }
    }

    public void Execute(object parameter)
    {
        _execute(parameter);
    }

    #endregion // ICommand Members
}

Answer 1

This problem is reported several times a week on StackOverflow. The problem is that each new lambda created inside the loop shares the same "action" variable. The lambdas do not capture the value, they capture the variable. That is, when you say

List<Action> list = new List<Action>();
foreach(int x in Range(0, 10))
    list.Add( ()=>{Console.WriteLine(x);} );
list[0]();

that of course prints "10" because that's the value of x now. The action is "write the current value of x", not "write the value that x was back when the delegate was created."

To get around this problem make a new variable:

List<Action> list = new List<Action>();
foreach(int x in Range(0, 10))
{
    int y = x;
    list.Add( ()=>{Console.WriteLine(y);} );
}
list[0]();

Since this problem is so common we are considering changing the next version of C# so that a new variable gets created every time through the foreach loop.

See http://ericlippert.com/2009/11/12/closing-over-the-loop-variable-considered-harmful-part-one/ for more details.

UPDATE: From the comments:

Every ICommand has the same methodinfo:

{ Method = {Void <CreateCommands>b__0(System.Object)}}

Yes, of course it does. The method is the same every time. I think you are misunderstanding what a delegate creation is. Look at it this way. Suppose you said:

var firstList = new List<Func<int>>() 
{ 
  ()=>10, ()=>20 
};

OK, we have a list of functions that return ints. The first one returns 10, the second one returns 20.

This is just the same as:

static int ReturnTen() { return 10; }
static int ReturnTwenty() { return 20; }
...
var firstList = new List<Func<int>>() 
{ ReturnTen, ReturnTwenty };

Make sense so far? Now we add your foreach loop:

var secondList = new List<Func<int>>();
foreach(var func in firstList)
    secondList.Add(()=>func());

OK, what does that mean? That means exactly the same thing as:

class Closure
{
    public Func<int> func;
    public int DoTheThing() { return this.func(); }
}
...
var secondList = new List<Func<int>>();
Closure closure = new Closure();
foreach(var func in firstList)
{
    closure.func = func;
    secondList.Add(closure.DoTheThing);
}

Now is it clear what is going on here? Every time through the loop you do not create a new closure and you certainly do not create a new method. The delegate you create always points to the same method, and always to the same closure.

Now, if instead you wrote

foreach(var loopFunc in firstList)
{
    var func = loopFunc;
    secondList.Add(func);
}

then the code we would generate would be

foreach(var loopFunc in firstList)
{
    var closure = new Closure();
    closure.func = loopFunc;
    secondList.Add(closure.DoTheThing);
}

Now each new function in the list has the same methodinfo -- it is still DoTheThing -- but a different closure.

Now does it make sense why you are seeing your result?

You might want to also read:

What is the lifetime of a delegate created by a lambda in C#?

ANOTHER UPDATE: From the edited question:

What I tried as per suggestions, but did not help:

    foreach (var action in actionArray)         
    {
         Action<object> executeHandler = o => { action(); };
         commands.Add(new RelayCommand(executeHandler));         } 
    }

Of course that did not help. That has exactly the same problem as before. The problem is that the lambda is closed over the single variable 'action' and not over each value of action. Moving around where the lambda is created obviously does not solve that problem. What you want to do is create a new variable. Your second solution does so by allocating a new variable by making a field of reference type. You don't need to do that explicitly; as I mentioned above the compiler will do so for you if you make a new variable interior to the loop body.

The correct and short way to fix the problem is

    foreach (var action in actionArray)         
    {
         Action<object> copy = action;
         commands.Add(new RelayCommand(x=>{copy();}));
    }

That way you make a new variable every time through the loop and each lambda in the loop therefore closes over a different variable.

Each delegate will have the same methodinfo but a different closure.

I'm not really sure about these closure and lambdas

You're doing higher-order functional programming in your program. You'd better learn about "these closures and lambdas" if you want to have any chance of doing so correctly. No time like the present.

Answer 2

I just made a working example of exactly what you're trying to do: http://ideone.com/hNcGx

    interface ICommand
    {
        void Print();
    }

    class CommandA : ICommand
    {
        public void Print() { Console.WriteLine("A"); }
    }

    class CommandB : ICommand
    {
        public void Print() { Console.WriteLine("B"); }
    }

    public static void Main()
    {
        var actions = new List<Action>();
        foreach (var command in new ICommand[]{
                    new CommandA(), new CommandB(), new CommandB()})
        {
            var commandcopy = command;
            actions.Add(() => commandcopy.Print());
        }

        foreach (var action in actions)
            action();
    }

Output:

A
B
B

Does this help any?

Answer 3

Make a local reference to action in the scope of the loop.

foreach (var action in actionArray)
{ 
   var myAction = action;
   // always add the same _execute member for each RelayCommand (_execute = {Method = {Void <CreateCommands>b__0(System.Object)}})
   commands.Add(new RelayCommand(o => { action(); }));
}

Answer 4

You're only ever using the first item in the actionArray array.

ie

commands.Add(new RelayCommand(o => { actionArray[0](); }));

You need to iterate through the collection of actions.

eg

public static ICommand[] CreateCommands(IEnumerable<Action> actions)
{
  commands = actions.Select(o => new RelayCommand(o)).ToArray();
}

Code is freehand so might be some typos but should point you in the right idea.