C# wpf multithreading

Question

I want to generate 3 random number in threads then save it to list. Here's my code

    using System;
    using System.Collections.Generic;
    using System.Linq;
    using System.Text;
    using System.Windows;
    using System.Windows.Controls;
    using System.Windows.Data;
    using System.Windows.Documents;
    using System.Windows.Input;
    using System.Windows.Media;
    using System.Windows.Media.Imaging;
    using System.Windows.Navigation;
    using System.Windows.Shapes;
    using System.Threading;
    using System.IO;

    namespace Multithreading1
    {
        /// <summary>
        /// Interaction logic for MainWindow.xaml
        /// </summary>
        public partial class MainWindow : Window
        {
            List<int> myList = new List<int>();
            int threadNumber = 0;
            int currentRecNumber = -1;

            public MainWindow()
            {
                InitializeComponent();
            }

            void ThreadHandler(int recNumber,int number)
            {
                Action action = null;
                action = () =>
                        {
                            myList[recNumber] = number;
                            ++currentRecNumber;
                            --threadNumber;
                            if (currentRecNumber < myList.Count)
                            {
                                ++threadNumber;
                                Thread t = new Thread(() => GetRandomNumber(currentRecNumber));
                                t.Start();
                            }
                            else
                                if (threadNumber == 0) //finish
                                {
                                    List<String> stringList = new List<String>();
                                    for (int i = 0; i < myList.Count;i++)
                                    {
                                        stringList.Add(myList[i].ToString());
                                    }
                                    File.WriteAllLines("C:\\Users\\Public\\Documents\\MyList.txt", stringList);
                                    System.Windows.MessageBox.Show("Finish");
                                }
                        };
                this.Dispatcher.BeginInvoke(action);
            }

            void GetRandomNumber(int recNumber)
            {
                Random rnd = new Random();
                int randomInt = rnd.Next(1, 13);
                ThreadHandler(recNumber, randomInt);
            }

            private void button1_Click(object sender, RoutedEventArgs e)
            {
                for (int i = 0; i < 20; i++)
                {
                    myList.Add(-1);
                }
                for (int i = 0; i < 3; i++)
                {
                    ++currentRecNumber;
                    ++threadNumber;
                    Thread t = new Thread(() => GetRandomNumber(currentRecNumber));
                    t.Start();
                }
            }
        }
    }

The problem are: 1. Sometimes it throw ArgumentOutOfRangeException at myList[recNumber] = number; 2. If it get past (1) the resulting file still contain -1s, eg:

-1
-1
8
6
11
-1
1
3
-1
3
3
8
8
8
8
10
10
10
10
12

Anyone know what is wrong ? Thanks in advance.

Answer 1

Thanks Matt for the ebook. It's very easy to understand. I managed to fix my code with minor addition. The key of its problem is "Lambda expressions and captured variables", so I add couple local variables to it.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Windows;
using System.Windows.Controls;
using System.Windows.Data;
using System.Windows.Documents;
using System.Windows.Input;
using System.Windows.Media;
using System.Windows.Media.Imaging;
using System.Windows.Navigation;
using System.Windows.Shapes;
using System.Threading;
using System.IO;

namespace Multithreading1
{
    /// <summary>
    /// Interaction logic for MainWindow.xaml
    /// </summary>
    public partial class MainWindow : Window
    {
        List<int> myList = new List<int>();
        int threadNumber = 0;
        int currentRecNumber = -1;

        public MainWindow()
        {
            InitializeComponent();
        }

        void ThreadHandler(int recNumber,int number)
        {
            Action action = null;
            action = () =>
                    {
                        myList[recNumber] = number;
                        ++currentRecNumber;
                        --threadNumber;
                        int localCurrentRecNumber = currentRecNumber;
                        int localThreadNumber = threadNumber;
                        if (localCurrentRecNumber < myList.Count)
                        {
                            ++threadNumber;
                            Thread t = new Thread(() => GetRandomNumber(localCurrentRecNumber));
                            t.Start();
                        }
                        else
                            if (localThreadNumber == 0) //finish
                            {
                                List<String> stringList = new List<String>();
                                for (int i = 0; i < myList.Count;i++)
                                {
                                    stringList.Add(myList[i].ToString());
                                }
                                File.WriteAllLines("C:\\Users\\Public\\Documents\\MyList.txt", stringList);
                                System.Windows.MessageBox.Show("Finish");
                            }
                    };
            this.Dispatcher.BeginInvoke(action);
        }

        void GetRandomNumber(int recNumber)
        {
            Random rnd = new Random();
            int randomInt = rnd.Next(1, 13);
            ThreadHandler(recNumber, randomInt);
        }

        private void button1_Click(object sender, RoutedEventArgs e)
        {
            for (int i = 0; i < 20000; i++)
            {
                myList.Add(-1);
            }
            for (int i = 0; i < 3; i++)
            {
                ++currentRecNumber;
                ++threadNumber;
                int localCurrentNumber = currentRecNumber;
                Thread t = new Thread(() => GetRandomNumber(localCurrentNumber));
                t.Start();
            }
        }
    }
}

Answer 2

Your Dispatcher.BeginInvoke will call each action on the thread the dispatcher is associated with, so you are effectively not actually running the actions on different threads. It might be better to do as much as you can in the ThreadHandler method, and only make UI changes inside the BeginInvoke action.

Also in your button1_Click, you increment currentRecNumber before you start each thread, so that will cause the first few threads to skip the first few items in the list.

You also have a major issue because you are accessing shared variables (currentRecNumber, threadNumber, and myList) from different threads, which can cause all manner of threading issues. You need to use some sort of synchronisation to ensure that each thread is reading and writing the correct values from these variables. You can use the InterlockedIncrement and InterlockedDecrement to mitigate some of these issues, but not all of them.

I'll also point out that creating threads is expensive, it's much better to schedule the work you want doing on thread pool threads , use a BackgroundWorker, or use one of the parallelism libraries, like the Task Parallel Library or PLINQ.

I would recommend having a read of this free ebook on threading by Joe Albahari.