Runtime: Pass cancellation token to Task.Delay when used with Task.WhenAny

[terrajobst]

Flag places where a Task.Delay is used as an argument to WhenAny and where that Task.Delay doesn't take a cancellation token, in which case the Task.Delay is likely leaving a timer running for longer than is necessary.

Category: Performance

[jeffhandley]

Estimates:

• Analyzer: Medium
• Fixer: Not Applicable

[stephentoub]

Fixer: Not Applicable

I think a fixer could reasonably be done here.

Also, it's such a common pattern, we could consider introducing new overloads to simplify it.

[Mrnikbobjeff]

Fixer: Not Applicable

I think a fixer could reasonably be done here.

Also, it's such a common pattern, we could consider introducing new overloads to simplify it.

Would a fixer simply check if the current method has a cancellationtoken parameter? Otherwise where would our cancellationtoken come from?

[stephentoub]

Would a fixer simply check if the current method has a cancellationtoken parameter? Otherwise where would our cancellationtoken come from?

The fixer would manufacture it. Essentially this:
```C#
if (task != await Task.WhenAny(Task.Delay(timeout), task))
throw new TimeoutException();

would become
```C#
using (var cts = new CancellationTokenSource())
{
    Task t = await Task.WhenAny(Task.Delay(timeout, cts.Token), task);
    cts.Cancel();
    if (t != task)
        throw new TimeoutException();
}

or something along those lines.

That said, a better approach might instead be to just add an API like https://github.com/dotnet/runtime/issues/27723, as I expect that represents the 99% case for this pattern, and then a fixer here could offer to just switch to use that API if it's available.

[Mrnikbobjeff]

I believe the new API would be vastly superior as the intent is perfectly clear, while analysing the longer snippet costs some time. I would leave the fixer blank for now as the earliest an analyzer could ship appears to be .net 6 and I would hope the Task Timeout extension method becomes standardised by then.

[carlossanlop]

@Mrnikbobjeff @stephentoub this analyzer seems to be already covered with the one I wrote (CA2016), which passes a CancellationToken to methods that can take it: https://github.com/dotnet/runtime/issues/33774 (PR)

The case that analyzer CA2016 wouldn't cover is the one shared by Stephen in the example above, where we detect a newly created CancellationTokenSource in the same context and then pass its underlying cts.Token to the method that can take it.

We could enhance CA2016 to cover that case as well. I don't think we need another analyzer for this. Anyone thinks differently?

[stephentoub]

This is different from CA2016. In fact if CA2016 fired for the shown pattern, it would be doing the wrong thing (or at least it wouldn't be doing as well as it should). The idiom being used is simply trying to time out an await, and using a Task.Delay combined with a Task.WhenAny to do it, but then not disposing of the resources created for that timeout, namely the timer underneath the Task.Delay. So a dedicated CancellationTokenSource needs to be created that can be used to cancel the Task.Delay after the operation completes.

I think the right answer here is shipping some form of https://github.com/dotnet/runtime/issues/27723, and then the analyzer would flag patterns like await Task.WhenAny(task, Task.Delay(timeout)) and recommend switching to that new API.
cc: @eiriktsarpalis