Thread Abortion Delay causes Delay in Display Response

Question

I'm writing a program which updates a display using three threads, two that both write images to the display form, and one to switch between the two when conditions are necessary. I'll include code for all three at the bottom. I've noticed that when the absence of audio prompts for the abortion of the talkingThread, and the creation & start of the neutralThread, the talking images will hang there for a few seconds, the duration varying. I suspect it has something to do with the Sleep calls at the end of the thread, that calling Abort doesn't actually dispose the thread until that sleep is cleared, but I don't know if that would affect another thread's ability to write over the display. Is there any way to ensure that the thread is disposed sooner?

Neutral Loop -

private void neutralLoop() //loop for switching to and maintaining the neutral images
        {  
            while (true)
            {
                if (isBlinking) //if blinking is enabled, check whether or not to blink, and hold for 1 second if so
                {
                    int blinkChance = rnd.Next(2);
                    if (blinkChance == 1)
                    {
                        myImage = this.neutralEyesClosedMap;
                        pictureBox1.Image = myImage;
                        Thread.Sleep(1000);
                    }
                }
                myImage = this.neutralEyesOpenMap; //replace neutral eyes open state, hold for 5 secs
                pictureBox1.Image = myImage;
                Thread.Sleep(5000);
            }
        }

Talking Loop -

private void talkingLoop() //loop for switching to and maintaining the talking images
        {
            while (true)
            {
                if (isBlinking) //if blinking is enabled, check whether or not to blink, and hold for 1 second if so
                {
                    int blinkChance = rnd.Next(2);
                    if (blinkChance == 1)
                    {
                        myImage = this.talkingEyesClosedMap;
                        pictureBox1.Image = myImage;
                        Thread.Sleep(1000);
                    }
                }
                myImage = this.talkingEyesOpenMap; //replace neutral eyes open state, hold for 5 secs
                pictureBox1.Image = myImage;
                Thread.Sleep(5000);
            }
        }

Switch Loop

private void switchLoop()
        {
            neutralThread.Start();
            while (true)
            {
               if (deviceMeter.MasterPeakValue > 0 && deviceMeter.MasterPeakValue < 1) //check for audio
               {
                    if (talkingThread.ThreadState != ThreadState.Running && talkingThread.ThreadState != ThreadState.WaitSleepJoin) //if talkingThread isnt running (neutralThread is running), get rid of it and start talkingThread.
                    {
                        neutralThread.Abort();
                        talkingThread = new Thread(new ThreadStart(talkingLoop));
                        talkingThread.Start();
                    }
               } else {
                    if (neutralThread.ThreadState != ThreadState.Running && neutralThread.ThreadState != ThreadState.WaitSleepJoin) //if neutralThread isnt running (talkingThread is running), get rid of it and start neutralThread.
                    {
                        talkingThread.Abort();
                        neutralThread = new Thread(new ThreadStart(neutralLoop));
                        neutralThread.Start();
                    }
               }
            }
        }

FWIW, I would highly suggest you come up with a solution that avoids `Thread.Abort()`. — 500 - Internal Server Error, Nov 04 '21 at 12:58
You should not be using Thread.Abort in the first place. Instead, you should move to proper async programming as you can leverage the same classes to run on background thread, but then you have support cancellation tokens and such to implement proper **cooperative** thread synchronization. Again, **don't use Thread.Abort!!** — Lasse V. Karlsen, Nov 04 '21 at 13:14
Additionally, you shouldn't be changing properties on controls from a background thread in WinForms. — Lasse V. Karlsen, Nov 04 '21 at 13:14
There are a lot of "rules" (that exist for good reason) being violated in this code. One is that you're not allowed to modify Windows Forms controls from any thread but the UI thread. So this whole approach is out. Fortunately, if you only need the threads to implement timing, you can do that without introducing threads at all with a System.Windows.Forms.Timer. — adv12, Nov 04 '21 at 13:15
@LasseV.Karlsen I have a few questions. I apologize, I am mostly self-taught when it comes to threading, so I am unfamiliar with proper practice. What do you mean by proper async programming? I've researched a bit about cooperative thread cancellation, and cancellation tokens, and see how they would be useful, but is there more to it I should be aware of? — James Ehrlinger, Nov 04 '21 at 13:32
@adv12 The problem with using a timer is that the states switch at varying intervals. Also, is there any way to influence a form from a non-main thread that satisfies convention? — James Ehrlinger, Nov 04 '21 at 13:35
@JamesEhrlinger, yes, you can use Control.Invoke() to marshal a call back onto the main thread. But your problem really is easily solvable with a timer; it just requires some bookkeeping. Set the timer for 1s and keep track of how many 1s intervals have passed since your last state change, and figure out whether a state change is required on this interval. Believe me, that's a lot less work than learning everything you'd need to know to do this right with threading. — adv12, Nov 04 '21 at 13:38
If you really want to learn to do threading right (without producing highly error-prone code), it's worth reading this entire book: http://www.albahari.com/threading/ — adv12, Nov 04 '21 at 13:56
Though as @LasseV.Karlsen points out, many of the details are abstracted away with async programming — adv12, Nov 04 '21 at 13:58

JonasH · Accepted Answer · 2021-11-04T14:44:22.393

Using Thread.Abort() is a terrible idea that can lead to all kinds of issues. The example also looks like it is modifying UI elements on background threads, and this is also not allowed, and can lead to various issues.

The correct way to cancel some running background task is to do so cooperatively. However, it does not look like we need to use any background threads at all. What we do need is a state-machine and a timer. The state-machine keeps track of what state we are in, what images to use, and what should happen when we switch between states. The timer is used to trigger a state-change after some time.

It is possible to use async/await for this since this will be compiled to a statemachine. while Task.Delay wraps a timer in a way that can be used with async/await. So as far as I can tell you could rewrite your example like this:

        private async Task Loop() 
        {
            while (true)
            {
                cts = new CancellationTokenSource(); // replace the cancellationToken source each iteration
                try
                {
                    if (isBlinking) {
                        int blinkChance = rnd.Next(2);
                        if (blinkChance == 1)
                        {
                            pictureBox1.Image = isneutral ? neutralEyesClosedMap : talkingEyesClosedMap;
                            await Task.Delay(1000, cts.Token);
                        }
                    } 
                    pictureBox1.Image = isneutral ? neutralEyesOpenMap : talkingEyesOpenMap;
                    await Task.Delay(5000, cts.Token);
                }
                // Task.Delay may throw when isneutral changes state, this is expected, just continue
                catch(OperationCanceledException){} 
            }
        }
        public async Task SetMasterPeakValue(double value)
        {
            var oldValue = isneutral;
            isneutral = value > 0 && value < 1;
            if (oldValue != isneutral)
            {
                cts.Cancel(); // cancel the current loop
            }
        }

This should as far as I can tell maintain the same behavior, while running all the code on the main-thread, and avoids all nastiness with threads. Note that this requires the class to be informed when the talking/neutral condition actually changes, my example uses a method, but it could just as well be an event. You probably also need some way to stop the entire loop, this could be something simple as a boolean, or a another cancellationTokenSource.

Thread Abortion Delay causes Delay in Display Response

1 Answers1