Why does my delegate only use the last item from my foreach loop?

Question

Scenario: I am building a scheduling system and each timer event I wanted to run a custom method instead of the usual Timer.Elapsed event.

So I wrote something like this.

foreach (ScheduleElement schedule in schedules) {
    TimeSpan timeToRun = CalculateTime(schedule);
    schedule.Timer = new Timer(timeToRun.TotalMilliseconds);
    schedule.Timer.Elapsed += delegate { Refresh_Timer(schedule); };
    schedule.Timer.AutoReset = true;
    schedule.Timer.Enabled = true;
}

Ok so simple enough that actually did create my timers. However, I wanted each elapsed event to run using the schedule element that it passed in. My question is, why does the Elapsed event only pass in the last ScheduleElement in the for loop for every single Timer.Elapsed event.

Now I know what fixes it, I am just not sure why. If I roll back to the original Timer.Elapsed event and extend the Timer class with my own class I can work around it. Like so.

The Fix:

foreach (ScheduleElement schedule in schedules) {
    TimeSpan timeToRun = CalculateTime(schedule);
    schedule.Timer = new TimerEx(timeToRun.TotalMilliseconds);
    schedule.Timer.Elapsed +=new System.Timers.ElapsedEventHandler(Refresh_Timer);
    schedule.Timer.Tag = schedule;
    schedule.Timer.AutoReset = true;
    schedule.Timer.Enabled = true;
}

I then cast the object sender back into its original object, and thieve the Tag property off of it which gives me my correct schedule for each unique timer.

So again, why does using a delegate { } only pass in the last ScheduleElement in the foreach loop for all Timers?

EDIT 1

The Timer class

public TimerEx : Timer {

    public TimerEx(double interval) : base(interval) { }

    private Object _Tag;

    public Object Tag {
        get { return _Tag; }
        set { _Tag = value; }
    }
}

Research "Closing over the loop"

Matthew
– Matthew

2013-07-16 23:44:08 +00:00
Commented Jul 16, 2013 at 23:44 — Matthew
– Matthew, Commented Jul 16, 2013 at 23:44
Check this: stackoverflow.com/questions/271440/…

fcuesta
– fcuesta

2013-07-16 23:48:27 +00:00
Commented Jul 16, 2013 at 23:48 — fcuesta
– fcuesta, Commented Jul 16, 2013 at 23:48

Reed Copsey · Accepted Answer · 2013-07-16 23:44:34Z

9

This is because you're using closure in your delegate, and it closes over the same variable, which is shared for each iteration of the foreach loop.

For details, see Eric Lippert's article Closing over the loop variable considered harmful.

In this case, you can easily fix it with a temporary:

foreach (ScheduleElement schedule in schedules) {
    TimeSpan timeToRun = CalculateTime(schedule);
    schedule.Timer = new Timer(timeToRun.TotalMilliseconds);

    // Make a temporary variable in the proper scope, and close over it instead
    var temp = schedule;
    schedule.Timer.Elapsed += delegate { Refresh_Timer(temp); };

Note that C# 5 changes this behavior for foreach loops. If you compile this with the latest compilers, the issue no longer exists.

answered Jul 16, 2013 at 23:44

Reed Copsey

567k80 gold badges1.2k silver badges1.4k bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

Steven Combs Over a year ago

Where can I find out more information on using the latest compiler? I am not really sure what my VS is using, I have 2010.

Reed Copsey Over a year ago

@meanbunny You'd need to use VS 2012 or later to get the new behavior. It was changed in C# 5, which was released with VS 2012.

Steven Combs Over a year ago

Ok thanks, great answer and I really appreciate it! Got 4 minutes left till I can accept.

Mathieu Guindon Over a year ago

Maybe worth noting that ReSharper underlines this as a "Access to modified closure" warning, and suggests the introduction of the temporary / iteration-scope variable.

Collectives™ on Stack Overflow

Why does my delegate only use the last item from my foreach loop?

1 Answer 1

4 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

4 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related