CODEWITHSUNDEEP
c#multithreading
YardimaIhtiyaciOlan
YardimaIhtiyaciOlan

Reputation: 125

Multithreading error

I have a form and there is no control.I am trying get the controls from Database so my project is slow i thinks i can use threading but I get a error.

    void Form_Load(object sender,EventArgs e)
    {

    SqlDataAdapter adap=new SqlDataAdapter("Select * from MyControls");
    DataTable dt=new DataTable();

    adap.Fiil(dt);

     foreach(DataRow dr in dt.Rows) 
     {
      ThreadStart ts=delegate{ Sample1(dr) };
Thread th=new Thread(ts);
th.start();

     }

    }

    public void Sample1(DataRow dr)
    {
    this.Invoke(new AddControlsDelegate(AddControls),new object[] {dr } );
    }
    public void AddControls(DataRow dr)
    {
    TextBox tx=new TextBox();
    tx.Name=dr["Id"].ToString();
    this.Controls.Add(tx);

    }

public delegate void AddControlsDelegate(DataRow dr);

I am tryin with this code .But it dont work.It adding same control twice time,3 time,4 time

Where is the my wrong? thanks

Upvotes: 0

Views: 121

Answers (2)

Brian Gideon
Brian Gideon

Reputation: 48949

Aside from the fact that you have created a closure over a loop variable there a couple of pretty major problems with your general approach.

First, creating one thread for each row in the DataTable is not a good idea. The thing is creating and starting threads is expense and consumes a lot of resources. You need to choose wisely how and when you create threads. For example, having 10,000 threads running (assuming there are 10,000 corresponding rows in the DataTable) is not going to work out very well.

But, even if you moved to a more sane approach using the ThreadPool via QueueUserWorkItem, asynchronous delegates, or by starting a new Task there is yet another big problem. The operation you intended to have happen on another thread winds up being marshaled back onto the UI thread. The use of threads in this case serves absolutely no purpose. In fact, it makes everything worse since the thread does no useful work at all. And it requires an expensive marshaling operation via Control.Invoke to get nothing done. In other words, your UI thread tells a worker thread to do something and the worker thread turns around and says "I don't want to, you do it instead."

A better approach would be to execute the SQL command and populate the DataTable on another thread. Once that is complete you can then add all of those TextBox controls to the form from the UI thread. Actually, doing anything with a UI element must done from the UI thread so you really do not have many acceptable options here.

Upvotes: 0

BrokenGlass
BrokenGlass

Reputation: 160852

You are closing over the loop variable:

 foreach(DataRow dr in dt.Rows) 
 {
   ThreadStart ts=delegate{ Sample1(dr) };
   ...
 }

You are creating a closure of the loop variable, not its value at the time - since the thread will be only started a little time later, the loop has completed and each thread will use the value of the loop variable at that time, which will be the last row.

Instead create a local variable within the loop and use it in your delegate:

 foreach(DataRow dr in dt.Rows) 
 {
    DataRow currentRow = dr;
    ThreadStart ts=delegate{ Sample1(currentRow) };
    Thread th=new Thread(ts);
    th.start();
 }

Also see "Closing over the loop variable considered harmful"

Upvotes: 2

Related Questions