Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have the async code that implements cancellation token. It's working but Im not pretty sure if this is the right way to do it so I just want feedback about it.

Here is the actual code:

/// <summary>
/// 
/// </summary>
private async void SaveData() {

    if (GetActiveServiceRequest() != null)
    {
        var tokenSource = new System.Threading.CancellationTokenSource();


        this.ShowWizardPleaseWait("Saving data...");

        var someTask = System.Threading.Tasks.Task<bool>.Factory.StartNew(() =>
        {

            bool returnVal = false;

            // Set sleep of 7 seconds to test the 5 seconds timeout.
            System.Threading.Thread.Sleep(7000);

            if (!tokenSource.IsCancellationRequested)
            {
                // if not cancelled then save data

                App.Data.EmployeeWCF ws = new App.Data.EmployeeWCF ();
                returnVal = ws.UpdateData(_employee.Data);
                ws.Dispose();
            }

            return returnVal;

        }, tokenSource.Token);


        if (await System.Threading.Tasks.Task.WhenAny(someTask, System.Threading.Tasks.Task.Delay(5000)) == someTask)
        {
            // Completed
            this.HideWizardPleaseWait();
            if (someTask.Result)
            {
                this.DialogResult = System.Windows.Forms.DialogResult.OK;
            }
            else
            {
                this.DialogResult = System.Windows.Forms.DialogResult.Abort;
            }
            btnOK.Enabled = true;
            this.Close();
        }
        else
        {
            tokenSource.Cancel();

            // Timeout logic
            this.HideWizardPleaseWait();
            MessageBox.Show("Timeout. Please try again.")
        }


    }
}

Does async / await / cancellation code is well implemented?

share|improve this question

1 Answer 1

It's better to use CancellationTokenSource(TimeSpan) constructor to set the cancellation after 5 seconds.

Also, Task.Run method is a recommended way to run compute-bound tasks (see remark here).

Other issues worth noting:

  • by conventions asynchronous methods should have a suffix Async
  • using keyword is a recommended way to correctly dispose IDisposable objects
  • if a task to be cancelled cannot return correct results (according to business requirements) it's recommended to throw OperationCanceledException by calling token.ThrowIfCancellationRequested() method.

    private async void SaveDataAsync() 
    {
        if (GetActiveServiceRequest() == null)
            return;
    
        var cancellationToken = new CancellationTokenSource(TimeSpan.FromSeconds(5)).Token;
        this.ShowWizardPleaseWait("Saving data...");
    
        var someTask = Task.Run<bool>(() =>
        {
            // Set sleep of 7 seconds to test the 5 seconds timeout.
            System.Threading.Thread.Sleep(7000);
    
            cancellationToken.ThrowIfCancellationRequested();
    
            // if not cancelled then save data
            using (App.Data.EmployeeWCF ws = new App.Data.EmployeeWCF())
            {
                return ws.UpdateData(_employee.Data);
            }
        }, cancellationToken);
    
        try
        {
            this.DialogResult = await someTask
                ? DialogResult.OK
                : DialogResult.Abort;
    
            btnOK.Enabled = true;
            this.Close();
        }
        catch(OperationCanceledException)
        {
            MessageBox.Show("Timeout. Please try again.")
        }
        finally
        {
            this.HideWizardPleaseWait();
        }
    }
    
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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