locked
Threading Problem , lock problem RRS feed

  • Question

  • clarification in another way
    _________________ Starts ___________

    what i want to do is:

    i have 100 item in a collection named arrItemsFrom.

    i want 15 thread to read the item and add its value in another collection named arrItemsTo.

    note, each thread should wait for 5 second ( sleep ) before adding it to another place.

    console sample is okay,

     ArrayList arrItemsFrom = new ArrayList(); 
     ArrayList arrItemsTo = new ArrayList(); 
     
     for (int i = 0; i < 100; i++) 
       arrItemsFrom.Add("Data:" + i.ToString()); 

    ________________ Ends________________




    Hello,

    I am learning threading, here is what i want to do,

    There is a listbox which is filled with un-processed data , there is another listbox where processed data is written.
    The processing will takes around 3 second.

    So, if i have 1 thread then it will take 3 x 100 second to process 100 data,
    if i have 5 thread working then it will take ( 3 x 100 )/5 time to process 100 data,

    What I am doing ?
    ________________

    1. Create a thread , increase counter , load data from unprocessed list box using the counter as index of listbox
    2. Process the data , ( takes 3 seconds, using Thread.Sleep for that.. )
    3. Write the data extracted from un-processed listbox to processed listbox

    The Problem
    ________________

    When I run 5 thread using for loop, the extraction happen only after 5th index.
    Thread are taking same data multiple times.

    I also thinks the code is very badly written.




    My Remedy
    _____________

    When i keep the doWork() function inside the lock then it works , but only one thread will run at once, so even there is 5 thread , it will take 3 x 100 second.

    When i wait some time inside the for loop when creating thread , it also seems working , but it is just because the multiple thread were run in different  timing so its not the solution.


    GUI
    _____________________________

    I have lstUnProcessesData and lstProcessesData ( listboxes)
    btnStart Start Button.
    btnStop Stop Button.



       Please  check my code where i am doing wrong,

    using System; 
    using System.Collections.Generic; 
    using System.ComponentModel; 
    using System.Data; 
    using System.Drawing; 
    using System.Text; 
    using System.Windows.Forms; 
    using System.Threading; 
    using System.Collections; 
     
    namespace ThreadLearningNew 
         
         
        public partial class Form1 : Form 
        { 
            int counter = -1; 
            int threadCounter = 0; 
            Hashtable hashThread = new Hashtable(); 
     
            public Form1() 
            { 
                InitializeComponent(); 
            } 
     
            private void Form1_Load(object sender, EventArgs e) 
            { 
                for (int i=0;i<=100;i++) 
                { 
                    lstUnProcessesData.Items.Add("Data:" + i.ToString()); 
                } 
            } 
     
     
            void processData() 
            { 
                while(counter<100) //loop when counter is less < lstUnProcessedData.Count 
                { 
                        lock (this
                        { 
                            counter++; 
                        } 
                        doWork();        
                } 
            } 
     
            void doWork() 
            { 
                
                Thread.Sleep(3000); //time consuming work.. 
                SetItemSafe(GetListBoxText(counter) + " , " + Thread.CurrentThread.Name ); //notify work done! 
            } 
     
            private void btnStart_Click(object sender, EventArgs e) 
            { 
                for (int i = 0; i < 5; i++) 
                { 
                    threadCounter++; 
                    Thread t = new Thread(processData); 
                    hashThread.Add(threadCounter, t); 
                    t.Name = threadCounter.ToString(); 
                    t.Start(); 
                    this.Text = "Total Thread:" + threadCounter.ToString(); 
                } 
     
            } 
     
            private delegate void Invoker(string parameter);   
            private void SetItemSafe(string data) 
            { 
                if ( lstProcessedData.InvokeRequired) 
                { 
                    lstProcessedData.BeginInvoke(new Invoker(SetItemSafe), data); 
                } 
                else 
                { 
                    lstProcessedData.Items.Add(data); 
                } 
            } 
     
     
            private delegate string GetListBoxTextInvoker(int index); 
            private string GetListBoxText(int index) 
            { 
                if (lstUnProcessesData.InvokeRequired) 
                { 
                    return (string)lstUnProcessesData.Invoke(new GetListBoxTextInvoker(GetListBoxText), index); 
                } 
                else 
                { 
                    return lstUnProcessesData.Items[index].ToString(); 
                } 
            } 
     
            private void btnStop_Click(object sender, EventArgs e) 
            { 
                for (int i = 1; i <= hashThread.Count; i++) 
                { 
                    Thread t = (Thread)hashThread[i]; 
                    t.Abort(); 
                    t.Join(); 
                } 
            } 
     
        } 





    Let's Program.. , Yam Sapkota
    • Edited by Yam Sapkota Sunday, January 4, 2009 4:34 PM
    Saturday, January 3, 2009 4:17 PM

Answers

  • thanks to alex and mexilus ,

    here is the solution found myself with the help of mexilus code, even though this is resolved, i am learning threading so new idea and more good code is always welcome,

    please post your ideas and suggestion..

    the code i post below can take any number of thread, even 300 , doesn't matter even there is just 100 data. check the processData() for fixed code.

    using System; 
    using System.Collections; 
    using System.Drawing; 
    using System.Threading; 
    using System.Windows.Forms; 
     
    namespace ThreadLearningNew 
        public partial class Form1 : Form 
        { 
            int counter = 0; 
            int threadCounter = 0; 
            Hashtable hashThread = new Hashtable(); 
             
     
            public Form1() 
            { 
                InitializeComponent(); 
            } 
     
            private void Form1_Load(object sender, EventArgs e) 
            { 
                for (int i = 0; i < 100; i++) 
                { 
                    lstUnProcessesData.Items.Add("Data:" + i.ToString("D2")); //to sort correctly in listbox.. 
                } 
            } 
     
            private void processData() 
            { 
               // while (counter < (lstUnProcessesData.Items.Count-1)) 
                while(getIndex()<(lstUnProcessesData.Items.Count-1)) //this is the improved line :) 
                { 
                    string data = GetListBoxText(counter); 
                    doWork(); 
                    SetItemSafe(data + " , " + Thread.CurrentThread.Name); //notify work done!     
                } 
            } 
     
            private int getIndex() 
            { 
                lock (this
                { 
                    return counter++; 
                } 
            } 
            
            void doWork() 
            { 
                Thread.Sleep(3000); //time consuming work..     
            } 
     
            private void btnStart_Click(object sender, EventArgs e) 
            { 
                lstProcessedData.Items.Clear(); 
     
                for (int i = 0; i < 23; i++) 
                { 
                    threadCounter++; 
                    Thread t = new Thread(processData); 
                    hashThread.Add(threadCounter, t); 
                    t.Name = threadCounter.ToString(); 
                    t.Start(); 
                    this.Text = "Total Thread:" + threadCounter.ToString(); 
                } 
            } 
     
            private delegate void Invoker(string parameter); 
            private void SetItemSafe(string data) 
            { 
                if (lstProcessedData.InvokeRequired) 
                { 
                    lstProcessedData.BeginInvoke(new Invoker(SetItemSafe), data); 
                } 
                else 
                { 
                     
                    lstProcessedData.Items.Add(data); 
                } 
            } 
     
            private delegate string GetListBoxTextInvoker(int index); 
            private string GetListBoxText(int index) 
            { 
                if (lstUnProcessesData.InvokeRequired) 
                { 
                    return (string)lstUnProcessesData.Invoke(new GetListBoxTextInvoker(GetListBoxText), index); 
                } 
                else 
                { 
                    return lstUnProcessesData.Items[index].ToString(); 
                } 
            } 
     
            private void btnStop_Click(object sender, EventArgs e) 
            { 
                for (int i = 1; i <= hashThread.Count; i++) 
                { 
                    Thread t = (Thread)hashThread[i]; 
                    t.Abort(); 
                    t.Join(); 
                } 
            } 
     
        } 
     


    Let's Program.. , Yam Sapkota
    • Marked as answer by Yam Sapkota Sunday, January 4, 2009 5:57 PM
    Sunday, January 4, 2009 5:55 PM

All replies

  •  The solution is to buy different  hardware. You need a dual processor to gain 2-fold improvement in speed. You need a quad to feel a 4-fold improvement and you need two quads to notice an 8-fold improvement in performance. In practice the improvement will not be 4-fold exactly but somewhat closer although still some time will be lsot on management.
    AlexB
    Saturday, January 3, 2009 4:42 PM
  • thanks alex,

    but speed is not my problem, the code is not working as desired, there must be error around lock(this) line, of doWork() ..
    Also check the screenshot, its also showing problem there.

    Let's Program.. , Yam Sapkota
    Saturday, January 3, 2009 5:01 PM
  • Yes, there is a bug there. As you mentioned, different threads are retrieving the same data from lstUnProcessesData.
     
    Consider this: While thread 1 is sleeping, other threads are incrementing counter. When thread 1 wakes up, and reads counter, it has changed.

    Also, this loop executes 101 times, not 100: for (int i = 0; i <= 100; i++).

    And the way processData is written will ultimately cause counter to be incremented to 101.
    Saturday, January 3, 2009 5:41 PM
  • On the other hand, your use of InvokeRequired is interesting. I learned something from you.

    Try this code, and examine the differences.

    using System;  
    using System.Collections;  
    using System.Drawing;  
    using System.Threading;  
    using System.Windows.Forms;  
     
    namespace ThreadLearningNew  
    {  
        public partial class Form1 : Form  
        {  
            int counter = 0;  
            int threadCounter = 0;  
            Hashtable hashThread = new Hashtable();    
     
            public Form1()  
            {  
                InitializeComponent();  
            }  
     
            private void Form1_Load(object sender, EventArgs e)  
            {  
                for (int i = 0; i < 100; i++)  
                {  
                    lstUnProcessesData.Items.Add("Data:" + i.ToString());  
                }  
            }  
     
            private void processData()  
            {  
                while (counter < (lstUnProcessesData.Items.Count - 2))  
                {  
                    string data = getData();  
                    doWork();  
                    SetItemSafe(data + " , " + Thread.CurrentThread.Name); //notify work done!    
                }  
            }              
     
            private string getData()  
            {  
               lock (this)  
               {  
                   return GetListBoxText(counter++);  
               }  
            }  
     
            void doWork()  
            {  
                Thread.Sleep(3000); //time consuming work..    
            }  
     
            private void btnStart_Click(object sender, EventArgs e)  
            {  
                lstProcessedData.Items.Clear();  
     
                for (int i = 0; i < 5; i++)  
                {  
                    threadCounter++;  
                    Thread t = new Thread(processData);  
                    hashThread.Add(threadCounter, t);  
                    t.Name = threadCounter.ToString();  
                    t.Start();  
                    this.Text = "Total Thread:" + threadCounter.ToString();  
                }    
            }  
     
            private delegate void Invoker(string parameter);  
            private void SetItemSafe(string data)  
            {  
                if (lstProcessedData.InvokeRequired)  
                {  
                    lstProcessedData.BeginInvoke(new Invoker(SetItemSafe), data);  
                }  
                else 
                {  
                    lstProcessedData.Items.Add(data);  
                }  
            }  
     
            private delegate string GetListBoxTextInvoker(int index);  
            private string GetListBoxText(int index)  
            {  
                if (lstUnProcessesData.InvokeRequired)  
                {  
                    return (string)lstUnProcessesData.Invoke(new GetListBoxTextInvoker(GetListBoxText), index);  
                }  
                else 
                {  
                    return lstUnProcessesData.Items[index].ToString();  
                }  
            }  
     
            private void btnStop_Click(object sender, EventArgs e)  
            {  
                for (int i = 1; i <= hashThread.Count; i++)  
                {  
                    Thread t = (Thread)hashThread[i];  
                    t.Abort();  
                    t.Join();  
                }  
            }   
     
        }  
    }  
     
    • Edited by mexil Saturday, January 3, 2009 5:47 PM I originally pasted in the wrong code.
    Saturday, January 3, 2009 5:46 PM
  • Correction -- my use of counter was incorrect. Here is a replacement for the processData function.

            private void processData()  
            {  
                while (counter < (lstUnProcessesData.Items.Count))  
                {  
                    string data = getData();  
                    doWork();  
                    SetItemSafe(data + " , " + Thread.CurrentThread.Name); //notify work done!    
                }  
            } 
    Saturday, January 3, 2009 5:54 PM
  • hi mexilus, thanks

    your code is working nicely while taking data from unprocessed listbox, but it is not processing all the data.
    also try to change the no. of thread count to 13 , it will throw out of range exception.

    Let's Program.. , Yam Sapkota
    Saturday, January 3, 2009 6:11 PM
  •  You have to use Monitor class to control the thread execution. Another option is to use a ThreadPool, yet another is to use AutoRecetEvent class, there are many other options. Abort method has been deprecated and this may be one of your problems. Don't expect the Thread to dies as soon as you aborted it. It takes GC some time to figure out what to do. Also it appears you are joining the threads after you aborted them. Why? With Threadpool you will have least trouble but no control as to when the Thread will stop. GC will be taking care of it. Otherwise, generate all your threads and Join them immediately.

    I am criticisng the original code here, but it looks maxilus is doing the same thing. I wonder what is the meaning of it: Abort the thread and then Join it?

    AlexB
    Saturday, January 3, 2009 6:30 PM
  • hi alex, thanks

    I am not willing to use threadpool ( or i don't know how to ) , i am still learning and what i believe is thread were used in the situation like mine, so it must work. mexilus's code seems good , hope he will fix the issue.

    about join after calling abort, just following Microsoft for that. Also post the correct method for aborting thread if possible.


    From: http://msdn.microsoft.com/en-us/library/5b50fdsz.aspx

     " The thread is not guaranteed to abort immediately, or at all. This situation can occur if a thread does an unbounded amount of computation in the finally blocks that are called as part of the abort procedure, thereby indefinitely delaying the abort. To wait until a thread has aborted, you can call the Join method on the thread after calling the Abort method, but there is no guarantee that the wait will end. "



    Let's Program.. , Yam Sapkota
    Saturday, January 3, 2009 6:49 PM
  •  I read the explain and was surprised to find that they don't mention the fact that it has been deprecated and that it will not be used in future releases. Apparenttly MS has changed its mind on that perhaps in the last year or so. I do use Abort threads efficiently but in situations when the threads created pretty much the way you do are left to their own devices. I abort them and forget. Acvtually if you named a thread, aborted it and then tried to creat a new thread with the same name you will get an exception no matter how long the time span is between the events. Apparently the names are kept somewhere, I have no idea.


    AlexB
    Saturday, January 3, 2009 7:48 PM
  • Alex: Good catch -- I did not notice that the original code calls t.Abort before t.Join. It sounds to me like it will do what Yam intends, but I haven't studied that usage.

    Yam:

    In my testing, the program does process all 100 list box items from the left side of the form. They appear out of order, but all 100 are processed. Their indexes run from 0 to 99, so the maximum index in the output will be 99. (Also, make sure you use the revised version of processData which I posted.)

    Using 13 threads does lead to an out of range exception. I'll have a look at that.


    Sunday, January 4, 2009 3:53 AM
  • The out of range exception appears to be occurring because we read the value of counter in processData, in a thread-unsafe way:

            private void processData()  
            {  
                while (counter < (lstUnProcessesData.Items.Count))  
                {  
                    string data = getData();  
                    doWork();  
                    SetItemSafe(data + " , " + Thread.CurrentThread.Name); //notify work done!  
                    appendText(data);  
                }  
            } 

    That it occurs with 13 threads, and not with 5, may be a coincidence due more to timing than the number of threads. The root cause is the unsafe read.

    Sunday, January 4, 2009 3:24 PM
  • The solution may need some restructuring. Do you guys see anything that can be done to the existing code?
    Sunday, January 4, 2009 3:32 PM
  •  Using 13 threads does lead to an out of range exception. I'll have a look at that.

    Easy to explain. It is the devil's dozen:)

    AlexB
    Sunday, January 4, 2009 3:42 PM
  •         private void processData() 
            { 
                while (counter < (lstUnProcessesData.Items.Count-1)) 
                { 
                     
                    string data = getData(); 
                    if (counter > lstUnProcessesData.Items.Count) continue
                    doWork(); 
                    SetItemSafe(data + " , " + Thread.CurrentThread.Name); //notify work done!     
                } 
            }  

    this is working , but i know this is not the correct way to do. so , its not an answer.

    I would like to clarify my question again differently, added in the top.










    Let's Program.. , Yam Sapkota
    Sunday, January 4, 2009 4:21 PM
  • thanks to alex and mexilus ,

    here is the solution found myself with the help of mexilus code, even though this is resolved, i am learning threading so new idea and more good code is always welcome,

    please post your ideas and suggestion..

    the code i post below can take any number of thread, even 300 , doesn't matter even there is just 100 data. check the processData() for fixed code.

    using System; 
    using System.Collections; 
    using System.Drawing; 
    using System.Threading; 
    using System.Windows.Forms; 
     
    namespace ThreadLearningNew 
        public partial class Form1 : Form 
        { 
            int counter = 0; 
            int threadCounter = 0; 
            Hashtable hashThread = new Hashtable(); 
             
     
            public Form1() 
            { 
                InitializeComponent(); 
            } 
     
            private void Form1_Load(object sender, EventArgs e) 
            { 
                for (int i = 0; i < 100; i++) 
                { 
                    lstUnProcessesData.Items.Add("Data:" + i.ToString("D2")); //to sort correctly in listbox.. 
                } 
            } 
     
            private void processData() 
            { 
               // while (counter < (lstUnProcessesData.Items.Count-1)) 
                while(getIndex()<(lstUnProcessesData.Items.Count-1)) //this is the improved line :) 
                { 
                    string data = GetListBoxText(counter); 
                    doWork(); 
                    SetItemSafe(data + " , " + Thread.CurrentThread.Name); //notify work done!     
                } 
            } 
     
            private int getIndex() 
            { 
                lock (this
                { 
                    return counter++; 
                } 
            } 
            
            void doWork() 
            { 
                Thread.Sleep(3000); //time consuming work..     
            } 
     
            private void btnStart_Click(object sender, EventArgs e) 
            { 
                lstProcessedData.Items.Clear(); 
     
                for (int i = 0; i < 23; i++) 
                { 
                    threadCounter++; 
                    Thread t = new Thread(processData); 
                    hashThread.Add(threadCounter, t); 
                    t.Name = threadCounter.ToString(); 
                    t.Start(); 
                    this.Text = "Total Thread:" + threadCounter.ToString(); 
                } 
            } 
     
            private delegate void Invoker(string parameter); 
            private void SetItemSafe(string data) 
            { 
                if (lstProcessedData.InvokeRequired) 
                { 
                    lstProcessedData.BeginInvoke(new Invoker(SetItemSafe), data); 
                } 
                else 
                { 
                     
                    lstProcessedData.Items.Add(data); 
                } 
            } 
     
            private delegate string GetListBoxTextInvoker(int index); 
            private string GetListBoxText(int index) 
            { 
                if (lstUnProcessesData.InvokeRequired) 
                { 
                    return (string)lstUnProcessesData.Invoke(new GetListBoxTextInvoker(GetListBoxText), index); 
                } 
                else 
                { 
                    return lstUnProcessesData.Items[index].ToString(); 
                } 
            } 
     
            private void btnStop_Click(object sender, EventArgs e) 
            { 
                for (int i = 1; i <= hashThread.Count; i++) 
                { 
                    Thread t = (Thread)hashThread[i]; 
                    t.Abort(); 
                    t.Join(); 
                } 
            } 
     
        } 
     


    Let's Program.. , Yam Sapkota
    • Marked as answer by Yam Sapkota Sunday, January 4, 2009 5:57 PM
    Sunday, January 4, 2009 5:55 PM
  • Hi Yam, that's better, but note that you're still reading counter in a thread-unsafe way (inside the while loop in processData).

    I've renamed getIndex as getNextIndex, and have made a few other changes to it. I've also revised processData to eliminate the unsafe read of counter's value.


            private void processData()  
            {  
                // Initialize index from counter in a thread-safe way  
                int index = getNextIndex();   
     
                while (index != -1)  
                {  
                    // Get data and do work on it  
                    string data = GetListBoxText(index);  
                    doWork();  
     
                    // Notify that work is done  
                    SetItemSafe(data + " , " + Thread.CurrentThread.Name);  
     
                    // Get next index in a thread-safe way  
                    index = getNextIndex();  
                }  
            }  
     
            private int getNextIndex()  
            {  
                lock (this)  
                {  
                    if (counter == lstUnProcessesData.Items.Count)  
                    {  
                        return -1; // max index was previously reached  
                    }  
                    else 
                    {  
                        counter++; // increment count  
                        return counter - 1; // index is 1 less than count  
                    }  
                }  
            } 

    Thanks for coming up with such an interesting exercise. I just learned .NET threading myself last week, so this has been good practice.

    Incidentally, ThreadPool is worth checking out for future learning. That was a good suggestion from Alex.
    Monday, January 5, 2009 3:22 AM