Potential memory leak in AsyncDictionaryHelper

[jnyrup]

When calling GetItemAsync we add tcs to an entry in MultiMap, but when removing a tcs, we don't remove empty entries in MultiMap.
When the wrapped ConcurrentDictionary has an item removed or is cleared, I believe we should accordingly update _pendingRequests.
I haven't looked deeper into how much an issue this might be.

My suggestion is to change AsyncDictionaryHelper to create the ConcurrentDictionary itself and expose necessary methods to ensure cleanup is performed.

I've made a POC in jnyrup@f62ec11

[jnyrup]

[Test]
public async Task MemoryLeak()
{
    var dict = new ConcurrentDictionary<int, int>();
    var asyncDict = new AsyncDictionaryHelper<int, int>(dict, "");
    while (true)
    {
        using var t = asyncDict.GetItemAsync(0);
        asyncDict.AddItem(0, 0);
        _ = await t.ConfigureAwait(false);
        dict.TryRemove(0, out _); // Leak of TaskCompletionSource
    }
}

After running the code above for ~20s this is the difference in memory usage I see

With the POC code added, and adjusting the test code, the memory dump no longer shows the _pendingRequests growing.

[Test]
public async Task MemoryLeak()
{
    var asyncDict = new AsyncDictionaryHelper<int, int>("");
    while (true)
    {
        using var t = asyncDict.GetItemAsync(0);
        asyncDict.AddItem(0, 0);
        _ = await t.ConfigureAwait(false);
        asyncDict.TryRemove(0, out _); // now also removes from `AsyncDictionaryHelper._pendingRequests`
    }
}

[kblok]

Do you know how bad is the leak during normal Puppeteer usage?

[kblok]

BTW. Sounds like a great idea. The AsyncDictionaryHelper was created to patch existing dictionaries. I think this should have been the way to go back on day 1.

[jnyrup]

After 40K iterations of the code below, the memory usage had increased by about 34MB, so roughly 1KB per iteration.

[Test]
public async Task MemoryLeak()
{
    var dict = new ConcurrentDictionary<int, int>();
    var asyncDict = new AsyncDictionaryHelper<int, int>(dict, "");
    var i = 0;
    while (true)
    {
        using var t = asyncDict.GetItemAsync(i);
        asyncDict.AddItem(i, i);
        _ = await t.ConfigureAwait(false);
        dict.TryRemove(i, out _); // Leak of TaskCompletionSource
        ++i;
    }
}

When the value is present in the ConcurrentDictionary, the TCS is immediately removed from the _pendingRequests again, which after 40K iterations "only" increased memory by 28MB.

[Test]
public async Task MemoryLeak_Key_is_present()
{
    var dict = new ConcurrentDictionary<int, int>();
    var asyncDict = new AsyncDictionaryHelper<int, int>(dict, "");
    var i = 0;
    while (true)
    {
        asyncDict.AddItem(i, i);
        _ = await asyncDict.GetItemAsync(i).ConfigureAwait(false);
        dict.TryRemove(i, out _); // Leak of TaskCompletionSource
        ++i;
    }
}

I don't have any estimate of how critical this is for real code.
I can't even recall now how I stumbled upon this leak, but it wasn't during day to day use.

[jnyrup]

Just tried modifying FrameManagementTests.ShouldHandleNestedFrames to run Page.GoToAsync 100 times.
That increased memory usage by 2.97MB

With the POC the overhead was reduced to 2.66MB.

[kblok]

@jnyrup I think you can create a PR and we can give it a try.