Memory leak in compiled query cache (compiled Func<QueryContext,T>)

[rducom]

Path to the leak

We currently have a memory leak in production since we migrate from 2.1 to 3.1, exacerbated by the following context :

• we are supporting a dynamic query language, so we receive a lot of different query shapes
• we don't use DbContext Pooling, so our DbContext are not "cleaned" on dispose
• we are on 3.1.4, and ServiceProviderEngineScope stopped removing ResolvedServices recently (since 3.0?)

We have double checked we never use Expression.Constant directly in our code base (we already have had this leak before)

The leak is related to DiagnosticsLogger<T> instances.

Reasons for the leak

• DiagnosticsLogger are scoped
• Interceptors and CoreOptionExtensions have injected IServiceProvider since Generalize interceptors and add connection and transaction interception #16252 , and because the ServiceProviderEngineScope no longer remove Resolved Service reference, theses instances are indirectly holding scoped instances references.
• When we are compiling a Lambda to a Func<QueryContext,T> we are holding a closure generated from https://github.com/dotnet/efcore/blob/master/src/EFCore.Relational/Query/Internal/QueryingEnumerable.cs#L31 (you can see the Closure in the dotMemory screenshot).
• Each compiled query is referenced in a Singleton MemoryCache, so we should consider each compiled query as a Singleton, but they have a reference to a Scoped IServiceProvider

To summarize : each compiled query holds a reference to a scoped ResolvedServices existing when the query have been .Compile(). This behavior leaks memory. The fact "Singleton" compiled queries (Func<QueryContext,T>) are holding Scoped instances is a design mistake at the root of the leak.

Resolution

While debugging, I noticed the only reason for DiagnosticsLogger to be Scoped is for the IInterceptors property introduced in #16252 . For all the code paths around cached queries compilation, there's no need to have references to IInterceptors, the only need is to Log.

A clean fix should be to provide only singleton to the Func<QueryContext,T> closure, without reference to IServiceProvider or any other scoped instance.

Suggested PR : #21017

Note : I've validated the fix on our code base : before the fix, we keep references to all our DbContext upon GC. After this PR, we no longer have references to any DbContext after GC

[ajcvickers]

Note from triage:

• We will fix this in master now so it gets into the 5.0 daily builds
• We will take this issue to .NET Tactics to request patching in 3.1.x