What is the proper procedure to alert a thread the stop what it's doing and return before exiting?

Question

What is the proper procedure to alert a running thread to stop what it's doing and return before exiting the application?

protected Thread T;
protected static ManualResetEvent mre = new ManualResetEvent(false);
protected static bool ThreadRunning = true;

public Form1()
{
    InitializeComponent();
    T = new Thread(ThreadFunc);
    T.Start();
}

private void ThreadFunc()
{
    while (ThreadRunning)
    {
        // Do stuff
        Thread.Sleep(40);
    }
    mre.Set();
}

private void ExitButton_Click(object sender, EventArgs e)
{
    ThreadRunning = false;
    mre.WaitOne();
    mre.Close();
    Application.Exit();
}

Initially I had my code setup like the above. My thinking for how exit properly is as follows:

1. Set ThreadRunning = false so that the next time thread T checks that variable it knows to stop.
2. Call mre.WaitOne() to wait for thread T to say it's actually done via it calling mre.Set().
3. If so, then unblock and continue, dispose of mre (mre.Close()) and exit.

For some reason the above setup sometimes fails after the exit button is clicked and the whole form becomes inactive.

My new setup is below but doesn't seem entirely correct to me, for instance mre.Set() isn't going to wait for anything and Application.Exit() is immediately after it. I'm just waiting for it to fail like before, but so far it hasn't.

protected Thread T;
protected static ManualResetEvent mre = new ManualResetEvent(false);
protected static bool ThreadRunning = true;

public Form1()
{
    InitializeComponent();
    T = new Thread(ThreadFunc);
    T.Start();
}

private void ThreadFunc()
{
    while (ThreadRunning)
    {
        // Do stuff
        Thread.Sleep(40);
    }
    mre.WaitOne();
    mre.Close();
}

private void ExitButton_Click(object sender, EventArgs e)
{
    ThreadRunning = false;
    mre.Set();
    Application.Exit();
}

Answer 1

UI hangs because you blocks UI thread with mre.WaitOne();. If you need to wait until thread exits, you can use its IsAlive property and process application messages, you don't need application events for that:

while(_t.IsAlive)
  Application.DoEvents();

There are 2 thread cancelation aproaches:

• cooperative - code that is executed by thread knows that it could be cancelled and cancellation handled gracefuly, that's what you try to do here.
• imperative - force thread to stop - call Thread.Abort or Interrupt, don't use that.

As @HansPassant mentioned, bool is not the best option because that compiler may optimize that and bool value could be cached and its change may not be handled by looping thread. You need to make it at least volatile or just refactor code to use CancellationSource.

Given what your thread is doing, perhaps BackgroundWorker, Timer or Producer/Consumer pattern is a better alternative to Thread, but I have too little context to recommend anything. Also it works well only if you have only 1 instance of Form1 in the application, if you have multiform application and user can open several Form1 forms, you'll have problems.

General recommendation, if you can work with instance level fields, please do that, don't use static.

Answer 2

Waiting for 40 msec between doing stuff creates no problems, but what if you had to wait 5 sec or more? Then canceling between each waiting would be problematic, and the right thing to do would be to cancel the awaiting itself. It is fairly easy to do it actually. Just replace Thread.Sleep(40) with Task.Delay(40, token).Wait() where token is a CancellationToken.

class Form1 : Form
{
    protected readonly CancellationTokenSource _cts;
    protected readonly Thread _thread;

    public Form1()
    {
        InitializeComponent();
        _cts = new CancellationTokenSource();
        _thread = new Thread(ThreadFunc);
        _thread.Start();
    }

    private void ThreadFunc()
    {
        try
        {
            while (true)
            {
                // Do stuff here
                Task.Delay(40, _cts.Token).GetAwaiter().GetResult();
            }
        }
        catch (OperationCanceledException)
        {
            // Ignore cancellation exception
        }
    }

    private void ExitButton_Click(object sender, EventArgs e)
    {
        _cts.Cancel();
        this.Visible = false; // Hide the form before blocking the UI
        _thread.Join(5000); // Wait the thread to finish, but no more than 5 sec
        this.Close();
    }
}

Personally I would prefer to do the background job using a Task instead of a Thread, because it is more easily awaited without blocking the UI. This task would run lazily using thread-pool threads. The drawback is that the stuff that runs every 40 msec would not always run in the same thread, so I could have thread-safety issues to resolve.

class Form1 : Form
{
    protected readonly CancellationTokenSource _cts;
    protected readonly Task _task;

    public Form1()
    {
        InitializeComponent();
        _cts = new CancellationTokenSource();
        _task = Task.Run(TaskFunc);
        this.FormClosing += Form_FormClosing;
    }

    private async Task TaskFunc()
    {
        try
        {
            while (true)
            {
                // Do async stuff here, using _cts.Token if possible
                // The stuff will run in thread-pool threads
                await Task.Delay(40, _cts.Token).ConfigureAwait(false);
            }
        }
        catch (OperationCanceledException)
        {
            // Ignore cancellation exception
        }
    }

    private void ExitButton_Click(object sender, EventArgs e)
    {
        this.Close();
    }

    private async void Form_FormClosing(object sender, FormClosingEventArgs e)
    {
        if (_task == null || _task.IsCompleted) return;
        e.Cancel = true;
        _cts.Cancel();
        this.Visible = false; // Or give feedback that the form is closing
        var completedTask = await Task.WhenAny(_task, Task.Delay(5000));
        if (completedTask != _task) Debug.WriteLine("Task refuses to die");
        _task = null;
        await Task.Yield(); // To ensure that Close won't be called synchronously
        this.Close(); // After await we are back in the UI thread
    }
}