Detect non-cancelable Task.Delay passed to Task.WhenAny

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

Estimates:

Analyzer: Medium
Fixer: Not Applicable

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.

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?

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:

if (task != await Task.WhenAny(Task.Delay(timeout), task))
    throw new TimeoutException();

would become

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 #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.

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.

@Mrnikbobjeff

@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: #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?

@eiriktsarpalis

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 #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

@stephentoub

So there were a bunch of APIs that got approved and them merged (Mar 11) to help with tasks and timeouts.

Based on this comment:

I think the right answer here is shipping some form of ~~#27723~~ #47525, and then the analyzer would flag patterns like await Task.WhenAny(task, Task.Delay(timeout)) and recommend switching to that new API.

Maybe there's more than one pattern this analyzer could potentially flag. @stephentoub @eiriktsarpalis would you mind helping determine the cases to flag and what the fixer should suggest?

Video

Seems good as proposed. Steve and Stephen pointed out that it's eventually a problem beyond performance, so the category is now Reliability.

Analyzer to detect when Task.Delay with no cancellation token is used in Task.WhenAny, using the next available diagnostic ID.
A fixer which suggests to use WaitAsync with a timeout over the WhenAny approach, in the cases where it is safe to do so (and the TFM has WaitAsync available).
The docs page for the diagnostic should demonstrate with "with cancellation token" pattern for older TFMs, and perhaps talk about when some other token is available.

Severity: Info
Category: Reliability

Estimates:

Analyzer: Medium
Fixer: Medium

@buyaa-n

@buyaa-n I've ran a prototype analyzer against dotnet/aspnetcore to see how the pattern is used, and I am not so sure if a fixer makes sense for this analyzer:

Replacing WhenAny with WaitAsync changes the behavior, as WaitAsync fails the Task with a TimeoutException.
To preserve non-throwing, the exception must be caught, but for test code a simple replacement might be ok (though catching the exception and failing with a more descriptive message is probably preferable):

[Fact]
public async Task ReloadingThePage_GracefullyDisconnects_TheCurrentCircuit()
{
    // Arrange & Act
    Browser.Navigate().Refresh();
    await Task.WhenAny(Task.Delay(10000), GracefulDisconnectCompletionSource.Task);

    // Assert
    Assert.Contains((Extensions.Logging.LogLevel.Debug, "CircuitTerminatedGracefully"), Messages.ToArray());
    Assert.Contains((Extensions.Logging.LogLevel.Debug, "CircuitDisconnectedPermanently"), Messages.ToArray());
}

But WaitAsync also states that the returned task could be a different task than the instance:

Returns
Task
The Task representing the asynchronous wait. It may or may not be the same instance as the current instance.

So a simple replacement in the following cases could lead to broken code:

var task = await Task.WhenAny(_allBlocksReturned.Task, Task.Delay(timeout));
if (task != _allBlocksReturned.Task)

or

// Verify that the response isn't flushed by verifying the TCS isn't set
var res = await Task.WhenAny(tcs.Task, Task.Delay(1000)) == tcs.Task;
Assert.False(res);

or

if (await Task.WhenAny(exitedTcs.Task, Task.Delay(TimeSpan.FromSeconds(TimeoutSeconds * 2))) != exitedTcs.Task)
{
    try
    {
        process.Kill();
    }
    catch (Exception ex)
    {
        throw new TimeoutException($"h2spec didn't exit within {TimeoutSeconds * 2} seconds.", ex);
    }
    throw new TimeoutException($"h2spec didn't exit within {TimeoutSeconds * 2} seconds.");
}

or

public async Task WhenAllBlocksReturnedAsync(TimeSpan timeout)
{
    var task = await Task.WhenAny(_allBlocksReturned.Task, Task.Delay(timeout));
    if (task != _allBlocksReturned.Task)
    {
        MemoryPoolThrowHelper.ThrowInvalidOperationException_BlocksWereNotReturnedInTime(_totalBlocks - _blocks.Count, _totalBlocks, _blocks.ToArray());
    }

    await task;
}

