C# Tasks not working as expected. Bizarre error

Question

I've been toying with parallelism and I'm having some trouble understanding what's going on in my program.

I'm trying to replicate some of the functionality of the XNA framework. I'm using a component-style setup and one way I thought of making my program a little more efficient was to call the Update method of each component in a separate Task. However, I'm obviously doing something horribly wrong.

The code I'm using in my loop for an update call is:

public void Update(GameTime gameTime)
{
    Task[] tasks = new Task[engineComponents.Count];

    for (int i = 0; i < tasks.Length; i++)
    {
        tasks[i] = new Task(() => engineComponents[i].Update(gameTime));
        tasks[i].Start();
    }

    Task.WaitAll(tasks);
}

This throws a weird error:

An unhandled exception of type 'System.AggregateException' occurred in mscorlib.dll

The inner exception talks about an index being out of range.

If I change

Task[] tasks = new Task[engineComponents.Count];

to

Task[] tasks = new Task[engineComponents.Count - 1];

then this seems to work (or at least the program executes without an exception), but there isn't enough room in the array for all of the components. Nonetheless, all of the components are updated, despite there not being enough room in the tasks array to hold them all.

However, the gameTime object that is passed as a parameter goes somewhat insane when the game is running. I've found it hard to pin-point the problem, but I have two components that both simply move a circle's x-position using

x += (float)(gameTime.ElapsedGameTime.TotalSeconds * 10);

However, when using Tasks, their x-positions very quickly become disparate from one another, when they should in fact be the same. Each engineComponent.Update(gameTime) is called once per-update cycle, and the same gameTime object is passed.

When using tasks[i].RunSynchronously(); in place of tasks[i].Start();, the program runs exactly as expected.

I understand that using Tasks in this manner may not be a particularly efficient programming practice, so my question is one of curiosity: why isn't the above code working as I would expect? I know I'm missing something obvious, but I've been unable to track down what specifically is wrong with this implementation.

Apologies for the long question, and thanks for reading ;)

Answer 1

The problem is that your lambda expression is capturing i - not the value of i, but the variable itself.

That means that by the time your task executes, the loop may well be on the next iteration (or even later). So some of your components may be updated more than once, some may not be updated at all, and the final tasks are likely to execute when i is outside the range of engineComponents, hence the exception. For more details, see Eric Lippert's blog posts:

• Closing over the loop variable considered harmful
• Closing over the loop variable, part two

Three options to fix this:

• Take a copy of the variable inside the loop. Each variable declared inside the loop will be captured separately:

  for (int i = 0; i < tasks.Length; i++)
  {
      int copyOfI = i;
      tasks[i] = new Task(() => engineComponents[copyOfI].Update(gameTime));
      tasks[i].Start();
  }
  

• Capture engineComponents[i] in a separate variable instead:

  for (int i = 0; i < tasks.Length; i++)
  {
      var component = engineComponents[i];
      tasks[i] = new Task(() => component.Update(gameTime));
      tasks[i].Start();
  }
  

• If you're using C# 5, using a foreach loop will do what you want:

  var tasks = new List<Task>();
  foreach (var component in engineComponents)
  {
      Task task = new Task(() => component.Update(gameTime));
      tasks.Add(task);
      task.Start();
  }
  Task.WaitAll(tasks.ToArray());
  

Note that the last solution will not work with the C# 4 compiler, as the behaviour of the foreach iteration variable was for it to be a single variable, just like i. You don't need to target .NET 4.5 or higher for it to work, but you do need to use a C# 5 compiler.

Another option is to not use tasks explicitly at all - use Parallel.ForEach instead:

// This replaces your entire method body
Parallel.ForEach(engineComponents, component => component.Update(gameTime));

Much simpler!

Answer 2

Try the following:

for (int i = 0; i < tasks.Length; i++)
{
    var innerI = i;
    tasks[i] = new Task(() => engineComponents[innerI].Update(gameTime));
    tasks[i].Start();
}

You need a new variable for each task, which will be captured by linq expression and will hold index of your job part. Now all your tasks uses i variable and perform work on the latest element.

Answer 3

Your loop variable is captured in a closure. Here is an article by Eric Lippert for a more detailed explanation. You can easily solve the issue by declaring an inner variable inside the loop:

public void Update(GameTime gameTime)
{
    Task[] tasks = new Task[engineComponents.Count];

    for (int i = 0; i < tasks.Length; i++)
    {
        int inner = i; // Declare another temp variable
        tasks[i] = new Task(() => engineComponents[inner].Update(gameTime));
        tasks[i].Start();
    }

    Task.WaitAll(tasks);
}