1

Is it valid to refer to i in the delegate statement?

targets[i].PingReply = e.Reply;

Will it refer to the same array element defined in

pingSender.SendAsync( targets[i].IPAddress, targets[i].Timeout);

or is something different going for the value of i when the delegate fires? I ask because I'm getting an index out of bounds with i=3 in the PingCompleted and I'm not sure why.

public void Ping(PingTest[] targets)
{
    var finished = new CountdownEvent(targets.Count());
    for (int i = 0; i < targets.Count(); i++)
    {
        finished.AddCount();
        var pingSender = new Ping();
        pingSender.PingCompleted += (sender, e) =>
                                        {
                                            targets[i].PingReply = e.Reply;
                                            finished.Signal();
                                        };
        pingSender.SendAsync(targets[i].IPAddress, targets[i].Timeout);
    }
    finished.Signal();
    finished.Wait();
}

Here is the call...

var pingTests = new PingTest[]
                    {
                        new PingTest("Router", new IPAddress(new byte[] {192, 168, 1, 8}), 2),
                        new PingTest("Exchange", new IPAddress(new byte[] {192, 168, 1, 78}), 3),
                        new PingTest("SQL", new IPAddress(new byte[] {192, 168, 1, 99}), 3)
                    };
netwrkService.Ping(pingTests);
strattonn
  • 1,790
  • 2
  • 22
  • 41

3 Answers3

5

What do you expect this program fragment to do?

int i = 0;
Func<int> f = ()=>i;
i = 3;
Console.WriteLine(f());

Try it. Did it do what you think it should have done?


Anonymous functions are closed over variables, not the value that the variable had in the past. When you're invoking your lambda, the loop variable no longer has the value that it did when you created the delegate.

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

Eric Lippert
  • 647,829
  • 179
  • 1,238
  • 2,067
  • Ah, so I was wrong in my other comment - Value types don't get captured as copies, they get boxed/passed into the closure as ref types? – JerKimball Mar 23 '13 at 02:58
  • 1
    @JerKimball Neither, strictly speaking. See [this answer](http://stackoverflow.com/a/15302848/1159478) for an approximation of how the C# compiler will implement closures. It doesn't take a reference to the local, nor does it box it (in the traditional C# sense of the word). What it does encapsulate that variable as an instance field of a reference type. – Servy Mar 23 '13 at 03:23
  • Cheers, @Servy - I've looked over reflected closures before, but it never really jumped out at me before. Obliged! – JerKimball Mar 23 '13 at 03:25
  • @JerKimball: It is not possible in the CLR type system to store a ref to a variable in a field. And whether a variable is of value type or not is irrelevant to the question of what its capturing semantics are; int variables are captured just the same as string variables. – Eric Lippert Mar 23 '13 at 06:33
  • I follow now, thanks Eric - I'm not exactly sure why, but for some reason I thought the generated code was more along the lines of "Oh, I see we need this captured variable; thanks to the compiler, there's a space for it in this closure class, I'll just `closure.Capture = liveVariable`" (in which case, Value vs Ref does factor). As I said to @Servy, I've read through reflectored closures before, but for some reason the real structure never registered. – JerKimball Mar 23 '13 at 06:43
  • At first I would have said 0 but of course it's 3 and that explains my dilemma. Thanks! – strattonn Mar 25 '13 at 05:24
3

In C#, closures close over variables, not over values. In your for loop, there is one and only one variable i, and when each PingCompleted handler reads the value of i, it gets the current value of that single variable, not the value of i back when the handler was hooked up. So if the handlers execute after the for loop finishes, then i will equal 3 — not what you want!

To fix the problem, copy the value of i into another variable that's declared inside the loop, and then change the handler to use that new variable:

for (int i = 0; i < targets.Count(); i++)
{
    ...
    int j = i;
    pingSender.PingCompleted += (sender, e) =>
                                    {
                                        targets[j].PingReply = e.Reply; // <== j, not i
                                        finished.Signal();
                                    };

When you declare a variable inside a loop, a new instance of the variable is logically created every iteration. Thus, the PingCompleted handlers now refer to different instances of j, each of which holds the correct index for that handler.

Eric Lippert
  • 647,829
  • 179
  • 1,238
  • 2,067
Michael Liu
  • 52,147
  • 13
  • 117
  • 150
0

Since the loop finishes before the PingCompleted gets called, i is incremented to 3 when the PingCompleted gets called for the first time. Then it is called two more times (because that is what your for loop initialed with i still at 3 since the for loop is already finished and not incrementing i anymore.

Do you understand why i is always out of bounds? Your index i goes from 0 to 1 on the first iteration, 1 to 2 on the second iteration and 2 to 3 on the last (3 being set at the end). Once all iterations are complete your first call to PingCompleted is called with the index already at 3.

PmanAce
  • 4,000
  • 2
  • 24
  • 29