Most of these examples could be written more clearly with a bit more (individual) refactoring, so I am not sure if a fixer could do this automatically. Also seen in the changes in #48842.

All findings

W:\aspnetcore\src\Components\test\E2ETest\ServerExecutionTests\CircuitGracefulTerminationTests.cs(57,15): info CA2022: Do not use 'WhenAny' with a noncancelable Task created by 'Delay'
W:\aspnetcore\src\Components\test\E2ETest\ServerExecutionTests\CircuitGracefulTerminationTests.cs(70,15): info CA2022: Do not use 'WhenAny' with a noncancelable Task created by 'Delay'
W:\aspnetcore\src\Components\test\E2ETest\ServerExecutionTests\CircuitGracefulTerminationTests.cs(83,26): info CA2022: Do not use 'WhenAny' with a noncancelable Task created by 'Delay'
W:\aspnetcore\src\Components\test\E2ETest\ServerExecutionTests\CircuitGracefulTerminationTests.cs(97,15): info CA2022: Do not use 'WhenAny' with a noncancelable Task created by 'Delay'
W:\aspnetcore\src\Components\test\E2ETest\ServerExecutionTests\CircuitGracefulTerminationTests.cs(110,15): info CA2022: Do not use 'WhenAny' with a noncancelable Task created by 'Delay'
W:\aspnetcore\src\Components\test\E2ETest\ServerExecutionTests\CircuitGracefulTerminationTests.cs(123,15): info CA2022: Do not use 'WhenAny' with a noncancelable Task created by 'Delay'
W:\aspnetcore\src\Hosting\Hosting\test\WebHostTests.cs(228,38): info CA2022: Do not use 'WhenAny' with a noncancelable Task created by 'Delay'
W:\aspnetcore\src\Middleware\Spa\SpaServices.Extensions\src\Util\TaskTimeoutExtensions.cs(10,27): info CA2022: Do not use 'WhenAny' with a noncancelable Task created by 'Delay'
W:\aspnetcore\src\Servers\HttpSys\test\FunctionalTests\Listener\Utilities.cs(130,35): info CA2022: Do not use 'WhenAny' with a noncancelable Task created by 'Delay'
W:\aspnetcore\src\Servers\Kestrel\Core\src\Internal\Infrastructure\TransportConnectionManager.cs(86,22): info CA2022: Do not use 'WhenAny' with a noncancelable Task created by 'Delay'
W:\aspnetcore\src\Servers\Kestrel\test\InMemory.FunctionalTests\Http2\Http2StreamTests.cs(1602,29): info CA2022: Do not use 'WhenAny' with a noncancelable Task created by 'Delay'
W:\aspnetcore\src\Servers\Kestrel\test\InMemory.FunctionalTests\ResponseTests.cs(3106,29): info CA2022: Do not use 'WhenAny' with a noncancelable Task created by 'Delay'
W:\aspnetcore\src\Servers\Kestrel\test\Interop.FunctionalTests\H2SpecCommands.cs(254,23): info CA2022: Do not use 'WhenAny' with a noncancelable Task created by 'Delay'
W:\aspnetcore\src\Shared\Buffers.MemoryPool\DiagnosticMemoryPool.cs(157,26): info CA2022: Do not use 'WhenAny' with a noncancelable Task created by 'Delay'

Navigation Menu

Repository navigation

Detect non-cancelable Task.Delay passed to Task.WhenAny #33805

8 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Detect non-cancelable Task.Delay passed to Task.WhenAny #33805

Description

Activity

jeffhandley commented on Mar 21, 2020

stephentoub commented on Mar 21, 2020

Mrnikbobjeff commented on Sep 2, 2020

stephentoub commented on Sep 14, 2020

Mrnikbobjeff commented on Sep 16, 2020

carlossanlop commented on Nov 20, 2020

stephentoub commented on Nov 20, 2020

carlossanlop commented on Apr 13, 2021

8 remaining items

bartonjs commented on Sep 6, 2022

buyaa-n commented on Nov 16, 2022

mpidash commented on Aug 19, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions