QueryCache: Memory leak when using dbContext.Set<> in subquery

[Tasteful]

When using dbContext.Set<MyTable1>() query cache is caching each instance that will fill memory with the same query.

The following is the result from https://github.com/Tasteful/bugs/blob/query-cache-memory-leak/MemoryUsage/Program.cs#L29-L56 where the only different is that the first result is using DbSet<> and the second using Set<>.

Query cache is working, using the dbContext.MyTable1
Before iteration 0 query cache count 0
After iteration 0 query cache count 1
Before iteration 1 query cache count 1
After iteration 1 query cache count 1
Before iteration 2 query cache count 1
After iteration 2 query cache count 1
Before iteration 3 query cache count 1
After iteration 3 query cache count 1
Before iteration 4 query cache count 1
After iteration 4 query cache count 1

Query cache is not working, using the dbContext.Set<MyTable1>()
Before iteration 0 query cache count 1
After iteration 0 query cache count 2
Before iteration 1 query cache count 2
After iteration 1 query cache count 3
Before iteration 2 query cache count 3
After iteration 2 query cache count 4
Before iteration 3 query cache count 4
After iteration 3 query cache count 5
Before iteration 4 query cache count 5
After iteration 4 query cache count 6

This issue is probably related to #6737 that have fixes for DbSet<>.

Steps to reproduce

Full running example exists here https://github.com/Tasteful/bugs/tree/query-cache-memory-leak

Further technical details

EF Core version: 1.1.1
Database Provider: Microsoft.EntityFrameworkCore.SqlServer
Operating system: Windows 10
IDE: VS2017

[ajcvickers]

Note for triage: The call to Set is returning a new DbSet instance each time it is called. Probably needs special handling in the cache.

[anpete]

Verified fixed in 2.0

[Tasteful]

@anpete does it exist any workaround that I can do in my solution? I have multiple ecommerce applications that need to be restarted on a regular schedule.

[Tasteful]

I can see that this is partial fixed in branch rel/1.1.2 after adding 41031f5 the issue is solved. Can you consider to also include this missed commit into next patch?

@anpete @ajcvickers @rowanmiller

[ajcvickers]

@Tasteful Is it possible for you to use DbSet properties on your context?

[Tasteful]

@ajcvickers No, it is not possible. We register everything dynamic in DbContext.OnModelCreating so we don't have any DbSet properties on the DbContext. Our usage is in an ECommerce platform that is used by many of our customers, each customer have their own installation, if it was in a sass or internal application I should have used an own build of EF but cant do that now.

[ajcvickers]

@Tasteful How about something like this:

public class DbSets
{
    private readonly DbContext _context;

    public DbSets(DbContext context)
    {
        _context = context;
    }

    private readonly IDictionary<Type, object> _sets 
        = new Dictionary<Type, object>();

    public DbSet<TEntity> Set<TEntity>() where TEntity : class
    {
        object set;
        if (!_sets.TryGetValue(typeof(TEntity), out set))
        {
            set = _context.Set<TEntity>();
            _sets.Add(typeof(TEntity), set);
        }

        return (DbSet<TEntity>)set;
    }
}

Then in your context:

public class MyTestContext : DbContext
{
    private readonly DbSets _sets;

    public MyTestContext(DbContextOptions options)
        : base(options)
    {
        _sets = new DbSets(this);
    }

    public DbSet<TEntity> CachedSet<TEntity>() where TEntity : class 
        => _sets.Set<TEntity>();
}

Then use CachedSet in your queries instead of Set. I tested this with your repro code and I see it caching correctly.

[anpete]

@Tasteful Agree we should consider this for the next patch.

A possible workaround might be to register a custom IQueryCompiler so you can apply the fix yourself. I think the high-level approach would be:

1. Create a custom QueryCompiler by inheriting QueryCompiler and register your custom version with DI (ReplaceService etc)
2. Create a custom ParameterExtractingExpressionVisitor, again by inheriting our built-in one. Apply the fix from 41031f5 by overriding VisitMethodCall.
3. Override QueryCompiler.ExtractParameters so that the compiler uses your custom parameter extractor.

[Tasteful]

@ajcvickers If I understand it correctly the problem is that the Set<> is returning different instances for every method call? In that case I can make an override of Set<T> method of our dbcontext with your code above.

