4

I am trying to understand why this program doesn't work

Expected output: numbers 0-19 in random order What I get when I run: some numbers repeat, sometimes 20 is printed.

Please help. I tried with lock(obj) in DoSomething() but it didn't help.

Program

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading;

namespace ConsoleApplication2
{
    public delegate void callbackDelegate(int x);
    class Program
    {
        void processCallback(int x)
        {
            Console.WriteLine("IN callback: The value I got is " + x);
        }

        static void Main(string[] args)
        {
            Program p = new Program();
            p.processinThreads();
            Console.ReadKey();
        }

        public void DoSomething(int x, callbackDelegate callback)
        {
            Thread.Sleep(1000);
            //Console.WriteLine("I just woke up now " + x);
            callback(x);
        }

        public void processinThreads()
        {
            for (int i = 0; i < 20; i++)
            {
                Thread t = 
new Thread(new ThreadStart(()=>DoSomething(i, processCallback)));
                t.Start();
            }
        }
    }
}
radbyx
  • 9,352
  • 21
  • 84
  • 127
satya
  • 101
  • 1
  • 8
  • possible duplicate of [C# Captured Variable In Loop](http://stackoverflow.com/questions/271440/c-sharp-captured-variable-in-loop) – Matthias Feb 23 '12 at 12:03
  • right, I couldn't get this when I searched for this problem. Well I don't know this 'closure over lambda problem' :) – satya Feb 23 '12 at 12:08
  • Avoid manual thread creation. [Here](http://stackoverflow.com/questions/9397729/how-do-you-get-list-of-running-threads-in-c/9405662#9405662) is a detailed explanation with benchmarks why – Anastasiosyal Feb 23 '12 at 13:49

3 Answers3

9
public void processinThreads()
{
    for (int i = 0; i < 20; i++)
    {
        int local = i;
        Thread t = new Thread(new ThreadStart(()=>DoSomething(local, processCallback)));
        t.Start();
    }
}

Your problem is related to closure over lambda.

Jakub Konecki
  • 45,581
  • 7
  • 87
  • 126
  • 1
    Check out this article: [Closing over the loop variable considered harmful](http://blogs.msdn.com/b/ericlippert/archive/2009/11/12/closing-over-the-loop-variable-considered-harmful.aspx). It will be fixed in C# 5.0. – Paolo Moretti Feb 23 '12 at 12:19
7

You should just use the TPL, its a lot easier and recommended over manual thread management:

Parallel.For(0, 20, x => {
    Thread.Sleep(1000);
    Console.WriteLine("IN callback: The value I got is " + x);
});

This will also block until the loop finishes, if you don't want that you can use the TPL Task, but I would definitely recommend avoiding threads.

Paul Tyng
  • 7,924
  • 1
  • 33
  • 57
  • Moreover, this is likely to be more efficient: Spawning another thread is pretty expensive. The TPL uses an (automagically computed) optimum number of threads and re-uses them. – Matthias Meid Feb 23 '12 at 12:11
  • @user676571 I am planning to use TPL with Async pattern applied. – satya Feb 23 '12 at 13:18
1

As Jakub already told you, you need to copy i into another local variable local. In your code, the Delegates have direct access to i itself, not a copy of i, so they print out the very current value of i, which may be greater than when you started the thread. This is called closure.

Matthias Meid
  • 12,455
  • 7
  • 45
  • 79