Skip to content

New rule reliability/maintainability: Do await incomplete Tasks before returning and implicitly running other code #85175

Closed as not planned
@TimLovellSmith

Description

@TimLovellSmith

Approximate match on rule categories to put it in:

Usage: CA1801, CA1806, CA1816, CA2200-CA2209, CA2211-CA2260
Reliability: CA9998-CA9999, CA2000-CA2021
MicrosoftCodeAnalysisCorrectness: RS1000-RS1999
MicrosoftCodeAnalysisDesign: RS1000-RS1999
RoslynDiagnosticsMaintainability: RS0000-RS0999

Suggested rule name: await asynchronous methods when returning a Task directly.
Rationale: 'goes-out-of-scope' logic like 'using statement' may run sooner than you expect.

Describe the problem you are trying to solve

The following code is 'clearly a bug waiting to happen' to the eyes of some C# experts.

public Task GetHealthStatusAsync(CancellationToken cancellationToken = default)
{
    using IDisposable disposable = new System.IO.FileStream(); // implicit cleanup at end of scope
    return GetCdsProfileAsync(FromInstrumentationKey(_testIKey), cancellationToken); // return without await
} 

What we want to prevent, is people writing code with signature Task, that calls a Task method, without doing await, but just returning that Task directly, and then having automatic cleanup run because the 'synchronous' scope ends.

i.e. the using statement would dispose the object created even though the chained async call won't be completed yet.

This bug will be non-deterministic based on whether the chained async call actually completes immediately or not.

Describe suggestions on how to achieve the rule

Step 1: filter based on method signature. Rule only analyzes methods that return type Task or ValueTask, but do not use the 'async' keyword to implement an async state machine.
Step 2: filter based on method implementation. Rule only fires for methods that
a) return a Task object which isn't obviously going to complete immediately. Ones that are obviously always going to return a completed task include, 'Task.CompletedTask, Task.FromResult, Task.FromException', or have previously been provably completed using e.g. task.Wait(), task.Result.
b) have code that will run implicitly after the return statement. Because of using statements, finally clauses, or anything similar.

Recommended (automatable) fix:

Convert method to do async/await instead.

Additional context

With kudos to Paul Harrington for giving a talk mentioning this antipattern.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions