Botbuilder-dotnet: LanguageGeneratorExtensions.UseLanguageGeneration GC root / memory leak

[jamesemann]

Background

There is an issue when using LanguageGeneratorExtensions.UseLanguageGeneration at scale since it uses a static dictionary to store lg managers.

See https://github.com/microsoft/botbuilder-dotnet/blob/f55bbc3f19ca874dceebb59c3ffcb31688f141b4/libraries/Microsoft.Bot.Builder.Dialogs.Adaptive/LanguageGeneratorExtensions.cs#L63

and

https://github.com/microsoft/botbuilder-dotnet/blob/f55bbc3f19ca874dceebb59c3ffcb31688f141b4/libraries/Microsoft.Bot.Builder.Dialogs.Adaptive/LanguageGeneratorExtensions.cs#L17

This is a GC root so this and any objects in it are never released from memory and over time in a multi tenanted environment this eventually leads to out of memory issues.

We have worked around it by providing our own copy of the behaviour which does not cache. Would it be possible to add an additional parameter to indicate whether or not to cache the lg managers?

[boydc2014]

@tomlm how about we add a parameter for caching, and default turn it false?

[tomlm]

or use MemoryCache instead of dictionary.

[mrivera-ms]

@boydc2014 please add options for fixes and ETA. Please also add tests.

[Danieladu]

how about using ConditionalWeakTable
Below is the draft PR that uses the weak reference dictionary to restore the resourceExplorer and languageGenerationManager mapping.

[boydc2014]

@mrivera-ms there are several options, like adding a parameter, or using a different data structure. I will get some time down next week with Tom and Hongyang to pick a good solution.

The solution would be ready probably one week after that. The fix should be obvious (once we decided), but i suspect the tests would take some time to make a proper test against memory leak.

[jamesemann]

@boydc2014 Please feel free to tag me on this PR when ready and I can test the memory leak

[Danieladu]

@jamesemann Feel free to test the PR. Thanks.
I have two solutions in this PR.
The first solution uses ConditionalWeakTable.
The second solution uses "MemoryCache" which is Commented out. You can remove the // marks and make some testing.
Thanks!
I have no idea which solution is better.

[boydc2014]

@jamesemann can you try the PR above https://github.com/microsoft/botbuilder-dotnet/pull/4916

[jamesemann]

Will do and report back asap

[Danieladu]

@jamesemann Is there any update on the test?

[jamesemann]

@Danieladu not yet - will update today or tomorrow

[boydc2014]

ping @jamesemann ~

[jamesemann]

Apologies @boydc2014 I have not been able to try the fix yet. Will prioritize and report back