3

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.stack.imgur.com/4MHOP.png

Screenshot of code https://i.stack.imgur.com/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++;
        }
        }
    }
Ali
  • 1,801
  • 6
  • 43
  • 58
  • 1
    You can not update `mylabel[tcount].Text` in a thread other than main UI thread AFAIK. – Jahan Zinedine Jan 17 '12 at 12:51
  • 1
    I think you are having a cross-threading issue here, you shouldn't update any UI elements from a seperate thread. [Check this out](http://stackoverflow.com/questions/661561/how-to-update-gui-from-another-thread-in-c) – musefan Jan 17 '12 at 12:52
  • 2
    I suggest you read [Joe Albahari's ebook](http://www.albahari.com/threading/) - it's a good intro – Nick Butler Jan 17 '12 at 12:54
  • @Jani and musefan Yes I was having this problem before but then I used Invoke so now no such problem. – Ali Jan 18 '12 at 04:13
  • @Nicholas Butler Let me read that book. – Ali Jan 18 '12 at 04:14

3 Answers3

9

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.

BenMorel
  • 34,448
  • 50
  • 182
  • 322
TomTom
  • 61,059
  • 10
  • 88
  • 148
  • And finally you never parameterize the threads so they all work on the same varaibles.There is only one non thread save (no lock, no volatile) variable named tCount - bang. But I need one variable like tcount as global so I can keep count of threads. How am I gonna achieve it without tcount? (How do I give line breaks when replying?) – Ali Jan 18 '12 at 04:20
  • YOu ahve tCount, but you need to LOCK access to it or use atomic operations AND (!) the variable must be VOLATILE - otherwise the threads may not read it assuming they know the value and optimize th ememory access out. – TomTom Jan 18 '12 at 05:50
3

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(); });
    }
}
Christoph Fink
  • 22,727
  • 9
  • 68
  • 113
  • Your post taught me something new about not invoking whole worker method. I will keep in mind now. And using your code, now I am not getting any error. But still it doesn't look like multithreaded and seems I still need to do some tweaking to it. – Ali Jan 18 '12 at 04:28
0

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.

ElGaucho
  • 408
  • 2
  • 14