19
using System;
using System.Collections.Generic;
using System.Text;
using System.Threading.Tasks;

namespace ConsoleApplication1
{
    class Program
    {
        static void Main(string[] args)
        {
            MyClass myClass = new MyClass();
            myClass.StartTasks();
        }
    }
    class MyClass
    {
        int[] arr;
        public void StartTasks()
        {
            arr = new int[2];
            arr[0] = 100;
            arr[1] = 101;

            for (int i = 0; i < 2; i++)
            {
                Task.Factory.StartNew(() => WorkerMethod(arr[i])); // IndexOutOfRangeException: i==2!!!
            }
        }

        void WorkerMethod(int i)
        {
        }
    }
}

It seems that i++ gets executed one more time before the loop iteration is finished. Why do I get the IndexOutOfRangeException?

Bobby
  • 11,419
  • 5
  • 44
  • 69
Eiver
  • 2,594
  • 2
  • 22
  • 36
  • +1: that's interesting! Great piece of code, copy paste and I could test for myself. – Marijn Jun 03 '11 at 10:56
  • Apparently Microsoft also noticed, that this behavior is silly and they fixed it. In C# 5.0 the above code will work as expected :-D – Eiver Jun 19 '12 at 17:23
  • So happy someone seriously had this problem before.... just thought I'm totally stupid having a for loop going out of range lol. Upvoted. Really helpful question. – C4d May 06 '17 at 19:36

2 Answers2

20

You are closing over loop variable. When it's time for WorkerMethod to get called, i can have the value of two, not the value of 0 or 1.

When you use closures it's important to understand that you are not using the value that the variable has at the moment, you use the variable itself. So if you create lambdas in loop like so:

for(int i = 0; i < 2; i++) {
    actions[i] = () => { Console.WriteLine(i) };
}

and later execute the actions, they all will print "2", because that's what the value of i is at the moment.

Introducing a local variable inside the loop will solve your problem:

for (int i = 0; i < 2; i++)
{
    int index = i;
    Task.Factory.StartNew(() => WorkerMethod(arr[index])); 
}

<Resharper plug> That's one more reason to try Resharper - it gives a lot of warnings that help you catch the bugs like this one early. "Closing over a loop variable" is amongst them </Resharper plug>

Dyppl
  • 12,161
  • 9
  • 47
  • 68
  • 2
    +1 this answer is more clear on what's actually happening (closure(s) maybe/are evaluated after the loop finishes), than stating _"...Because tasks can execute concurrently the value of the loop variable may be different to the value it had when you started the task"_ – bottlenecked Jun 03 '11 at 11:32
  • @bottlenecked: well, in this case it still has to do with concurrency because `StartNew` actually starts the task right there, so it's *possible* that closures will evaluate in time. If you stuff `Sleep(1000)` inside the loop you should probably be alright. But I still think it's a problem of misunderstanding closures rather than concurrency – Dyppl Jun 03 '11 at 11:38
  • +1; not in the least for the shameless plug: R# does warn about "Access to modified closure". Note that it warns in case of `for` and `foreach` loops. – Marijn Jun 03 '11 at 13:00
  • After reading tons of MSDN documentation. It seems, that its not a bug - its a feature. Entire lambda expression is executed after, the loop exits, which is confusing and can produce results like above. I would like to avoid using lambda at all, but MSDN uses it everywhere in the samples now. Is there another way to start a thread with arbitrary input (say an int, string and char) without having to create a separate class and pass it as object? I want to avoid run-time type checking whenever possible. – Eiver Jun 05 '11 at 07:47
  • @user782534: it's not a problem of lambda expressions as a whole, it's a problem of using closures. Closures allow you to use variables from the context in which you declare you lambda expression. It's often extremely handy, you just have to be careful. Lambda expressions and closures are there exactly for that reason: because all other ways require you to write a lot of plumbing like creating a class etc. Rather than avoiding lambdas and closures, make it your friend by making sure you understand it all thoroughly. http://csharpindepth.com/Articles/Chapter5/Closures.aspx is a good article. – Dyppl Jun 05 '11 at 08:04
19

The reason is that you are using a loop variable inside a parallel task. Because tasks can execute concurrently the value of the loop variable may be different to the value it had when you started the task.

You started the task inside the loop. By the time the task comes to querying the loop variable the loop has ended becuase the variable i is now beyond the stop point.

That is:

  • i = 2 and the loop exits.
  • The task uses variable i (which is now 2)

You should use Parallel.For to perform a loop body in parallel. Here is an example of how to use Parallel.For

Alternativly, if you want to maintain you current strucuture, you can make a copy of i into a loop local variable and the loop local copy will maintain its value into the parallel task.

e.g.

for (int i = 0; i < 2; i++)
{
  int localIndex = i;
  Task.Factory.StartNew(() => WorkerMethod(arr[localIndex])); 
}
Colin Mackay
  • 18,736
  • 7
  • 61
  • 88
  • Here's some more [required reading](http://blogs.msdn.com/b/ericlippert/archive/2009/11/12/closing-over-the-loop-variable-considered-harmful.aspx). – Jeff Mercado Jun 03 '11 at 10:55
  • And a nice big ParallelProgramsinNET4_CodingGuidelines.pdf from MS; http://download.microsoft.com/download/B/C/F/BCFD4868-1354-45E3-B71B-B851CD78733D/ParallelProgramsinNET4_CodingGuidelines.pdf – Alex K. Jun 03 '11 at 10:58
  • Small follow up question: why doesn't a `foreach` throw? Pure chance? – Marijn Jun 03 '11 at 11:01
  • @Marijn: see my comment to your answer on `foreach` not throwing exceptions – Dyppl Jun 03 '11 at 11:08
  • Sounds like a classic Closure issue – Antony Jun 03 '11 at 13:20