CancellationTokenSource for changing state after few seconds

Question

I'm trying to recreate the effect one sees in Outlook / Gmail: a few seconds after opening an unread email, it is marked as read. But if one clicks a different email before that time elapses then the email remains unread.

private CancellationTokenSource? _cts;

private async Task OnEmailClicked(int id)
{

  if (_cts != null)
  {
    _cts.Cancel();                                // I suspect problem is here
    _cts.Token.ThrowIfCancellationRequested();    // or here
    _cts.Dispose();
    _cts = null;
  }

  await LoadEmail(id);

  try
  {
    _cts = new CancellationTokenSource();
    await Task.Delay(3000, _cts.Token);
  }
  catch (TaskCanceledException)
  {
    return;
  }
  finally {
    _cts.Dispose();
    _cts = null;
  }

  await MarkEmailAsRead(id);
}

That gives me weird results. Sometimes it works and sometimes not. I'm unsure where/how to create, cancel and dispose the token source. Obviously my code is wrong.

What is the correct way to achieve this?

(I don't need to "fix" this code - I'm happy to throw it away and do it the proper way, if you can show me how. I've only included it to demonstrate what I've tried.)

Answer 1

First, calling ThrowIfCancellationRequested() will break things. It will throw an exception every time that line is hit (because you just cancelled the token the line before) and the rest of the method won't run.

The other issue I see is that you're setting _cts to null after cancelling it. Remember that you will have two versions of this method in progress at the same time. Assuming both versions of the method will be running on the UI thread then this entire block of code will run uninterrupted (I've removed ThrowIfCancellationRequested()):

  if (_cts != null)
  {
    _cts.Cancel();
    _cts.Dispose();
    _cts = null;
  }

And then once LoadEmail(id) is awaited, then the previous incomplete version of the method is given a chance to run and it goes into the catch and the finally where it calls _cts.Dispose(). But _cts has already been set to null so you will get a NullReferenceException and MarkEmailAsRead(id) will never get run.

So I would only call Dispose() in one place, not two. But I would also keep a reference to the CancellationTokenSource that we created so that if something new is assigned to _cts, we can still call Dispose() on the one we created. That would look something like this:

private CancellationTokenSource? _cts;

private async Task OnEmailClicked(int id)
{
  
  _cts?.Cancel();

  // Keep a local reference to the token source so that we can still call Dispose()
  // on the object we created here even if _cts gets assigned a new object by another
  // run of this method
  var cts = _cts = new CancellationTokenSource();
  
  await LoadEmail(id);
  
  try
  {
    await Task.Delay(3000, cts.Token);
  }
  catch (TaskCanceledException)
  {
    return;
  }
  finally {
    // Only assign _cts = null if another run hasn't already assigned
    // a new token
    if (cts == _cts) _cts = null;
    cts.Dispose();
  }

  await MarkEmailAsRead(id);
}

I didn't test this, but see if it fulfills your requirements.

If the use of cts vs _cts looks confusing to you, it may help to read up on reference types. But the important part is that assigning a new instance to _cts doesn't destroy the object that it used to refer to, and any other part of the code that still has a reference to that object can still use it.

If you've never seen the ?. before, it's the null-conditional operator.

Answer 2

As I understand it, you're essentially restarting a watchdog timer for three seconds every time you click a different email. If the WDT expires without getting a new click then you go ahead and mark the latest email as 'Read'.

For years, I used (successfully) this same approach to cancel the old task when starting a new one for the WDT. But then I realized I could just do this:

int _wdtCount = 0;
// Returning 'void' to emphasize that this should 'not' be awaited!
async void onEmailClicked(int id)
{
    _wdtCount++;
    var capturedCount = _wdtCount;
    await Task.Delay(TimeSpan.FromSeconds(3));   
    // If the 'captured' localCount has not changed after waiting 3 seconds
    // it indicates that no new selections have been made in that time.        
    if(capturedCount.Equals(_wdtCount))
    {
        await MarkEmailAsRead(id);
    }
}

async Task MarkEmailAsRead(int id)
{
    Console.WriteLine($"The email with the ID '{id}' has been marked read");
}

Anyway, works for me and it's just a thought...

TESTBENCH

#region T E S T
onEmailClicked(id: 1);
await Task.Delay(TimeSpan.FromSeconds(1));


onEmailClicked(id: 2);
await Task.Delay(TimeSpan.FromSeconds(1));


onEmailClicked(id: 3);
await Task.Delay(TimeSpan.FromSeconds(4));


onEmailClicked(id: 10);
await Task.Delay(TimeSpan.FromSeconds(2));


onEmailClicked(id: 20);
await Task.Delay(TimeSpan.FromSeconds(1));


