VSTHRD002: Don't warn when getting Result on completed task

[KristianWedberg]

Bug description

VSTHR002 warns even though the task has been checked for completion or have been awaited.

It should not warn in these cases, since the task is provably completed, and Result therefore
is safe to use.

Not warning is important since the below is a common performance optimization - retrieving
Result on a completed task is roughly 2x faster than awaiting the completed task.

Edited: As @AArnott points out: guarding with IsCompleted will wrap completed exceptions
in an AggregateException. To avoid this, instead guard with IsCompletedSuccessfully (ValueTask
or .Net Core 2.x) or task.Status == TaskStatus.RanToCompletion (.Net Framework), since
a successfully completed task won't have any exceptions.

Repro steps

var task = MethodAsync();
if (!task.IsCompleted)
    await task.ConfigureAwait(false);
var useTaskResult = task.Result;

Expected behavior

VSTHR002 does not trigger when the task has been checked as completed via:

• IsCompleted
• IsCanceled
• IsFaulted
• IsCompletedSuccessfully (ValueTask or .Net Core 2.x)
• awaited

Actual behavior

VSTHR002 warns even though the task has been checked for completion or have been awaited.

• Version used: 15.7
• Application (if applicable): .Net Framework 4.6.1, C#7.2

[AArnott]

Yes, I agree we should be smarter for this analyzer and not report where it's clear from code inspection of the method that the task has already completed. We've done some work in this area already (e.g. #219). What you ask for generally requires execution flow analysis, which the Roslyn API does not yet offer but I hear is coming. Our plan has been to address it then. However some common cases like what you pointed out could probably be hard-coded to be recognized and suppress the warning.

Not warning is important since the below is a common performance optimization - retrieving
Result on a completed task is roughly 2x faster than awaiting the completed task.

I think you should double-check your perf test here, because the C# code you wrote is virtually equivalent to the code that the C# compiler itself emits for an await. It literally checks IsCompleted first and accesses the Result synchronously when it is already completed and only schedules the continuation when it is not complete.
Also note that your "optimization" does change behavior in the failure case, leading to non-deterministic results. If task.IsCompleted is true when you check it, you'll call task.Result and throw an AggregateException. But when task.IsCompleted is false when you check it, then when you await you'll end up throwing the inner exception of the AggregateException (most likely a different exception type). Your race condition could lead to your exception handlers not firing consistently.
While you could solve the race condition by changing your task.Result call to task.GetAwaiter().GetResult() (which only differs in which exception is thrown), I suggest you use await consistently and trust the compiler to do that if check for you.

[KristianWedberg]

@AArnott All good points, my repo above was minimalistic.

On performance, the differences certainly only matter with high volume and when a completed task is much (in my case +10,000 times) more common than an incomplete task. In my tests below, without ConfigureAwait() the difference is a negligible 15%. In my library though I use ConfigureAwait(false) everywhere, which makes it over 400% slower, and the ability to avoid that is (for me) hard to ignore. Yes one can add additional APIs to partly mitigate it, but multiple APIs for the same thing has its own drawbacks. If I'm missing something obvious though, I would be all ears :-)

EDIT: Added ConfigureAwait_GetAwaiter_Async(), showing that it's the ConfigureAwait(false) call that gives the +400% slowdown (not the await.)

BenchmarkDotNet=v0.10.14, OS=Windows 7 SP1 (6.1.7601.0)
Intel Core i7-4770K CPU 3.50GHz (Haswell), 1 CPU, 4 logical and 4 physical cores
Frequency=3417031 Hz, Resolution=292.6517 ns, Timer=TSC
  [Host]     : .NET Framework 4.7.1 (CLR 4.0.30319.42000), 64bit RyuJIT-v4.7.2650.0
  DefaultJob : .NET Framework 4.7.1 (CLR 4.0.30319.42000), 64bit RyuJIT-v4.7.2650.0

Method	Mean	Error	StdDev	Scaled	ScaledSD
ConfigureAwait_GetAwaiter_Async	11.491 ns	0.0909 ns	0.0850 ns	4.40	0.03
Await_ConfigureAwait_Async	10.748 ns	0.0398 ns	0.0372 ns	4.12	0.01
Await_Async	2.996 ns	0.0077 ns	0.0072 ns	1.15	0.00
Result_Async	2.610 ns	0.0046 ns	0.0038 ns	1.00	0.00
GetAwaiter_Async	2.540 ns	0.0057 ns	0.0050 ns	0.97	0.00

BenchmarkDotNet

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System.Threading.Tasks;

namespace VSThreading301
{
    [BenchmarkDotNet.Attributes.DisassemblyDiagnoser(printAsm: false, printIL: true)]
    public class BenchCompletedTask
    {
        private volatile Task<int> _completedTask = Task.FromResult(1);
        const int Operations = 100000;

        [Benchmark(OperationsPerInvoke = Operations)]
        public async Task<int> ConfigureAwait_GetAwaiter_Async()
        {
            await _completedTask.ConfigureAwait(false);
            int x = 0;
            for (int i = 0; i < Operations; i++)
            {
                if (!_completedTask.IsCompleted)
                    await _completedTask.ConfigureAwait(false);
                x += _completedTask.ConfigureAwait(false).GetAwaiter().GetResult();
            }
            return x;
        }

        [Benchmark(OperationsPerInvoke = Operations)]
        public async Task<int> Await_ConfigureAwait_Async()
        {
            await _completedTask.ConfigureAwait(false);
            int x = 0;
            for (int i = 0; i < Operations; i++)
            {
                x += await _completedTask.ConfigureAwait(false);
            }
            return x;
        }

        [Benchmark(OperationsPerInvoke = Operations)]
        public async Task<int> Await_Async()
        {
            await _completedTask.ConfigureAwait(false);
            int x = 0;
            for (int i = 0; i < Operations; i++)
            {
                x += await _completedTask;
            }
            return x;
        }

        [Benchmark(OperationsPerInvoke = Operations, Baseline = true)]
        public async Task<int> Result_Async()
        {
            await _completedTask.ConfigureAwait(false);
            int x = 0;
            for (int i = 0; i < Operations; i++)
            {
                if (!_completedTask.IsCompleted)
                    await _completedTask.ConfigureAwait(false);
                x += _completedTask.Result;
            }
            return x;
        }

        [Benchmark(OperationsPerInvoke = Operations)]
        public async Task<int> GetAwaiter_Async()
        {
            await _completedTask.ConfigureAwait(false);
            int x = 0;
            for (int i = 0; i < Operations; i++)
            {
                if (!_completedTask.IsCompleted)
                    await _completedTask.ConfigureAwait(false);
                x += _completedTask.GetAwaiter().GetResult();
            }
            return x;
        }
    }

    public class Program
    {
        public static void Main(string[] args)
        {
            var summary = BenchmarkRunner.Run<BenchCompletedTask>();
        }
    }
}

[AArnott]

Have you filed a bug over at dotnet/corefx for the slowness from .ConfigureAwait(false)? 400% slowdown seems incredibly high for a completed task.

Also, if you own the async method you're calling, given it usually is completed before returning have you considered using ValueTask<T> instead of Task<T>? That will avoid an allocation (although perhaps you're already caching a Task instance yourself) which can help throughput.

[KristianWedberg]

Good suggestion, I'll file it.

Yes, I use cached tasks. And ValueTask<T> has certainly been a life saver in some other places!

Cheers,
Kristian

[KristianWedberg]

Filed as https://github.com/dotnet/corefx/issues/30655, with additional tests and detail.

[sharwell]

💭 A CompletedResult extension method which throws if the task is not completed would help write clear (readable) code and also avoid this issue.

[AArnott]

I like it. In fact I already have such an extension method (I think I called it GetResultAssumeCompleted()) for my own code clarify and enforcement. Adding such a method to vs-threading may be a good idea. But we might make the analyzer allow any method with that name regardless of where it comes from so that it can work on downlevel versions of VS.

[markusschaber]

We have another use case, apart from performance optimization: I use Task.IsCompletedSuccessfully to conditionally use Task.Result when I could already already use the result in case the task is finished, but I do not want to actively wait for the task to finish, as there are other things keeping the current task busy.