Query: Allow non static methods in top level projection

[ksmithRenweb]

Under a specific condition, disposed context's are not being released with a GC. In production it is rapidly increasing memory usage.

Memory Analysis done with Jetbrains dotMemory.

Attached working project:
ReproduceEFCoreMemoryLeakNonStaticHelperMethods.zip

Sample from attached code:

            Console.WriteLine("Take 1st snapshot!");
            Console.ReadLine();

            for(var i=0; i<10; i++)
            using (var context = new TestContextSql())
            {
                var repo = new Repository(context); // <--- The non static helper will leave 10 instances TestContextSql in memory.
                //var repo = new RepositoryStatic(context); // <--- The static helper will leave 1 instances TestContextSql in memory.

                var query = repo.GetInt();

                var list = query.ToList();
            }

            Console.WriteLine("Force GC");
            GC.Collect();
            Console.WriteLine("Take 2nd snapshot");
            Console.ReadLine();

        public IQueryable<int> GetInt()
        {
            return _context.TestEntities.Select(t => HashCode(t.DateRange1));
        }

        private int HashCode(string dateRange1)
        {
            return dateRange1.GetHashCode();
        }

Occurs in:
EF Core 2.0.1
EF Core 2.1.1

[ajcvickers]

Note for triage: I was able to reproduce this. The key part is that the query includes a call to an opaque instance method on the the repository:

public IQueryable<int> GetInt()
{
    return _context.TestEntities.Select(t => HashCode(t.DateRange1));
}

private int HashCode(string dateRange1)
{
    return dateRange1.GetHashCode();
}

Note that the repository holds a reference to the context. Removing the opaque call makes the leak go away:

public IQueryable<int> GetInt()
{
    return _context.TestEntities.Select(t => t.DateRange1.Length);
}

[ksmithRenweb]

Morning,

Last Friday I tried to keep to just the bare bones so I did not run on and on. I'll add a bit more detail today.

I took a memory dump of a web api process running at GB's of memory when it should have been under 500 MB like all the others. I found that it was holding onto 50k context objects each with 300 KB of memory reserved.

After a review of how we are handling context disposal and knowing that every other web api was not having this issue. I reviewed commits till i found the culprit.

A developer wrote a couple repository helper methods, that are called during the IQueryable. Best practices say they should have been static if they could have been and they could have. The helper methods are more complex than the GetHashCode I used in the example here.

It seems that when EF Core compiles the IQueryable it stores any such methods in a MemoryCache. Probably a really good idea for performance. But. Disposing of the context does not release those methods from the MemoryCache. Which is the Memory Leak!

The reason that making the helper methods static negates this memory leak is due to how static works in the language. Without static, EFCore is keeping a reference to each instance of the repository and it's helper method, which happened to hold a reference to the context. Thereby not allowing GC to pick it up. With static, EFCore only keeps 1 reference to the "static" helper method. So though its still arguably a memory leak, it is very minor.

I hope this helps understanding the issue.

[ajcvickers]

@ksmithRenweb Sorry, I should have said that we know what the issue is. Deciding what to do about it is a different matter--it plays into #12795 because behavior is a consequence of how we evaluate the opaque method call on the client. We're considering changing some of this in 3.0.

[ajcvickers]

Outcomes from triage discussion: detect when something problematic is being captured and then warn and don't cache (since cache entry will never be used anyway).

[ajcvickers]

For triage: discuss workarounds.

[ajcvickers]

We investigated workarounds here, but the options are limited. In general, the workarounds are all about preventing state being captured where that state causes problems. In the specific cases here, that state is the context instance. For this, three things to try are:

• Make sure the context instance (or whatever other state exists that is causing issues) isn't referenced from the method call, and hence won't be captured in the closure.
• In some cases, this may be possible by replacing instance access with static access, like in the example above, such that the implicit reference to the context is not captured in the closure.
• Another approach that works for the DbContext specifically is to make the method call actually be on the DbContext type. This pattern is detected and parameterized. For example, in the code above if the call is made directly on the context instead of through the repository, then it would be detected and parameterized.

[Mart-Bogdan]

We have encountered this issue yesterday. Actually this memory leaks was terrorizing us for quite long period, but recently we were able to trace source.

