Long-running IAsyncEnumerable<T> leaks the last seen enumerated value.

[hexawyz]

Version Used: .NET 8.0 / C# 12

Steps to Reproduce:

When using async iterators, the "live" last enumerated value will be kept rooted within the IAsyncEnumerator, preventing it from being collected, even though the user code does not explicitly reference it anymore. While at the surface, it may seem like an innocuous problem, this can have severe implications when the object graph behind the enumerated values is quite huge (e.g. collectible assemblies) and the run time of the iterator is very long (think process lifetime).

Also, I believe it can be adressed with a reasonable change. (See the details later)

Run the following C# program:

using System.Runtime.CompilerServices;

using (var cts = new CancellationTokenSource())
{
    ConsoleCancelEventHandler consoleCancelKeyPressEventHandler = (s, e) =>
    {
        if (e.SpecialKey == ConsoleSpecialKey.ControlC)
        {
            cts.Cancel();
        }
    };
    Console.CancelKeyPress += consoleCancelKeyPressEventHandler;
    try
    {
        var collectTask = CollectAfterTenSecondsAsync(cts.Token);
        try
        {
            await foreach (var it in WatchThemAsync(cts.Token))
            {
                Console.WriteLine($"Index #{it.Index}");
                GC.Collect(2, GCCollectionMode.Forced, true);
            }
        }
        finally
        {
            await collectTask;
        }
    }
    finally
    {
        Console.CancelKeyPress -= consoleCancelKeyPressEventHandler;
    }
}

async Task CollectAfterTenSecondsAsync(CancellationToken cancellationToken)
{
    await Task.Delay(10_000, cancellationToken).ConfigureAwait(false);
    GC.Collect(2, GCCollectionMode.Forced, true);
}

async IAsyncEnumerable<WatchMe> WatchThemAsync([EnumeratorCancellation] CancellationToken cancellationToken)
{
    await Task.Yield();
    yield return new(0);
    await Task.Delay(100, cancellationToken).ConfigureAwait(false);
    yield return new(1);
    await Task.Delay(100, cancellationToken).ConfigureAwait(false);
    yield return new(2);
    await Task.Delay(100, cancellationToken).ConfigureAwait(false);
    yield return new(3);
    await Task.Delay(Timeout.Infinite, cancellationToken).ConfigureAwait(false);
    yield return new(4);
}

internal sealed class WatchMe(int index)
{
    public int Index { get; } = index;

    ~WatchMe()
    {
        Console.WriteLine($"Finalized #{Index}.");
    }
}

Expected Behavior:

At some point, the message Finalized #3. would appear.

For example, something like this:

Index #0
Index #1
Finalized #0.
Index #2
Finalized #1.
Index #3
Finalized #2.
Finalized #3.

Or ideally even this:

Index #0
Finalized #0.
Index #1
Finalized #1.
Index #2
Finalized #2.
Index #3
Finalized #3.

Actual Behavior:

The message does not appear, even after the GC.Collect() that is forced about 10s after the start:

Index #0
Index #1
Finalized #0.
Index #2
Finalized #1.
Index #3
Finalized #2.

Related

Related, but AFAIK (slightly) different problems:

• https://github.com/dotnet/runtime/issues/61717 (This one is about an iterator that completes after an await)
• https://github.com/dotnet/roslyn/issues/39321 (This one is about an iterator that completes at some point)

Details

From my understanding, this is related to the fact that the compiler stores the "current" item within the class that is generated to implement the IAsyncEnumerator<T> interface. As the field is never overwriten until a new item is yielded, the old item will be kept in memory for an arbitrary amount of time, without direct control of the user.

One way to address this could be to always explicitly clear up "__current" when the MoveNextAsync method is called: Unless I'm missing something, it would be unsafe for users of the IAsyncEnumerator to access Current when MoveNextAsync is running, so this would likely not break any reasonable code. (The untold assumption being that Current is valid and queryable as many time as needed only until MoveNextAsync() is called again.) The "current" value could still end up being kept alive for longer than we wish if the delay between two MoveNextAsync() calls is excessively long, but at least the control of this would be given back to the application.

PS: I also looked at this to confirm that the "current" value is currently not being cleared: https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA+ABATARgLABQGADAAQY4oDchJ5OAdACoAWsAhgCYCWAdgOY0iZCgFYhGAMzkspAMKEA3oVKry0iknIAOUgEkAggGcAnrzABRXgFcAtjCjtgAGxgAeOQD5SFgBQBKFTVlAjUw+hxyAHZSXhgAd3kAoXC1DABOciQGABEYZ3YTXyZuewhrABcGPV4AMz5uCph/FNSI6NiEpJag1QBfQj6gA== PPS: Of course, when I mention that the "__current" value could be cleared to fix this isse, it should be noted that this can be conditioned on the fact that T contains references. (i.e. unmanaged values don't need clearing)

[dotnet-issue-labeler[bot]]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[dotnet-issue-labeler[bot]]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[jaredpar]

@stephentoub, @jcouv for comment

[jcouv]

A few thoughts:

1. this behavior already exists with enumerables (see example below) but I can see how this sort of situation would be more common with async-enumerables
2. clearing the current value is possible but there is a (small) cost which would only benefit users in this situation. It could either be done in the resumption logic for a yield return (ie. as soon as the current value is no longer useful, in AsyncIteratorMethodToStateMachineRewriter.VisitYieldReturnStatement) or in the suspension logic for an await (where it is most realistic we'd spend time).

I'll let @stephentoub comment on value and trade-off.

    public static IEnumerable<WatchMe> M()
    {
        yield return new WatchMe(1);
        yield return new WatchMe(2); // this item will not get finalized until the time-consuming logic completes
        while (true) { } // time-consuming logic
    }

[stephentoub]

I'll let @stephentoub comment on value and trade-off.

Zero'ing out the Current field when about to suspend at an await seems like a reasonable tradeoff. If the compiler can prove that the T doesn't contain any references, it could skip the zero'ing. Or it could guard the zero'ing with a check on RuntimeHelpers.IsReferenceOrContainsReferences<T>(). Though there's already so much going on during async suspension, as long as the T isn't large, the zero'ing should hopefully not be measurable, anyway.