4
\$\begingroup\$

Introduction:

For a project I'm working on I used a BackGroundWorker to lift some heavy tasks away from the UI thread and report progress. For this I created some sort of modal dialog with a label and progress bar which were updated from the ProgressChanged method of the backgroundworker.

I read a very interesting article on async/await and I thought I might update my code, since the BackGroundWorker is, despite being handy, getting outdated anyway. After some more reading on this matter I found an article with code that had the backgroundworker instance inside the modal dialog form and not on the caller form.

This gave me the idea to do the same but using async/await instead. This is a first result, which is working, and I was wondering if I am doing things right. Any tips and/or improvements are much appreciated, certainly regarding scalability/extensibility and the use of async/await.

The form:

enter image description here

The code:

public partial class CancellableProgress : Form
{
    private readonly IEnumerable<KeyValuePair<string, Action>> _tasks;
    private CancellationTokenSource _cancelTokenSource;
    private bool _isTaskCompleted;

    public CancellableProgress(string message, Action action)
        : this(new[] { new KeyValuePair<string, Action>(message, action) })
    {
    }

    public CancellableProgress(IEnumerable<KeyValuePair<string, Action>> tasks)
    {
        InitializeComponent();
        Load += CancellableProgress_Load;
        _tasks = tasks;
    }

    private void CancellableProgress_Load(object sender, EventArgs e)
    {
        PerformTasks(_tasks);
    }

    private async void PerformTasks(IEnumerable<KeyValuePair<string, Action>> tasks)
    {
        _cancelTokenSource = new CancellationTokenSource();
        var token = _cancelTokenSource.Token;
        var progressHandler = new Progress<string>(value => MessageLabel.Text = value);
        var progress = progressHandler as IProgress<string>;

        try
        {
            await Task.Run(() =>
            {
                foreach (var task in tasks)
                {
                    if (token.IsCancellationRequested)
                    {
                        token.ThrowIfCancellationRequested();
                    }
                    progress.Report(task.Key);
                    task.Value();
                }
            }, token);
            _isTaskCompleted = true;
        }
        catch (OperationCanceledException)
        {
            MessageLabel.Text = @"Cancelled!";
        }
        catch (Exception ex)
        {
            MessageLabel.Text = ex.Message;
        }
        finally
        {
            _cancelTokenSource.Dispose();
            DialogResult = _isTaskCompleted ? DialogResult.OK : DialogResult.Cancel;
        }
    }

    private void btnCancel_Click(object sender, EventArgs e)
    {
        CancelTaskButton.Enabled = false;
        _cancelTokenSource.Cancel();
    }
}

Example usage:

This is some code on how I use the modal form.

private void TaskButton_Click(object sender, EventArgs e)
{
    var operations = new[]
    {
        new KeyValuePair<string, Action>("Operation 1", LongOperation),
        new KeyValuePair<string, Action>("Second operation", LongOperationTwo)
    };

    using (var progress = new CancellableProgress(operations))
    {
        progress.ShowDialog();
    }
}

private static void LongOperation()
{
    for (var i = 1; i <= 5; i++)
        System.Threading.Thread.Sleep(1000);
}

private static void LongOperationTwo()
{
    for (var i = 1; i <= 5; i++)
        System.Threading.Thread.Sleep(1000);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ If I read this code correctly, token.ThrowIfCancellationRequested is only called between tasks. Given that you are using this in a modal dialog, that means when the user cancels, they will still have to wait until the current task finishes. An improvement would be to pass the cancellation token as a parameter to each LongOperation, which would test for cancel in each loop iteration, token.IsCancellationRequested. Then the longest delay would be the time it takes for one iteration. \$\endgroup\$ – ToolmakerSteve Apr 24 '18 at 8:49
  • 1
    \$\begingroup\$ ... code example of cancelling inside a loop here: Microsoft- dotnet- Cancellation in Managed Threads. \$\endgroup\$ – ToolmakerSteve Apr 24 '18 at 8:56
  • \$\begingroup\$ @ToolmakerSteve Long time since I saw this code. Thanks for your comment , I will look at it and see if I can improve the code using your advice. :) \$\endgroup\$ – Abbas Apr 24 '18 at 9:19
4
\$\begingroup\$

Clean code, easy to follow its meaning. Some minor remarks:

var progressHandler = new Progress<string>(value => MessageLabel.Text = value);
var progress = progressHandler as IProgress<string>;

This looks like you can just define it as

IProgress<string> progressHandler = new Progress(value => MessageLabel.Text = value);

Or just a Progress<string> type, really:

var progressHandler = new Progress(value => MessageLabel.Text = value);

Unless I'm mistaking, you don't actually need it as an interface. Note that I also removed the explicit type parameter since that should be inferred from the argument passed to it.


MessageLabel.Text = @"Cancelled!";

The verbatim string isn't necessary here


if (token.IsCancellationRequested)
{
    token.ThrowIfCancellationRequested();
}

This seems redundant to me.

It should be either this

if (token.IsCancellationRequested)
{
    throw new OperationCanceledException();
}

or this:

token.ThrowIfCancellationRequested()

This is also reinforced by the documentation


private async void PerformTasks()

Asynchronous methods should return Task or Task<T>, the former being for traditional void methods and the latter being for methods with T return type.

The reason for this is that a void method cannot be awaited (since it doesn't return a Task) and will in effect be a fire-and-forget kind of thing: it will start executing your asynchronous code on a separate thread and continue doing business as normal. However once the main thread reaches its end, the application will exit regardless of whether or not your asynchronous code has finished as well. This obviously leads to nasty bugs.

You won't notice this in this case because you're using a GUI which means there will always be a main thread active (as long as the application runs).

However in case you ever want to re-use this code for a console application or just in a class library where you don't know the client, I would suggest making it async Task just for good measure.

The only exception to this are asynchronous event handlers since they have to adhere to the convention that event handlers return void. This also explains why events are "dangerous" to use in asynchronous code.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks a lot for your useful answer. The first point can just be shortened to var progress = new Progress<string>(value => MessageLabel.Text = value) as IProgress<string>;, it has to be a reference to the interface. Points 2 and 3 are implemented. For the 4th point I will do some more reading and research as it was a useful tip with extensibility in mind. \$\endgroup\$ – Abbas Mar 19 '15 at 15:49
  • 1
    \$\begingroup\$ From a quick search I'd say that this is a good starting point for the 4th point: msdn.microsoft.com/en-us/magazine/jj991977.aspx \$\endgroup\$ – Jeroen Vannevel Mar 19 '15 at 15:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.