Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
terrajobst opened this issue Mar 19, 2020 · 13 comments
Open

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

terrajobst opened this issue Mar 19, 2020 · 13 comments
Labels
api-approved API was approved in API review, it can be implemented area-System.Threading.Tasks code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@terrajobst
Copy link
Member

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

@terrajobst terrajobst added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Threading.Tasks code-analyzer Marks an issue that suggests a Roslyn analyzer labels Mar 19, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Mar 19, 2020
@jeffhandley jeffhandley added this to the Future milestone Mar 20, 2020
@jeffhandley
Copy link
Member

Estimates:

  • Analyzer: Medium
  • Fixer: Not Applicable

@terrajobst terrajobst removed the untriaged New issue has not been triaged by the area owner label Mar 21, 2020
@stephentoub
Copy link
Member

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
Copy link

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
Copy link
Member

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.

@Mrnikbobjeff
Copy link

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
Copy link
Member

carlossanlop commented Nov 20, 2020

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

@stephentoub
Copy link
Member

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

@carlossanlop
Copy link
Member

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?

@carlossanlop carlossanlop added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Apr 13, 2021
@stephentoub
Copy link
Member

There are lots of variations to the erroneous pattern, so it might be hard to flag them all as well as come up with a good fixer. But we could probably just flag cases where Task.Delay(timeout) (no cancellation token argument) is passed to Task.WhenAny with just one other task, e.g.

Task.WhenAny(task, Task.Delay(timeout))

and suggest that it's likely an issue and Task.WaitAsync should be used instead.

@buyaa-n
Copy link
Contributor

buyaa-n commented Dec 13, 2021

Based on the above comments the analyzer/fixer behavior could look like this:

  • Suggested severity: Info
  • Suggested category: Performance
  • Suggested milestone: Future
public static void CancellationNotProvidedFlag(Task task, int timeout)
{
    Task.WhenAny(task, Task.Delay(timeout)); // warn
}

public static void CancellationNotProvidedAfterFix(Task task, int timeout)
{
    task.WaitAsync(TimeSpan.FromMilliseconds(timeout));
}
public static void CancellationNotProvidedFlag(Task task, TimeSpan timeout)
{
    Task.WhenAny(task, Task.Delay(timeout)); // warn
}

public static void CancellationNotProvidedAfterFix(Task task, TimeSpan timeout)
{
    task.WaitAsync(timeout);
}
public static void CancellationProvidedButNotPassedFlag(Task task, int timeout, CancellationToken token)
{
    Task.WhenAny(task, Task.Delay(timeout)); // warn
}

public static void CancellationProvidedButNotPassedAfterFix(Task task, int timeout, CancellationToken token)
{
    task.WaitAsync(TimeSpan.FromMilliseconds(timeout), token);
}
public static void CancellationProvidedButNotPassedFlag(Task task, TimeSpan timeout, CancellationToken token)
{
    Task.WhenAny(task, Task.Delay(timeout));
}

public static void CancellationProvidedButNotPassedAfterFix(Task task, TimeSpan timeout, CancellationToken token)
{
    task.WaitAsync(timeout, token);
}
// I assume if the `CancellationToken` is passed into `Task.Delay` as needed we should not flag
public static void CancellationProvidedAndPassedDoNotFlag(Task task, int timeout, CancellationToken token)
{
    Task.WhenAny(task, Task.Delay(timeout, token)); // no warning
}

public static void CancellationProvidedAndPassedDoNotFlag(Task task, TimeSpan timeout, CancellationToken token)
{
    Task.WhenAny(task, Task.Delay(timeout, token)); // no warning
}

cc @carlossanlop @stephentoub

@buyaa-n buyaa-n removed the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Dec 13, 2021
@ghost ghost added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Feb 1, 2022
@carlossanlop carlossanlop added api-ready-for-review API is ready for review, it is NOT ready for implementation code-fixer Marks an issue that suggests a Roslyn code fixer and removed needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration labels Feb 1, 2022
@teo-tsirpanis teo-tsirpanis removed the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 8, 2022
@stephentoub stephentoub changed the title Pass cancellation token to Task.Delay when used with Task.WhenAny Detect non-cancelable Task.Delay passed to Task.WhenAny Mar 15, 2022
@bartonjs
Copy link
Member

bartonjs commented Sep 6, 2022

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

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Sep 6, 2022
@buyaa-n buyaa-n added the help wanted [up-for-grabs] Good issue for external contributors label Sep 6, 2022
@buyaa-n buyaa-n modified the milestones: Future, 8.0.0 Nov 11, 2022
@buyaa-n
Copy link
Contributor

buyaa-n commented Nov 16, 2022

Estimates:

  • Analyzer: Medium
  • Fixer: Medium

@mpidash
Copy link
Contributor

mpidash commented Aug 19, 2023

@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
  1. W:\aspnetcore\src\Components\test\E2ETest\ServerExecutionTests\CircuitGracefulTerminationTests.cs(57,15): info CA2022: Do not use 'WhenAny' with a noncancelable Task created by 'Delay'
  2. W:\aspnetcore\src\Components\test\E2ETest\ServerExecutionTests\CircuitGracefulTerminationTests.cs(70,15): info CA2022: Do not use 'WhenAny' with a noncancelable Task created by 'Delay'
  3. W:\aspnetcore\src\Components\test\E2ETest\ServerExecutionTests\CircuitGracefulTerminationTests.cs(83,26): info CA2022: Do not use 'WhenAny' with a noncancelable Task created by 'Delay'
  4. W:\aspnetcore\src\Components\test\E2ETest\ServerExecutionTests\CircuitGracefulTerminationTests.cs(97,15): info CA2022: Do not use 'WhenAny' with a noncancelable Task created by 'Delay'
  5. W:\aspnetcore\src\Components\test\E2ETest\ServerExecutionTests\CircuitGracefulTerminationTests.cs(110,15): info CA2022: Do not use 'WhenAny' with a noncancelable Task created by 'Delay'
  6. W:\aspnetcore\src\Components\test\E2ETest\ServerExecutionTests\CircuitGracefulTerminationTests.cs(123,15): info CA2022: Do not use 'WhenAny' with a noncancelable Task created by 'Delay'
  7. W:\aspnetcore\src\Hosting\Hosting\test\WebHostTests.cs(228,38): info CA2022: Do not use 'WhenAny' with a noncancelable Task created by 'Delay'
  8. 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'
  9. 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'
  10. 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'
  11. 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'
  12. 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'
  13. 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'
  14. W:\aspnetcore\src\Shared\Buffers.MemoryPool\DiagnosticMemoryPool.cs(157,26): info CA2022: Do not use 'WhenAny' with a noncancelable Task created by 'Delay'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-System.Threading.Tasks code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests