Runtime: Rooted objects from IAsyncEnumerable (only in debugger)

[BrennanConroy]

Disclaimer: This only roots objects when in the debugger because of the s_currentActiveTasks dictionary that keeps around incomplete tasks. Which may be changed one day to WeakReferences https://github.com/dotnet/runtime/issues/26565.

This issue is similar to https://github.com/dotnet/roslyn/issues/39321 except that it doesn't "leak" every enumerator, instead it "leaks" occasionally.

The following code will leave 3 objects rooted per "failed async enumerable".

```c#
for (var iteration = 0; iteration < 10000; iteration++)
{
static async IAsyncEnumerable {
await Task.Yield(); // await Task.CompletedTask; will not repro
for (var i = 0; i < 10; ++i)
{
yield return new byte[] { 1 };
}
}

await foreach (var item in func())
{
}

}
```

Took a look at this with @jcouv and we didn't see anything obviously wrong from the compiler side of things. The fix for the linked issue is present in the repro.

cc @bartonjs and @jkotas for routing as Toub is not available.

[msftbot[bot]]

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

[msftbot[bot]]

Tagging subscribers to this area: @tarekgh
See info in area-owners.md if you want to be subscribed.

[tarekgh]

@stephentoub I put this for 6.0 milestone but let me know if it is important enough to bring it back to 5.0.

[stephentoub]

This is the same as https://github.com/dotnet/runtime/issues/26565

[BrennanConroy]

This issue is more about why there is a non-completed task from fairly innocent code.

[stephentoub]

Took a look at this with @jcouv and we didn't see anything obviously wrong from the compiler side of things.

The fix for the linked issue is present in the repro.

Looking in sharplab, I don't see AsyncIteratorMethodBuilder.Complete getting called. Is it? I'm on my phone so maybe I'm just missing it.

https://sharplab.io/#v2:EYLgtghgzgLgpgJwD4AEBMBGAsAKBQBgAIUMBWAblwOIwBZKcqBmYtQgYUIG9dC+aAbMQAchAJIBBKAE8AdgGMAorICuYRBGAAbOAB5g0+AG0AugD5CAMxUKAFAEpe/Hjn5viATmICAdAE0ASzgtABMHckIAekjPbx92AHswAAcdeBCUAQiAdwCtLUJZBJhCBDhkhASndysEhEJbADcIeoDCAF5CfAi23UIMbsIAaiGAx1ca7mrJvhIMYgB2QrhswgNjE25+wgBfBhmd6cOcHaA=

[stephentoub]

Nevermind, search was failing me, I see the complete call now.

[stephentoub]

Won't be at my computer to actually try running it until at least tomorrow. But a question: when you say this only repros under the debugger, does it only repro when actually stepping, or does it repro without ever breaking into the async method?

[BrennanConroy]

F5 no breakpoints.

I assume it's the same if you step through, but since it doesn't leak every enumerator I'm not sure how easy it is to observe the issue like that.

[stephentoub]

Does it repro on 3.1?

[BrennanConroy]

Yes and 5.0

[stephentoub]

Yeah, was just confirming it wasn't a regression. It can wait a few weeks until I'm back then. Thanks.

[stephentoub]

@jcouv, looking at the sharplab repro, I see:

<>v__promiseOfValueOrEnd.SetResult(false);
<>t__builder.Complete();

and similar for the exceptional path. I believe those two lines need to be swapped, such that Complete is called before SetResult. Can you try swapping those in the compiler and see if the issue goes away?

[jcouv]

@stephentoub I will prepare a PR to swap those two calls. But it doesn't look like that's fixing the leak.
There is some variability in repro, but I could still see a leak of similar size with this change.

I've prepared nuget packages of the compiler with change made: https://www.dropbox.com/s/wojznub86kvndhm/compiler-async-leak.zip?dl=0 (to use it, decompress to a local folder, add nuget source referencing this folder, then add compilers toolset package version 3.8.0-dev).

Here's the program I test: https://gist.github.com/jcouv/7142c07e2c4cc2792cdcd7a98c898250 (note there is a warm up loop and calls to GC.Collect() before places that I break and collect data for).

Here's what the IL looks like with change made:

[stephentoub]

Thanks. So what you're saying is I can't fix this all just by staring on my phone :-)

[davidfowl]

Thanks. So what you're saying is I can't fix this all just by staring on my phone :-)

I believe in you. Stare at it longer.

[stephentoub]

Finally made time with a debugger. Fundamentally the problem is too many methods named "MoveNext" :wink:

If anyone wants to play along at home, here's a deterministic repro that doesn't require the debugger be attached (by using private reflection no one should depend on):
```C#
using System;
using System.Collections.Generic;
using System.Reflection;
using System.Threading.Tasks;

public class C
{
static async IAsyncEnumerable Func(TaskCompletionSource tcs)
{
await tcs.Task;
yield return 1;
}

static void Main()
{
    typeof(Task).GetField("s_asyncDebuggingEnabled", BindingFlags.NonPublic | BindingFlags.Static).SetValue(null, true);

    for (int i = 0; i < 1000; i++)
    {
        TaskCompletionSource tcs = new();
        IAsyncEnumerator<int> e = Func(tcs).GetAsyncEnumerator();
        Task t = e.MoveNextAsync().AsTask();
        tcs.SetResult();
        t.Wait();
        e.MoveNextAsync().AsTask().Wait();
    }

    Console.WriteLine(((dynamic)typeof(Task).GetField("s_currentActiveTasks", BindingFlags.NonPublic | BindingFlags.Static).GetValue(null)).Count);
}

}
```

The state machine task is removed from the static tracking collection here:
https://github.com/dotnet/runtime/blob/722b8a660e7e0c59419196be102784244883ff08/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncTaskMethodBuilderT.cs#L335-L341
This AsyncStateMachineBox.MoveNext method is the invoked in response to an await completing. So, for example, in the above repro, it’ll be invoked when the tcs.Task completes, causing this MoveNext to be called, which in turn will call StateMachine.MoveNext to invoke the compiler-generated MoveNext method for the iterator. The iterator will proceed to set the current value as 1 and complete the ValueTask<bool> returned from the IAsyncEnumerable<int>.MoveNextAsync call. All’s good so far.

But, then the next call to e.MoveNextAsync() calls to AsyncIteratorMethodBuilder.MoveNext(ref stateMachine) which effectively just invoked StateMachine.MoveNext directly; there’s no more work to be done, so it just completes the call synchronously, and we never end up calling into the AsyncStateMachineBox.MoveNext again, which means we never execute that code path which checks to see whether the task has completed and removes it from tracking.

I need to think about the cleanest way to address this.