onEmailClicked(id: 30);
await Task.Delay(TimeSpan.FromSeconds(4));
#endregion T E S T

---

EDIT

Though I realize that the post no longer has the wording entertaining 'other' options than task cancellation, I still wanted to incorporate the excellent comments and improve my original answer by putting the WatchDogTimer in a class...

class WatchDogTimer
{
    int _wdtCount = 0;
    public TimeSpan Interval { get; set; } = TimeSpan.FromSeconds(1);
    public void ThrowBone(Action action)
    {
        _wdtCount++;
        var capturedCount = _wdtCount;
        Task
            .Delay(Interval)
            .GetAwaiter()
            .OnCompleted(() =>
            {                    
                // If the 'captured' localCount has not changed after 
                // awaiting the Interval, it indicates that no new
                // 'bones' have been thrown during that interval.    
                if (capturedCount.Equals(_wdtCount))
                {
                    action();
                }
            });
    }
}

... and eliminating the async void from the posit.

void onEmailClicked(int id)
    => _watchDog.ThrowBone(() => MarkEmailAsRead(id));

void MarkEmailAsRead(int id)
{
    // BeginInvoke(()=>
    // {
    Console.WriteLine($"Expired: {_stopWatch.Elapsed}");
    Console.WriteLine($"The email with the ID '{id}' has been marked read");
    // });
}

TESTBENCH

#region T E S T
// Same test parameters as before.
System.Diagnostics.Stopwatch _stopWatch = 
    new System.Diagnostics.Stopwatch();
WatchDogTimer _watchDog = 
    new WatchDogTimer { Interval = TimeSpan.FromSeconds(3) };

_stopWatch.Start();
Console.WriteLine(_stopWatch.Elapsed);
onEmailClicked(id: 1);
await Task.Delay(TimeSpan.FromSeconds(1));


Console.WriteLine(_stopWatch.Elapsed);
onEmailClicked(id: 2);
await Task.Delay(TimeSpan.FromSeconds(1));


Console.WriteLine(_stopWatch.Elapsed);
onEmailClicked(id: 3);
await Task.Delay(TimeSpan.FromSeconds(4));


Console.WriteLine(_stopWatch.Elapsed);
onEmailClicked(id: 10);
await Task.Delay(TimeSpan.FromSeconds(2));


Console.WriteLine(_stopWatch.Elapsed);
onEmailClicked(id: 20);
await Task.Delay(TimeSpan.FromSeconds(1));


Console.WriteLine(_stopWatch.Elapsed);
onEmailClicked(id: 30);
await Task.Delay(TimeSpan.FromSeconds(4));
#endregion T E S T

Answer 3

First, I am assuming that your code is single-threaded. The CancellationTokenSource.Dispose method is not thread-safe, so trying to dispose CancellationTokenSources that are shared in a multithreaded environment is a small nightmare. You could check that your code is single-threaded, by searching for Task.Run and .ConfigureAwait(false) in the code that interacts with the CancellationTokenSources. If you find any, you might be in trouble.

Second, you should be aware that the CancellationTokenSource.Cancel method is "jumpy". It immediately invokes all the callbacks that have been registred previously via the CancellationToken.Register route. Code like await Task.Delay(3000, _cts.Token); implicitly registers a callback, so it is possible that the execution flow will jump to the code after the await before continuing with the code after the Cancel. Your code should be prepared for such jumps. You shouldn't assume that the Cancel will complete quickly, nor that it won't throw. In case a registered callback fails, the exception will be surfaced by the Cancel call.

As for the specifics of your code, your mistake is that you interact with the _cts field too much. You should reduce the interactions to the minimum. You should also assume that after an await the _cts will not store the same instance that you assigned before the await. Your code is single-threaded, but asynchronous. Asynchronous code has a complex execution flow. I agree with Gabriel Luci that you should Dispose in one place only. Using async void is not that bad, since it's normal for UI event handlers to be async void. It might bite you when you are done with the application and attempt to close the Window, because you will have no way to wait for the completion of the pending async operations before closing. So you might get annoying errors after closing the Window, because the async operations will try to interact with UI components that have been disposed. Ideally you should store a reference of the latest asynchronous operation (the Task), and await it before starting the next operation, or before closing the Window. But for the purpose of keeping the answer simple I will omit this complexity. Here is my suggestion:

private CancellationTokenSource _cts;

private async void OnEmailClicked(int id)
{
    _cts?.Cancel();

    CancellationTokenSource cts = new();
    try
    {
        _cts = cts;
        await LoadEmail(id);
        try { await Task.Delay(3000, cts.Token); }
        catch (OperationCanceledException) { return; } // Do nothing
    }
    finally
    {
        if (cts == _cts) _cts = null;
        cts.Dispose();
    }
    await MarkEmailAsRead(id);
}