Problems with my threads array

Question

The form I'm trying to develop has an array of 6 picture boxes and an array of 6 die images. I have a button that when clicked needs to create 6 threads that "roll" the dice, showing each image for a moment. The problem I'm having is that I need to call a method within button click after the dice have been rolled. I can get the dice to roll but the message box is displayed immediately. I've tried a few different ways and get various errors. In the non working version below, the program freezes. I've checked out a ton of resources but I'm just not grasping some concepts like Delegates and Invoke all that well. Any help would be great! Here's my program

namespace testDice
{
    public partial class Form1 : Form
    {
        private Image[] imgAr;
        private PictureBox[] picBoxAr;
        private Random r;
        private Thread[] tArray;
        private ThreadStart tStart;
        private delegate void setTheImages();

        public Form1()
        {
            InitializeComponent();
            setImageArray();
            setPicBoxAr();
        }

        private void setImageArray()
        {
            imgAr = new Image[6];
            imgAr[0] = testDice.Properties.Resources.die6;
            imgAr[1] = testDice.Properties.Resources.die1;
            imgAr[2] = testDice.Properties.Resources.die2;
            imgAr[3] = testDice.Properties.Resources.die3;
            imgAr[4] = testDice.Properties.Resources.die4;
            imgAr[5] = testDice.Properties.Resources.die5;

        }

        private void setPicBoxAr()
        { 
            picBoxAr = new PictureBox[6];
            picBoxAr[0] = pictureBox1;
            picBoxAr[1] = pictureBox2;
            picBoxAr[2] = pictureBox3;
            picBoxAr[3] = pictureBox4;
            picBoxAr[4] = pictureBox5;
            picBoxAr[5] = pictureBox6;
        }   

        private void button1_Click(object sender, EventArgs e)
        {
            roll();

            //wait for threads to finish and update images--doesn't work
            for (int n = 0; n < 6; n++)
            {
                while (tArray[n].IsAlive)
                {
                    for (int i = 0; i < 6; i++)
                    {
                        this.picBoxAr[i].Update();
                    }
                }
            }

            MessageBox.Show("Each die has its own thread");
        }

        private void roll()
        {
            this.tStart = new ThreadStart(RunAllDiceThreads);
            this.tArray = new Thread[6];
            for (int i = 0; i < 6; i++)
            {
                this.tArray[i] = new Thread(tStart);
                this.tArray[i].Start();
            }
        }

        private void RunAllDiceThreads()
        {  
            int n = 0;
            while (n < 50)
            {
                setImg();
                Thread.Sleep(50);
                n++;
            }

            for (int i = 0; i < 6; i++)
            {
                if (tArray[i] != null)
                {
                    tArray[i].Abort();
                    tArray[i] = null;
                }
            }
        }// end RunAllDiceThreads

        private void setImg()
        {
            r = new Random();

            for (int i = 0; i < 6; i++)
            {
                    if (this.picBoxAr[i].InvokeRequired)
                    {
                        setTheImages s = new setTheImages(setImg);
                        // parameter mismatch error here
                        //this.Invoke(s, new object[] { imgAr[r.Next(6)] }); 
                        //Freezes here!!
                          this.Invoke(s);
                    }
                    else
                    {
                        this.picBoxAr[i].Image = imgAr[r.Next(6)];
                    }
            }
        }//end setImg

   }// end class Form1
}//end namespace testDice

1. That whole program looks like one big thread-safety violation. 2. I think you don't need threads for this at all. Instead, use a timer and update all dice when it ticks. — svick
– svick, Commented Dec 6, 2012 at 1:38

Community · Accepted Answer · 2017-05-23 12:29:20Z

2

Sounds like you're getting a deadlock between your invocation of setting the images and your update of the picture boxes.

I'd recommend rethinking your program a bit. Your program almost seems to be built on the concept that you're modeling an individual die with an individual thread. Break up the state of the die from the state of the thread. For example, you might want to create a Die class which has a certain state to it, such as IsRolling, or CurrentValue. Use and modify objects of that class (and that class only) inside your loops in your worker threads. That way, you won't have to invoke back to your UI thread to update. The dependencies are a lot cleaner that way. You might want to create a Timer in your UI thread which periodically fires (say 10-30 times a second), reads the state of each of the dice, and updates the images that way. That's a lot safer in terms of deadlocks because you don't have any cyclic dependencies. It'll also likely produce a more attractive interface because your die images will update in a smoother, more predictable fashion.

Another rule of thumb... Don't call Thread.Abort() (see references). It's generally a lot safer to use a property of a Die object and simply read from that to update your UI.

edited May 23, 2017 at 12:29

CommunityBot

11 silver badge

answered Dec 6, 2012 at 1:26

Dave Markle

98.3k20 gold badges152 silver badges172 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

