Crash from Invoke when MainForm is Disposed in FormClosing

Question

I have an issue that relates to threading, cleaning up unmanaged resources and shutting down my app.

In the main UI thread I have a method that creates a new instance of class Worker. In Worker's constructor I start a new thread that has a while(Scanning) loop that updates some controls in my UI using Invoke() continuously (until Scanning bool is set to false). In the UI thread I raise the event FormClosing() whenever the application is closing down (through X button or Application.Exit() etc.). In FormClosing() I set Scanning to false and do some cleanup of unmanaged resources (that can only be done after the worker thread is done, because it uses those resources. The problem is that when I close the app down the MainForm apparently gets instantly disposed, so the app crashes at the Invoke (because it is trying to make a delegate run from UI thread, but that thread is disposed).

In an attempt to make the worker finish before the UI closes I tried to create a method StopWorker() in the worker class where I put Scanning = false, and then Thread.Join. As you can imagine the Join caused a deadlock as it makes the UI thread sleep but the Invoke needs the UI thread to move on.

In summary I need to cleanup unmanaged resources in FormClosing. I need the worker thread to be done before I do that though, as it uses these resources. The worker thread cannot finish (it uses Invoke) if the MainForm is disposed, therefore creating a tricky situation.

Answer 1

Based on Hans Passant's answer here, I created the below solution. It seems to be working very well.

In UI class/thread:

private void Form1_FormClosing(object sender, FormClosingEventArgs e)
{
    var button = sender as Button;
    if (button != null && string.Equals(button.Name, @"CloseButton"))
    {
        //FormClosing event raised by a user-created button action
    }
    else
    {
        //FormClosing event raised by program or the X in top right corner
        //Do cleanup work (stop threads and clean up unmanaged resources)
        if (_bw.Scanning)
        {
            _bw.Scanning = false;
            ClosePending = true;
            e.Cancel = true;
            return;
        }

        //Code to clean up unmanaged resources goes here (dummy code below)
        ApplicationLogger.Get.Log("Doing final cleanup work and exiting application...");
        MemoryHandler.Get.Dispose();
        ApplicationLogger.Get.Dispose();
    }
}

My worker thread is in another class that has a public bool property called Scanning. It also has this while loop (notice the line at the bottom):

private void Worker()
{
    while (Scanning)
    {
        Thread.Sleep(50);

        _sendBackValue[0] = "lbOne";
        _sendBackValue[1] = "blaBla";
        _synch.Invoke(_valueDelegate, _sendBackValue);

        _sendBackValue[0] = "lbTwo";
        _sendBackValue[1] = "blaBla";
        _synch.Invoke(_valueDelegate, _sendBackValue);

        _sendBackValue[0] = "lbThree";
        _sendBackValue[1] = "blaBla";
        _synch.Invoke(_valueDelegate, _sendBackValue);
    }

    MainForm.Get.Invoke((Action)(() => MainForm.Get.StopScanning()));
}

Finally, back in the UI class/thread I have this method:

public void StopScanning()
{
    if (!ClosePending) return;
    ApplicationLogger.Get.Log("Worker thread is closing the application...");
    Close();
}

Answer 2

Could you not better use the BackgroundWorker class/control? It is much easier to use because it has already a lot of synchronization stuff in it.

But if you have a separate thread, in your FormClosing event, use:

yourThread.Abort(); 
yourThread.Join(); // or yourThread.Join(1000);  where 1000 is some kind of time out value

in your thread use try-excpet-finally construct

try
{
     // do your stuff
}
catch (ThreadAbortException)
{
    // do someting when your thread is aborted
}
finally
{
    // do the clean up. Don't let it take too long.
}

Note that the Join command will block further execution until the thread has stopped. Therefore, I would recommend a not too high value for the time out parameter, otherwise the user interface will be blocked and will irritate users.

Answer 3

Disclaimer: I do not advocate the use of Thread, ManualResetEvent and, above all, volatile in the .NET 4.5+ era, but since the .NET version was not specified I've done my best to address the problem while keeping things as backwards-compatible as possible.

Here's a solution which uses a polling variable and a ManualResetEvent to block the execution of the FormClosing handler until the loop has completed - without any deadlocks. In your scenario if you have a class-level reference to the Thread which runs the loop, you can use Thread.Join instead of ManualResetEvent.WaitOne in the FormClosing handler - the semantics will be the same.

using System;
using System.Threading;
using System.Windows.Forms;

namespace FormClosingExample
{
    public partial class Form1 : Form
    {
        private volatile bool Scanning = true;
        private readonly ManualResetEvent LoopFinishedMre = new ManualResetEvent(false);
        private readonly SynchronizationContext UiContext;

        public Form1()
        {
            this.InitializeComponent();

            // Capture UI context.
            this.UiContext = SynchronizationContext.Current;

            // Spin up the worker thread.
            new Thread(this.Loop).Start();
        }

        private void Loop()
        {
            int i = 0;

            while (this.Scanning)
            {
                // Some operation on unmanaged resource.
                i++;

                // Asynchronous UI-bound action (progress reporting).
                // We can't use Send here because it will deadlock if
                // the call to WaitOne sneaks in between the Scanning
                // check and sync context dispatch.
                this.UiContext.Post(_ =>
                {
                    // Note that it is possible that this will
                    // execute *after* Scanning is set to false
                    // (read: when the form has already closed),
                    // in which case the control *might* have
                    // already been disposed.
                    if (this.Scanning)
                    {
                        this.Text = i.ToString();
                    }
                }, null);

                // Artifical delay.
                Thread.Sleep(1000);
            }

            // Tell the FormClosing handler that the
            // loop has finished and it is safe to
            // dispose of the unmanaged resource.
            this.LoopFinishedMre.Set();
        }

        private void Form1_FormClosing(object sender, FormClosingEventArgs e)
        {
            // Tell the worker that it needs
            // to break out of the loop.
            this.Scanning = false;

            // Block UI thread until Loop() has finished.
            this.LoopFinishedMre.WaitOne();

            // The loop has finished. It is safe to do cleanup.
            MessageBox.Show("It is now safe to dispose of the unmanaged resource.");
        }
    }
}

Now, while this solution is (somewhat) tailored to your description of the problem (which I interpreted to the best of my ability), I had to make a large number of assumptions. If you want a better answer, you'll need to post a concise repro of the problem - not necessarily your production code, but at least a trimmed down working version which still has all the main nuts and bolts in place and exhibits the problem you've described.