EFCore based Memory Leak

[Poltuu]

Description

There is a memory leak, based on the way EF Core cache queries, especially queries with parameters automatically generated with such a librairy.

The following test (on GetTests.cs) shows just that, with cachedQueryCount being equal to 1000 instead of 1.

        [Fact]
        public async Task Memory_Leak()
        {
            for (int i = 0; i < 1000; i++)
            {
                await _fixture.SendAsync("GET", $"/api/v1/courses?filter[number]:eq={i}", null);
            }

            int cachedQueryCount = 0;

            var cache = _fixture.Server.GetService<IMemoryCache>() as MemoryCache;
            var entriesProperty = typeof(MemoryCache).GetProperty("EntriesCollection", BindingFlags.NonPublic | BindingFlags.Instance);
            var entries = entriesProperty.GetValue(cache) as ICollection;
            var items = new List<string>();
            if (entries != null)
            {
                foreach (var item in entries)
                {
                    var methodInfoVal = item.GetType().GetProperty("Value");
                    var val = methodInfoVal.GetValue(item) as ICacheEntry;
                    if (val?.Value == null)
                    {
                        continue;
                    }
                    var contentType = val.Value.GetType();
                    var gens = contentType.GenericTypeArguments;
                    if (gens.Length == 2 && gens[0] == typeof(QueryContext) && gens[1] == typeof(IAsyncEnumerable<CourseEntity>))
                    {
                        cachedQueryCount++;
                    }
                }
            }

            Assert.Equal(1, cachedQueryCount); //Except it's 1000
        }

Environment

• JsonApiDotNetCore Version: master

[jaredcnance]

Thanks for reporting this. How are your test server and fixture constructed? This might be caused by a memory leak in the test not the application.

Have you observed this in an actual app?

[Poltuu]

I used the server and fixture from the file GetTests.cs.
I'm developping a framework very similar, and we did observe this behavior in production.

The problem comes from the expression generation using Expression.Constant. Ef Core does not recognize those a parameters anymore (unlike Ef 6), and the EF query plan gets cached for each new parameter.

The solution is to replace
Expression.Constant(value, typeof(T))
by something like
((Expression<Func<T>>)(() => value)).Body
So that EF interprets the value as a parameter, not a constant, but I don't know json-api-dotnet well enough to fix it.

[jaredcnance]

@Poltuu thanks for sharing! I'll work on a minimal test for the query execution itself. We do use Expression.Constant when building our queries.

UPDATE: I haven't forgotten about this issue, but tried to bring this down to some minimal tests and haven't been able to reproduce it yet.

[maurei]

@bart-degreed have you seen signs of this memory-leak in action?

[bart-degreed]

Yes, I can reproduce it on master with the test below, which I temporarily added to ContentNegotiation.cs:

[Fact]
public async Task Memory_Leak()
{
    for (int i = 0; i < 1000; i++)
    {
        var request = new HttpRequestMessage(HttpMethod.Get, $"/api/v1/todoItems?filter[ordinal]:eq={i}");
        var response = await _fixture.Client.SendAsync(request);
        Assert.Equal(HttpStatusCode.OK, response.StatusCode);
    }

    int cachedQueryCount = 0;

    var cache = _fixture.GetService<IMemoryCache>() as MemoryCache;
    var entriesProperty = typeof(MemoryCache).GetProperty("EntriesCollection", BindingFlags.NonPublic | BindingFlags.Instance);
    var entries = entriesProperty.GetValue(cache) as ICollection;
    var items = new List<string>();
    if (entries != null)
    {
        foreach (var item in entries)
        {
            var methodInfoVal = item.GetType().GetProperty("Value");
            var val = methodInfoVal.GetValue(item) as ICacheEntry;
            if (val?.Value == null)
            {
                continue;
            }
            var contentType = val.Value.GetType();
            var gens = contentType.GenericTypeArguments;
            if (gens.Length == 2 && gens[0] == typeof(QueryContext) && gens[1] == typeof(IAsyncEnumerable<TodoItem>))
            {
                cachedQueryCount++;
            }
        }
    }

    Assert.Equal(1, cachedQueryCount); //Except it's 1000
}

But to make the problem visible, you also need to make EF Core use a shared cache due to a breaking change in EF Core 3.0 via Startup.cs:

public virtual void ConfigureServices(IServiceCollection services)
{
    var cacheInstance = new MemoryCache(new MemoryCacheOptions());
    services.AddSingleton<IMemoryCache, MemoryCache>(x => cacheInstance);

    services
        .AddDbContext<AppDbContext>(options =>
        {
            options.UseMemoryCache(cacheInstance);
...

I did not run a load test to see if there is an actual memory leak, but in any case it is inefficient usage of cache space. Each filter parameter in the query string restarts the query parse process and creates a new cache entry, because of a different constant value. By changing the constant into a parameter, the parsed query can be reused by EF Core. And probably by database servers too.