Correctly cancelling and clearing a CancellationToken

Question

We have an async method called GetThings that goes off to 4 different providers and looks for results. Two of those providers are very fast and as a consequence are not written asynchronously however the other two providers are slow and are written asynchronously.

The GetThings method is called on a text change so a call to it should cancel previous calls. We do this using a CancellationTokenSource. The start of the GetThings method looks like this:

private async void GetThings()
{
    if (this.quickEntryCancellationTokenSource != null &&
        this.quickEntryCancellationTokenSource.IsCancellationRequested == false)
    {
        this.quickEntryCancellationTokenSource.Cancel();
    }

    this.quickEntryCancellationTokenSource = new CancellationTokenSource();

We decided there is no point in returning anything from GetThings until all 4 providers have completed. We are accomplishing this using Task.WhenAll

var getThings1Task = this.GetThings1();
var getThings2Task = this.GetThings2();
var getThings3Task = this.GetThings3();
var getThings4Task = this.GetThings4();

await Task.WhenAll(getThings1Task, getThings2Task, getThings3Task, getThings4Task).ConfigureAwait(false);

// Read the .Result of each of the tasks

localSuggestions.Insert(0, getThings1Tasks.Result);
localSuggestions.Insert(1, getThings2Tasks.Result);
localSuggestions.Insert(2, getThings3Tasks.Result);
localSuggestions.Insert(3, getThings4Tasks.Result);

this.quickEntryCancellationTokenSource = null;

Once we have read the .Result of each of the Task we set the quickEntryCancellationTokenSource to null. All this code is wrapped in the usual CancellationTokenSource exception handling. The start of GetThings3 passes the CancellationTokenSource down a layer if necessary.

private async Task<GroupedResult<string, SearchSuggestionItemViewModel>> GetThings3()
{
    List<Code> suggestions;

    if (this.SearchTerm.Length >= 3)
    {
        suggestions = await this.provider3.SearchAsync(this.SearchTerm, this.quickEntryCancellationTokenSource.Token);
    }
    else
    {
        suggestions = new List<Code>();
    }

The problem we are seeing is that sometimes quickEntryCancellationTokenSource is null. In my naivety i cannot see how that is possible as the CancellationTokenSource is created before the Tasks are waited on and not set to Null until they are completed.

My questions are:

1. How is quickEntryCancellationTokenSource null?
2. Should we be synchronising access to the cancellationTokenSource? Because two of the four providers are so quick they never deal with the cancellationTokenSource.
3. Should all four methods be calling ThrowIfCancellationRequested?
4. Is it wrong that two of the four provider methods never await anything?

Answer 1

Should we be synchronising access to the cancellationTokenSource? Because two of the four providers are so quick they never deal with the cancellationTokenSource.

You should synchronize access to the CTS, but not for that reason. The reason is because the quickEntryCancellationTokenSource field can be accessed from multiple threads.

If this is in the context of a UI thread, then you can just use the UI thread for synchronization:

// No ConfigureAwait(false); we want to only access the CTS from the UI thread.
await Task.WhenAll(getThings1Task, getThings2Task, getThings3Task, getThings4Task);
...
this.quickEntryCancellationTokenSource = null;

Also, I would consider passing the CT in to getThings1Task and the others, instead of having them read from a private variable.

Should all four methods be calling ThrowIfCancellationRequested?

That's up to you. If your two "fast" provider methods don't take a CT, then you could check it, but it doesn't sound to me like it would provide any benefit.

Is it wrong that two of the four provider methods never await anything?

If they're doing I/O, they should be using await, even if they normally are very fast. If they're not doing I/O, then I don't see much point in wrapping them in a Task at all. Presumably, the current code executes synchronously and always returns a completed task, so it's just a more complex way of just executing code synchronously in the first place. OTOH, if they're doing I/O but you don't have an asynchronous API for them (and since you're in a UI context), you can wrap them in a Task.Run.

Once we have read the .Result of each of the Task

Consider using await, even for completed tasks. It'll make your exception handling code easier.