@anpete I can try this suggestion also, but in general I'm against to use the classes/methods in the internal namespace due to possible conflicts between versions.

[Tasteful]

@ajcvickers One strange thing is that if I add an override in my dbcontext as below, then the cache will still not work correctly, but if I set it as CaachedSet, then the cache will work. Should it not work with the override like this?

        private readonly ConcurrentDictionary<Type, object> _sets = new ConcurrentDictionary<Type, object>();
        public override DbSet<TEntity> Set<TEntity>()
        {
            return (DbSet<TEntity>)_sets.GetOrAdd(typeof(TEntity), t => base.Set<TEntity>());
        }

[ajcvickers]

@Tasteful Based on my understanding overriding Set like that should work. You shouldn't need the Concurrent Dictionary though because context instances are not thread-safe.

[Tasteful]

If I add an interface as below on the my dbcontext implementation and cast the dbcontext into that interface during query the cache will work without doing anything else.

var items = ((IDb)dbContext).Set<MyTable1>().Where(item => ((IDb)dbContext).Set<MyTable1>().Any(x => x.Id == item.Id)).ToList();

    public interface IDb
    {
        DbSet<TEntity> Set<TEntity>()
            where TEntity : class;
    }

[ajcvickers]

@Tasteful Seems my theory for the bug was off a bit. Glad you found a workaround; we will still discuss if this should go in a patch.

[Tasteful]

@ajcvickers I was wrong.

Casting to interface was only changing the query to client evaluation and that was throwing exception and I think that the performance not will be good to fetch everything from db.
I also tried your suggestion with the DbSets but that is also moving the query into client evaluation.

I added the following into options builder also
.ConfigureWarnings(w => w.Throw(RelationalEventId.QueryClientEvaluationWarning))

I will try to fix my own QueryCompiler instead.

[Tasteful]

@anpete Your solution to override the IQueryCompiler is working to solve the problem. It was some small issues that the QueryCompiler have private inner class and ParameterExtractingExpressionVisitor have a private ctor so was not possible to inherit. I solved both of them by copying the code.

[anpete]

@Tasteful Great to hear. The difficulty extending is an oversight, I will file an issue to fix.

[ajcvickers]

@anpete to confirm risk is low enough to take in patch.
@Eilon once @anpete has confirmed we would like to take in 1.1.2, if possible, or otherwise the next patch.

[ajcvickers]

@anpete How strongly do you feel we need to patch this? I expect most people can use DbSet properties, and for those that can't there is a workaround, even though it's not very pretty. /cc @Eilon

[anpete]

@ajcvickers Risk is low, I think we should patch it. I see Set being used pretty often in user repositories etc.

[ajcvickers]

@Eilon Based on @anpete's comment, can we send this (and #8142) to get approval for 1.1.2?

[Eilon]

This patch fix is approved. Please follow the normal code review / pull request process and make sure you make the change in the correct branch.

[ajcvickers]

@anpete Is this merged into the 1.1.2 branch yet?

[anpete]

@ajcvickers Yep.

[ajcvickers]

Preview builds of this patch fix should be available on the following feeds:

• https://dotnet.myget.org/gallery/aspnet-1-0-5-may2017-patch-public (for fixes in the 1.0.x release)
  • Feed URL to add to NuGet.config: https://dotnet.myget.org/F/aspnet-1-0-5-may2017-patch-public/api/v3/index.json
• https://dotnet.myget.org/gallery/aspnet-1-1-2-may2017-patch-public (for fixes in the 1.1.x release)
  • Feed URL to add to NuGet.config: https://dotnet.myget.org/F/aspnet-1-1-2-may2017-patch-public/api/v3/index.json

If you have a chance, please try out these preview builds and let us know if you have any feedback!

[ajcvickers]

@Tasteful Could you check using the feed above that the fix checked in solves the problem you are seeing. It would be greatly appreciated.

[Tasteful]

@ajcvickers I have tested the 1.1.2-rtm-201704210746 and what I can see the cache is now operating as it should.

I noticed that the package Microsoft.EntityFrameworkCore.Tools still is on 1.1.1. I'm not sure if you bump all versions number to the same or if they are independent per package.

[Eilon]

@Tasteful each "repo" versions independently, so all packages within a repo generally have the same version, though different repos might be on somewhat different versions. Glad you're now seeing the behavior you're expecting!

[maumar]

Verified fixed in 1.1.2 candidate build.