C# threading and Windows Forms

Question

Is my approach to a responsive GUI with a background process correct? If not, please please critique and offer improvements. In particular, indicate what code could potentially suffer from a deadlock or race condition.

The worker thread needs to be able to be cancelled and report it's progress. I didn't use a BackgroundWorker because all the examples I've seen have the Process code on the Form itself, rather than a separate object. I thought about inheriting the LongRunningProcess for BackgroundWorker but I figured that would introduce unnecessary methods on the object. Ideally, I'd prefer not to have a Form reference to the process ("_lrp"), but I don't see how it would then be possible to cancel the process, unless I have an event on the LRP that checks a flag on the caller, but that seems unnecessarily complex and possibly even wrong.

Windows Form (Edit: moved *.EndInvoke calls to the callback)

public partial class MainForm : Form
{
 MethodInvoker _startInvoker = null;
 MethodInvoker _stopInvoker = null;
 bool _started = false;

 LongRunningProcess _lrp = null;

 private void btnAction_Click(object sender, EventArgs e)
 {
  // This button acts as a Start/Stop switch.
  // GUI handling (changing button text etc) omitted
  if (!_started)
  {
   _started = true;
   var lrp = new LongRunningProcess();

   _startInvoker = new MethodInvoker((Action)(() => Start(lrp)));
            _startInvoker.BeginInvoke(new AsyncCallback(TransferEnded), null);
        }
        else
        {
         _started = false;
         _stopInvoker = new MethodInvoker(Stop);
                _stopInvoker.BeginInvoke(Stopped, null);
        }
 }

 private void Start(LongRunningProcess lrp)
    {
        // Store a reference to the process
        _lrp = lrp;

        // This is the same technique used by BackgroundWorker
        // The long running process calls this event when it 
        // reports its progress
        _lrp.ProgressChanged += new ProgressChangedEventHandler(_lrp_ProgressChanged);
        _lrp.RunProcess();
    }

    private void Stop()
    {
        // When this flag is set, the LRP will stop processing
        _lrp.CancellationPending = true;
    }

    // This method is called when the process completes
    private void TransferEnded(IAsyncResult asyncResult)
    {
        if (this.InvokeRequired)
        {
            this.BeginInvoke(new Action<IAsyncResult>(TransferEnded), asyncResult);
        }
        else
        {
            _startInvoker.EndInvoke(asyncResult);
            _started = false;
            _lrp = null;
        }
    }

    private void Stopped(IAsyncResult asyncResult)
    {
        if (this.InvokeRequired)
        {
            this.BeginInvoke(new Action<IAsyncResult>(Stopped), asyncResult);
        }
        else
        {
            _stopInvoker.EndInvoke(asyncResult);
            _lrp = null;
        }
    }

    private void _lrp_ProgressChanged(object sender, ProgressChangedEventArgs e)
    {
        // Update the progress
        // if (progressBar.InvokeRequired) etc...
    }
}

Background Process:

public class LongRunningProcess
{
 SendOrPostCallback _progressReporter;
 private readonly object _syncObject = new object();
    private bool _cancellationPending = false;

    public event ProgressChangedEventHandler ProgressChanged;

    public bool CancellationPending
    {
        get { lock (_syncObject) { return _cancellationPending; } }
        set { lock (_syncObject) { _cancellationPending = value; } }
    }

    private void ReportProgress(int percentProgress)
    {
        this._progressReporter(new ProgressChangedEventArgs(percentProgress, null));
    }

    private void ProgressReporter(object arg)
    {
        this.OnProgressChanged((ProgressChangedEventArgs)arg);
    }

    protected virtual void OnProgressChanged(ProgressChangedEventArgs e)
    {
        if (ProgressChanged != null)
            ProgressChanged(this, e);
    }

    public bool RunProcess(string data)
    {
        // This code should be in the constructor
        _progressReporter = new SendOrPostCallback(this.ProgressReporter);

        for (int i = 0; i < LARGE_NUMBER; ++i)
        {
         if (this.CancellationPending)
          break;

         // Do work....
         // ...
         // ...

         // Update progress
         this.ReportProgress(percentageComplete);

         // Allow other threads to run
         Thread.Sleep(0)
        }

        return true;
    }
}

