CODEWITHSUNDEEP
c#multithreading
Ali
Ali

Reputation: 1841

Simple multithreaded program in C# not working

I am trying to write a simple multithreaded program in C#. It has a button pressing which creates a new label on form, and then a for loop runs displaying loop value in label. So if you press button 3 times, it will create 3 threads with 3 labels on form with loop.

When I press the button once, it works fine. But when I press it more than once to create more labels, it runs into following problems:

  1. As soon as button is pressed more than once, it stops the loop in previous thread and runs loop of new thread. If it is multithreaded then it should not stop first loop.

  2. When loop of second label is finished, it gives following error

Object reference not set to an instance of an object

Here is my complete code. The line which throws error is at the end "mylabel[tcount].Text = i.ToString();".

Screenshot of program: https://i.sstatic.net/4MHOP.png

Screenshot of code https://i.sstatic.net/S0tQ0.png

namespace WindowsFormsApplication2{
    public partial class Form1 : Form{
        public Form1(){
            InitializeComponent();
        }

        private int tcount = 0;
        private int y_point = 0;

        Thread[] threads = new Thread[5];
        Label[] mylabel = new Label[5];

        private void button1_Click(object sender, EventArgs e){
            threads[tcount] = new Thread(new ThreadStart(work));
            threads[tcount].Start();
        }

        private void work(){
            if (this.InvokeRequired){
                this.Invoke(new MethodInvoker(delegate{
                    mylabel[tcount] = new Label();
                    mylabel[tcount].Text = "label" + tcount;
                    mylabel[tcount].Location = new System.Drawing.Point(0, y_point + 15);
                    y_point += 25;

                    this.Controls.Add(mylabel[tcount]);
                    for (int i = 0; i < 10000; i++){
                        mylabel[tcount].Text = i.ToString();
                        Application.DoEvents();
                    }
                }));
            }
            tcount++;
        }
        }
    }

Upvotes: 3

Views: 1573

Answers (3)

TomTom
TomTom

Reputation: 62157

If it is multithreaded then it should not stop first loop.

But it is not multithreaded.

this.Invoke(new MethodInvoker(delegate{

This switches via invoker the context back to the UI Thread, so while you open a lot of threads in the background, you basically then put all the processing back into one main thread.

This:

Application.DoEvents();

Then gives other queued work a chance. Still only on the UI thread.

And finally you never parametrize the threads so they all work on the same variables. There is only one non thread save (no lock, no volatile) variable named tCount - bang.

Basically you demonstrate:

  • Your problem is not solvable multi threaded - any UI element manipulation HAS to happen on the UI thread (which is why you invoke) and as this is all you do you basically can not multithread.
  • You lack a basic understanding on how UI programs work with threads and the message pump.
  • You lack a basic understanding on variable scoing and access patterns between threads.

Back to reading documentation I would say.

Upvotes: 9

Christoph Fink
Christoph Fink

Reputation: 23113

The problem is the scope of tcount, as all threads acces the same instance of it, so as soon as the second thread starts the first thread also wirtes into the second label.

Also you invoke your whole worker method which will let it run in the UI-Thread again -> not actually multithreaded...

Your worker method should look something like this:

private void work()
{
    int tIndex = tCount; //store the index of this thread
    tcount++;
    mylabel[tIndex] = new Label();
    mylabel[tIndex].Text = "label" + tcount;
    mylabel[tIndex].Location = new System.Drawing.Point(0, y_point + 15);
    y_point += 25;

    Invoke((MethodInvoker)delegate() { this.Controls.Add(mylabel[tIndex]); });

    for (int i = 0; i < 10000; i++)
    {
        //doWork
        Invoke((MethodInvoker)delegate() { mylabel[tIndex].Text = i.ToString(); });
    }
}

Upvotes: 3

ElGaucho
ElGaucho

Reputation: 408

Jep, you need to copy tcount to a local variable. As soon as you hit the button twice while a thread has not yet terminated, it is manipulating the second one.

Upvotes: 0

Related Questions