Async Code Leaks in 2.1 Runtime, but Not 2.0 Runtime

[kriskalish]

I have encountered an issue a specific instance of my code leaks when the .NET Core runtime is upgraded from 2.0 to 2.1. While the original code is too complex to post, I was able to reproduce the issue with a distilled example.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;

namespace MemoryTester
{
    class Program
    {
        static void Main(string[] args)
        {
            List<Task<long>> tasks = new List<Task<long>>();

            for (var i = 0; i < 2000; i++)
            {
                var task = MockWork(new byte[1024 * 1024]);
                tasks.Add(task);
                task.Wait();

                GC.Collect(GC.MaxGeneration, GCCollectionMode.Forced, true);
                GC.WaitForPendingFinalizers();
            }

            var all = Task.WhenAll(tasks).GetAwaiter().GetResult();

            Console.WriteLine($"Finished working on {all.Sum()} bytes.");
        }

        static async Task<long> MockWork(byte[] bytes)
        {
            await Task.Delay(10);
            await Task.Yield();
            return bytes.Length;
        }
    }
}

I can see how it would leak, if MockWork is compiled into a state machine with a property representing bytes, then the fact that Main holds a reference to all the tasks would prevent it from garbage collecting those byte arrays. However, I find it curious that this style of code works in 2.0 which leads me to believe that it nulls out unused fields in the state machine when the task is complete.

All in all, my question is this leak expected behavior?

For what it's worth, I suspect that the change in behavior may be related to these two PRs for 2.1:
dotnet/coreclr#13105, dotnet/coreclr#14178

[stephentoub]

It's expected.

In all versions before .NET Core 2.1, the Task object and the boxed state machine were completely separate, with the state machine boxed object referencing the Task, but not vice versa. So, if you held onto the Task, you weren't keeping the state machine object alive, and if everything else dropped its references to the state machine object, it would be GC'd. That would generally happen when the async method completed.

In .NET Core 2.1, the Task object and the boxed async state machine are one-in-the-same, with the async machinery creating a Task-derived type that stores the state machine onto it as a field. This then means that keeping the Task alive also keeps the state machine alive, because the state machine is just a set of fields on the Task. In addition to the reduced allocations and performance improvements the approach provides, it also yields some significant debuggability benefits, both in terms of making it easier to find the state machine object in, for example, the VS debugger, but also in being able to traverse backwards through an await chain in the debugger. It does, however, have this negative side effect you mention: while the vast, vast majority of cases involve doing "await SomeAsyncMethod()" and thus the Task being dropped as soon as the await completes anyway, if it's a case like yours where the state machine is holding onto state and the Task is then kept alive beyond its completion, that state machine state will also be kept alive.

If we wanted to address this, the solution is pretty easy: we can just null out the state machine state when the task completes, something like stephentoub/coreclr@0aec1a6. The downside to that is a small perf impact (probably negligable, but it would need to be measured), and a small debugging impact, in that you could no longer observe the state of the state machine once it completed. I opted not to clear it, but if it became a problem, obviously it could be done.

How big an issue is this for you?

[kriskalish]

Thanks @stephentoub, your response gives me the background necessary to understand the issue and it makes a sense to me.

This is not a big issue for me because I can rework my code to drop references to the Task objects as they complete (I'm actually processing them in chunks). In reality I was only using the Task objects as a convenient way to grab the results from all completed Tasks. However, I can accumulate results in my own collection and call it a day.

This change does make me want to turn a critical eye to any place I have used Task.WhenAll() or even Task.WhenAny() in my code base. I'll have to review.

In any case, I will close this issue because it was merely a question that has been answered. I appreciate the prompt response!

[stephentoub]

I measured the perf impact, and it's negligible. Given that, while this is a relatively rare use case, it still seems like the potential for leaks here outweighs the small debugging benefits the non-clearing could provide, so I'm making the fix.