I think, that EF should throw an exception if regular method being used inside Expression Tree used to generate DB query.
I believe EF 6 (not core) had such behavior (as far as I remember, don't have time to check now).

I understand that adding such behavior now could break existing code (despite it's most likely work with memory leaks, unless methods are static), so I'd suggest adding configuration flag, that would cause Exception on attempt to use Expression, which is using method defined in code, which don't map to any SQL function.

P.S. in our case Controllers was captured inside Expression, which pooled lots of objects inside cache, like HttpContext and Request/Response objects.

[MichaCo]

@ajcvickers is there any good way of detecting code blocks which potentially could cause this issue via static code analysis?

My team manages many different service endpoints with 1000s of lines of EF code doing all kinds of queries.
We already found 2 occurrences causing out of memory issues (that's how I found this issue), where we for example map entities to return models, e.g. ctx.Set<Entity>().Select(p => this.Map(p))...

Manually going through all this is pretty... .expensive though and waiting for and then upgrading to 3.0 is also not an option.

[Mart-Bogdan]

@MichaCo you should consider rewriting the code using proper Repository pattern, or Command Query Separation (CQS), from my experience it's highly unlikely to have such bug when using these patterns.

Regarding analysis, perhaps some rules could be written using Roslyn, but I'm not sure.

I'm advocating, that checker be added to EF, to display a warning, or event fail with an exception if config flag provided, upon detection of such usage. EF 6 wasn't allowing such usage.

[ksmithRenweb]

@MichaCo Only thought I have is for a rule that enforces methods be static if they can be. I can think of scenario's where this wouldn't help, but it should catch most cases.

[ajcvickers]

@MichaCo @Mart-Bogdan Oops commented on the wrong issue. Sorry!

The issue posted here does not involve query capturing or cache miss issues--I already verified that from the repro code. Also this issue is not about the .NET Core garbage collector being less aggressive--that is, the garbage collector does reclaim memory, it just doesn't do it right away. So if you are hitting issues like this that you think are a caused by a bug in EF, then please file a new issue and include a runnable project/solution or complete code listing that demonstrates the behavior you are seeing.

[ajcvickers]

@MichaCo I'm not aware of any static analysis tools that exist, but calling instance methods on the DbContext would be the the thing to check for.

[divega]

We discussed this recently with a customer that hit this while porting a codebse that uses LINQ to SQL to EF Core, and wanted to capture some of the ideas discussed:

• LINQ to SQL apparently handled situations like this quite differently, because they didn't detect a memory leak before they ported to EF Core. It is worth understanding what the strategy was. @smitpatel and I have a theory that references to opaque instances methods could be handled by the funcletizer just like reference to properties, but the funcletizer we had used so far doesn't do it.
• Besides that and having eviction implemented in the the cache (tracked by Improve query cache eviction #12905), another mitigation would be to fallback to not cache queries when we detect tha they reference opaque instance methods. If this is going to happen for sure a warning (or maybe an error that can be turned into a warning) should be generated.

[smitpatel]

I have a theory that references to opaque instances methods could be handled by the funcletizer just like reference to properties, but the funcletizer we had used so far doesn't do it.

Looking at expression tree, it is not possible to handle reference to instance (or even properties) of enclosing class same way as other reference to local variables. We need to figure out a way to identify such references. In short term we can stop caching such tree.

cc: @roji

[ajcvickers]

Okay to throw for 3.0.

[tiakun]

As a victim (and survivor) of this issue, I think it would be better if we can track the number of cached compiled query so it is easier to monitor whether we are leaking something into the cache or not. Now I have to do it by hacking into DbContext's service provider and get IMemoryCache, assuming that it is MemoryCache and read the Count property, again assuming that the cache instance is used exclusively for compiled query cache.

[smitpatel]

Work for 3.0 is being tracked in #17069
Moving to backlog if we want to do something else in future.

[smitpatel]

Since 3.0, now we throw exception when we capture something problematic in the query cache entry.
Further items,

• Rather than throwing warn and not cache entry?
• Investigate what EF6 did. I believe @maumar did look into this in 3.0 time frame. Can you post outcome if you remember.
• Investigate what LINQ to SQL does.

[toffuser]

I have the same problem, even if I dispose the context the Memory goes up. Any news ?

[smitpatel]

@toffuser - Which version of EF Core are you using? Starting from EF Core 3.0, this throws exception and forces users to use static methods only so you will not run into memory growth.

[LucasLopesr]

same problem here, any news ?

[roji]

@LucasLopesr see @smitpatel's comment just above - which version of EF Core are you using?