StephenCleary / AsyncEx

A helper library for async/await.
MIT License
3.49k stars 358 forks source link

The AsyncLazy<T> does not release resources closed over by the factory delegate #256

Open theodorzoulias opened 2 years ago

theodorzoulias commented 2 years ago

Description

Hi! I am reporting an issue that I discovered after reading a comment by Servy here. In case the factory delegate of an AsyncLazy<T> instance closes over some expensive resource, this resource will not be eligible for garbage collection after the completion of the delegate.

Reproduction Steps

public static async Task Main()
{
    Console.WriteLine($"TotalMemory: {GC.GetTotalMemory(true):#,0} bytes");
    var lazy = new Nito.AsyncEx.AsyncLazy<int>(GetFunction());
    Console.WriteLine($"AsyncLazy result: {await lazy:#,0}");
    await Task.Yield();
    GC.Collect();
    Console.WriteLine($"TotalMemory: {GC.GetTotalMemory(true):#,0} bytes");
    GC.KeepAlive(lazy);

    static Func<Task<int>> GetFunction()
    {
        byte[] bytes = new byte[20_000_000];
        return new Func<Task<int>>(async () =>
        {
            await Task.Delay(200);
            return bytes.Length;
        });
    }
}

Output:

TotalMemory: 86,192 bytes
AsyncLazy result: 20,000,000
TotalMemory: 20,133,704 bytes

Online demo.

Expected behavior

The GC.GetTotalMemory() after awaiting the AsyncLazy<T> should be roughly the same as before.

Actual behavior

The GC.GetTotalMemory() after awaiting the AsyncLazy<T> is around 20 MB more than before.

Configuration

Other information

After switching to a simpler AsyncLazy<T> implementation like the one below, the problem is not reproduced:

private class AsyncLazySimple<T>
{
    private readonly Lazy<Task<T>> _lazyTask;
    public AsyncLazySimple(Func<Task<T>> factory) => _lazyTask = new(() => factory());
    public Task<T> Task => _lazyTask.Value;
    public TaskAwaiter<T> GetAwaiter() => Task.GetAwaiter();
}

Output:

TotalMemory: 86,192 bytes
AsyncLazy result: 20,000,000
TotalMemory: 133,728 bytes
DaveVdE commented 1 year ago

Does this reproduce the problem?

public static async Task Main()
{
    Console.WriteLine($"TotalMemory: {GC.GetTotalMemory(true):#,0} bytes");
    var lazy = new Nito.AsyncEx.AsyncLazy<int>(GetFunction);
    Console.WriteLine($"AsyncLazy result: {await lazy:#,0}");
    await Task.Yield();
    GC.Collect();
    Console.WriteLine($"TotalMemory: {GC.GetTotalMemory(true):#,0} bytes");
    GC.KeepAlive(lazy);

    static Task<int> GetFunction()
    {
        byte[] bytes = new byte[20_000_000];
        await Task.Delay(200);
        return bytes.Length;
    }
}
theodorzoulias commented 1 year ago

@DaveVdE no, it doesn't. The new byte[20_000_000] in your example is not captured by a lambda, like in my example. Btw the GetFunction() in your example returns a Task, not a Func, so it's not properly named. It is also missing the async keyword.

DaveVdE commented 1 year ago

Sure, it's missing the async keyword, and I didn't change the name.

Oh I see, your point is that the function passed to the AsyncLazy constructor should be forgotten after the task has been complete.

theodorzoulias commented 1 year ago

@DaveVdE yep, as it does the native Lazy<T> class.

timcassell commented 1 year ago

I sent a fix #269.