Terminating/joining a thread in C#

Question

I can't seem to be able to kill my thread in C#. The program seems to get stuck in an infinite loop on the FormClosing event.

EDIT // I'm attempting to end the thread and close the whole program when the FormClosing event gets fired.

Here's the code:

public partial class Form1 : Form
{
    private Thread thread;
    private volatile bool threadRunning = true;

    public Form1()
    {
        InitializeComponent();
    }

    private void Loop()
    {
        Console.WriteLine(threadRunning);
        while (threadRunning)
        {
            MethodInvoker mi = delegate { timeLabel.Text = TimeWriterSingleton.Instance.OutputTime(); };
            Invoke(mi);
        }
    }

    private void Form1_Load(object sender, EventArgs e)
    {
        thread = new Thread(Loop);
        thread.Start();
    }

    private void Form1_FormClosing(object sender, FormClosingEventArgs e)
    {
        threadRunning = false;
        thread.Join();
    }
}

Simply don't do this. If you want some UI element to be updated at some time in the future, **make a timer** and when the timer ticks, update the element. — Eric Lippert, Oct 04 '16 at 18:09
Winforms and WPF are not thread safe!, So NEVER call them from any thread apart from the thread that runs the message loop. — Ian Ringrose, Oct 04 '16 at 18:47
See also e.g. https://stackoverflow.com/questions/17575673/thread-join-causing-deadlock, https://stackoverflow.com/questions/12502229/asynchronously-raised-events-which-use-invoke-causing-problems-with-multithreadi, and https://stackoverflow.com/questions/24211934/deadlock-when-thread-uses-dispatcher-and-the-main-thread-is-waiting-for-thread-t (that last one is about WPF, but the basic issue and concepts are identical). — Peter Duniho, Oct 04 '16 at 20:34

Blindy · Answer 1 · 2016-10-04T19:32:58.760

Your Join blocked the GUI thread, and your Invoke in the other thread is waiting for your GUI thread to process the delegate.

A quick fix would be to use BeginInvoke instead of Invoke, thus posting rather than sending the window message.

Alternatively, don't join. The purpose of that code is to clean up after yourself, why do you care when the thread dies?

A 3rd fix would be to just gut the thread, either through Thread.Abort or Environment.Exit. It might skip some clean up, but your particular code shouldn't care and the point is to exit anyway.

Edit: working code using BeginInvoke follows:

    private void Loop()
    {
        while (threadRunning)
        {
            BeginInvoke(new MethodInvoker(() => timeLabel.Text = DateTime.Now.ToString()));
            Thread.Sleep(100);
        }
    }

    private void Form1_FormClosing(object sender, FormClosingEventArgs e)
    {
        threadRunning = false;
        thread.Join();
    }

The issue with the original code is that it's running as fast as your CPU allows, filling the message queue to the point where the GUI thread can't keep up. Updating Windows controls is very expensive, compared to simply adding a number to a queue. So I added a pause between UI updates to let the GUI thread breathe.

To the downvoters, I'd be curious why you're doing it. Nothing I said is factually wrong.

Begininvoke is not allowing the form to close. It is running for ever — Pavan Chandaka, Oct 04 '16 at 18:47
Other alternatives may work. But I am trying to fix his specific scenario. It seems an example of race condition. My earlier answer (which I removed), worked for first time, but not from there onward. I removed it. — Pavan Chandaka, Oct 04 '16 at 18:48
It's not a race condition and `BeginInvoke` works just fine. I suggest brushing up on .NET multi-threading before further comments. — Blindy, Oct 04 '16 at 19:33

score 0 · Accepted Answer · answered Oct 04 '16 at 19:33

I decided to switch to using a timer. The code now looks like this, and the application works:

public partial class Form1 : Form
{
    private System.Timers.Timer timer;

    public Form1()
    {
        InitializeComponent();

        timer = new System.Timers.Timer(60000);
    }

    private void Form1_Load(object sender, EventArgs e)
    {
        timeLabel.Text = TimeWriterSingleton.Instance.OutputTime();
        timer.Elapsed += TimerElapsed;
        timer.Enabled = true;
    }

    private void TimerElapsed(object sender, ElapsedEventArgs e)
    {
        timeLabel.Text = TimeWriterSingleton.Instance.OutputTime();
    }
}

It should be noted that the value in the timer constructor (`60000`) is in milliseconds. So that will run once a minute. And it will hardly ever be aligned with the system clock. — Theraot, Oct 04 '16 at 20:11

score 0 · Answer 3 · answered Oct 04 '16 at 19:34

Actually using the BeginInvoke() is not bad idea. It might look like that:

private void Form1_Load(object sender, EventArgs e)
{
    thread = new Thread(() => Loop(this));
    thread.Start();
}

private void Loop(Form1 form)
{
    while (threadRunning && !form.IsDisposed)
    {
        MethodInvoker mi = delegate() { timeLabel.Text = /* Some text */ ; };
        BeginInvoke(mi);

        // Let sleep some time...
        Thread.Sleep(1);
    } 
}

private void Form1_FormClosing_1(object sender, FormClosingEventArgs e)
{
    threadRunning = false;
    thread.Join();
}

_bool threadRunning_ and _Sleep()_ can be combined to one **ManualResetEvent**. — Jackdaw, Oct 04 '16 at 19:45

Terminating/joining a thread in C#

3 Answers3