Answer 1

I like the separation of the background process in a separate object. However, my impression is that your UI thread is blocked until the background process is completed, because you call BeginInvoke and EndInvoke in the same button handler.

MethodInvoker methodInvoker = new MethodInvoker((Action)(() => Start(lrp)));
IAsyncResult result = methodInvoker.BeginInvoke(new AsyncCallback(TransferEnded), null);
methodInvoker.EndInvoke(result);

Or am I missing something?

Answer 2

I'm a little confused by your use of MethodInvoker.BeginInvoke(). Is there a reason you chose to use this instead of creating a new thread and using Thread.Start()...?

I believe you might block your UI thread because you call EndInvoke on the same thread as BeginInvoke. I would say the normal pattern is to call EndInvoke on the receiving thread. That's certainly true with asynchronous I/O operations - apologies if it doesn't apply here. You should easily be able to determine whether your UI thread is blocked until the LRP completes anyway.

Finally, you rely on a side-effect of BeginInvoke to start your LRP on a worker thread from the managed thread pool. Again, you should be sure that this is your intention. The thread pool includes queueing semantics and does a great job when loaded up with a large number of short-lived processes. I'm not sure it's such a good choice for long-running processes. I would favour using the Thread class to start your long-running thread.

Also, while I think your method of signalling the LRP to cancel it will work, I normally use a ManualResetEvent for that purpose. You don't have to worry about locking an event to check its state.

Answer 3

You may make your _cancellationPending volatile and avoid locking. Why are you calling Stop in another thread?

You should change you event calling method to avoid race condition :

protected virtual void OnProgressChanged(ProgressChangedEventArgs e)
{
    var progressChanged = ProgressChanged;
    if (progressChanged != null)
        progressChanged(this, e);
}

If the background worker fits, you don't have to recode it ;)

Answer 4

As Guillaume posted, you have a race condition in the OnProgressChanged method, however, I don't believe the answer provided is a solution. You still need a sync object to handle it.

private static object eventSyncLock = new object();

protected virtual void OnProgressChanged(ProgressChangedEventArgs e)
{
    ProgressChangedEventHandler handler;
    lock(eventSyncLock)
    {
      handler = ProgressChanged;
    }
    if (handler != null)
        handler(this, e);
}

Answer 5

You could use a BackgroundWorker, and still move your work code outside of the Form class. Have your class Worker, with its method Work. Let Work take a BackgroundWorker as a parameter, and overload the Work method with a non-BackgroundWorker signature, which sends null to the first method.

Then in your form, use a BackgroundWorker which has ProgressReporting, and in your Work( BackgroundWorker bgWorker, params object[] otherParams ), you can include statements such as:

    if( bgWorker != null && bgWorker.WorkerReportsProgress )
    {
        bgWorker.ReportProgress( percentage );
    }

... and similarly include checks for CancellationPending.

Then in your Forms code you can handle the events. First set bgWorker.DoWork += new DoWorkEventHandler( startBgWorker );, where that method kicks off your Worker.Work method, passing bgWorker as an argument.

This can be kicked off then from a button event, which called bgWorker.RunWorkerAsync.

A second cancellation button can then call bgWorker.CancelAsync, which will then get caught in your section where you checked for CancellationPending.

Upon success or cancellation, you will handle the RunWorkerCompleted event, where you check if the worker was cancelled. Then if it wasn't you assume it was successful and go that route.

By overloading the Work method, you can keep it reusable from code which doesn't care about Forms or the ComponentModel.

And, of course you implement the progresschanged event without needed to reinvent the wheel on that one. ProTip: ProgressChangedEventArgs takes an int, but doesn't force it to a max of 100. To report finer grained progress percentage, pass an argument with a multipler (say 100), so 14.32% will be a Progress of 1432. Then you can format the display, or override your progress bar, or show it as a text field. (all with DRY-friendly design)