Mox Over a year ago

Thanks for the comments. I've been trying to create a dice class in another version, but it's not going so well. How does the dice class object update the Picture Box? But yeah, I'm trying to go down that road.

Dave Markle Over a year ago

Ah, but see there is your catch... If you insist on going down the road of having the Dice update the UI thread, you open yourself up to complexity and deadlocks, just as you're having now. The program you posted is in fact a good, simple counterexample for not pursuing this architecture. A lot of programmers run into this trap -- pursuing the goal of encapsulating everything that's done by an object in a thread, when really what should be done to separate the functionality of UI from the behavior of the "model", in this case, your Dice. en.wikipedia.org/wiki/Separation_of_concerns

Mox Over a year ago

I hear what you are saying. I've been reading about MVC and trying to keep that in mind. I'm just not sure how to separate the 2 aspects of the assignment. I need to roll each dice on its own threads. Each thread has to update the image of the Picture Box. When the thread are done, I need to run a method (messagebox)...

Dave Markle Over a year ago

Keep a static array of your 6 Dice objects. When the timer ticks, check each one to see what number you want to display. When the timer ticks, and none of the dice are rolling anymore, have the timer show the Message Box. It's as simple as that. You don't need to invoke any methods across threads or anything.

Trisped · Accepted Answer · 2012-12-06 01:27:14Z

0

You need to remove MessageBox.Show("Each die has its own thread"); from button1_Click.

Create a property to track how many threads have returned. When it hits 6 invoke MessageBox.Show("Each die has its own thread"); (you will probably want to put this call in its own method and invoke that method).

Your problem is that you are starting the threads, then while they are running showing the message box rather then waiting for the threads to return.

answered Dec 6, 2012 at 1:27

Trisped

6,0442 gold badges51 silver badges65 bronze badges

Comments

Maurice Reeves · Accepted Answer · 2012-12-06 01:31:28Z

If you're able to work against the latest version of the .Net Framework, I would recommend making use of the System.Threading.Tasks namespace. The nice thing is that it encapsulates a lot of the multithreading details and makes things much cleaner. Here's a simple example.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace TasksExample
{
    class Program
    {
        static void Main(string[] args)
        {
            // holds all the tasks you're trying to run
            List<Task> waitingTasks = new List<Task>();

            // a simple object to lock on
            object padlock = new object();

            // simple shared value that each task can access
            int sharedValue = 1;

            // add each new task to the list above.  The best way to create a task is to use the Task.Factory.StartNew() method.
            // you can also use Task.Factory<RETURNVALUE>.StartNew() method to return a value from the task
            waitingTasks.Add(Task.Factory.StartNew(() => 
            {
                // this makes sure that we don't enter a race condition when trying to access the
                // shared value
                lock (padlock)
                {
                    // note how we don't need to explicitly pass the sharedValue to the task, it's automatically available
                    Console.WriteLine("I am thread 1 and the shared value is {0}.", sharedValue++);
                }
            }));

            waitingTasks.Add(Task.Factory.StartNew(() => 
            {
                lock (padlock)
                {
                    Console.WriteLine("I am thread 2 and the shared value is {0}.", sharedValue++);
                }
            }));

            waitingTasks.Add(Task.Factory.StartNew(() => 
            {
                lock (padlock)
                {
                    Console.WriteLine("I am thread 3 and the shared value is {0}.", sharedValue++);
                }
            }));

            waitingTasks.Add(Task.Factory.StartNew(() => 
            {
                lock (padlock)
                {
                    Console.WriteLine("I am thread 4 and the shared value is {0}.", sharedValue++);
                }
            }));

            waitingTasks.Add(Task.Factory.StartNew(() => 
            {
                lock (padlock)
                {
                    Console.WriteLine("I am thread 5 and the shared value is {0}.", sharedValue++);
                }
            }));

            waitingTasks.Add(Task.Factory.StartNew(() => 
            {
                lock (padlock)
                {
                    Console.WriteLine("I am thread 6 and the shared value is {0}.", sharedValue++);
                }
            }));


            // once you've spun up all the tasks, pass an array of the tasks to Task.WaitAll, and it will
            // block until all tasks are complete
            Task.WaitAll(waitingTasks.ToArray());

            Console.WriteLine("Hit any key to continue...");
            Console.ReadKey(true);
        }
    }
}

I hope this helps, and let me know if you need any more help.

How exactly is the code you posted relevant to this question?
My intention is to avoid the use of older threading code in favor of the newer Tasks namespace, which is easier to use. If the OP intends to use threading to update the value of the dice, then this is a cleaner way to do it. Also, it highlights the use of the lock keyword to prevent deadlock when accessing shared resources.

Collectives™ on Stack Overflow

Problems with my threads array

3 Answers 3

4 Comments

Comments

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

4 Comments

Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related