CancellationTokenSource for changing state after few seconds-CodePudding

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've not done this sort of thing before, so it must be a fundamental problem - 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.)

CodePudding user response：

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 (successfuly) 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

CodePudding user response：

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)
{
  
  try {
    _cts?.Cancel();
  }
  catch (ObjectDisposedException)
  { /* no worries */ }

  // 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 {
    cts.Dispose();
    // Don't assign _cts = null since another run might have already assigned
    // a new token
  }

  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.