dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.75k stars 3.18k forks source link

Query: Entities not being released due to leak in query caching #6737

Closed dkrooke closed 8 years ago

dkrooke commented 8 years ago

Apologies if this has been mentioned before:

The problem I am seeing, is that entities are still being held in memory, even after the Context has been disposed.

I have tried the following:

private void Method() {
     Test();
}

 private void Test()
 {
            using (var Context = new ExampleEntities())
            {
                Context.ExampleTable.ToList();  
          }
}

When you view the memory heap after the method Test has completed, ExampleTable rows are still held in memory.

rowanmiller commented 8 years ago

Are you running in release mode? And are you sure that garbage collection has run?

dkrooke commented 8 years ago

I was debugging and I'm 90% certain as other objects (non ef) are released when I force a collection.

Sent from my iPhone

On 10 Oct 2016, at 19:46, Rowan Miller notifications@github.com<mailto:notifications@github.com> wrote:

Are you running in release mode? And are you sure that garbage collection has run?

You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/aspnet/EntityFramework/issues/6737#issuecomment-252706890, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AVrOYIyD9fnI8LBY3Uy9hqrxvXS9SJVDks5qyofvgaJpZM4KSBYw.


All electronic mail to, from, or within the Telford & Wrekin Council may be the subject of a request under the Freedom of Information Act 2000 and related legislation, and therefore may be required to be disclosed to third parties.

Telford & Wrekin Council may monitor email traffic data and also the content of email in conjunction with the relevant policies.

divega commented 8 years ago

A few data points on this in case it helps:

A more reliable way to tell if the entity is still in memory is to look after the Test() and you force a garbage collection. If you can still observe any unexpected behavior when doing that, it would be great if you could provide a full repro.

dkrooke commented 8 years ago

Hi,

Thanks for the comments.

• Unfortunately AsNoTracking() does not help me, as I have found that by using it, the entity relationships are not loaded automatically.

Rightly or wrongly I have found that by doing the following:

class TableA { [key] public Guid Oid {get;set;} public string Code {get;set;} }

class TableB{ [key] public Guid Oid {get;set;} public Guid TableA {get;set;} public TABLEA TABLEA {get;set;} public string Code {get;set;} }

Context.TableA.ToList(); Context.TableB.ToList();

Significantly out performs

Context.TableB.Include(row => row.TABLEA). AsNoTracking().ToList();

On my more complex model, I also got out of memory exceptions using the AsNoTracking() technique, where I didn’t get anywhere close using the first method.

• I first noticed this when I ran my app in release mode. As you may gather I am loading a large amount of data for a year, If I want to load another years data in, I reset all my objects back to null and restart the load process, I noticed that the amount of memory being used was increasing rather than staying at a constant (as best as) level. Eventually, by keep loading data from 1 year to the next, Out Of Memory exceptions are thrown..

• I looked at the memory, when the USING had completed, then when Test() had completed and then after a system.gc.collect() completed. Each time, the objects were still in memory.

I have attached an sample project hopefully demonstrating my issue. EFTest.zip

You will need to add the Framework and adjust the DB Connection.

When the Main Method (EFTest) or MainWindow Constructor (WPFApplication) has got to the end, there are multiples of the same rows from TableA & TableB still in memory.

It's probably my complete misunderstanding of the garbage collection

Hope this makes sense

rowanmiller commented 8 years ago

Before we get someone to dig into this, I just want to double check that this isn't because the entities are databound to a form? If there is no databinding, then we'll get someone to look at it.

dkrooke commented 8 years ago

The data items are not bound

rowanmiller commented 8 years ago

Thanks, we'll take a look at it.

ajcvickers commented 8 years ago

This seems to be a leak in query caching, which is keeping references to the context. There may be other paths, but the one I have been able to trace is this:

Marking the bug for re-triage.

schwietertj commented 8 years ago

I can confirm with @ajcvickers that the issue is still happening. This seems like a decent bug because it is going to bogart resources/DTUs on Azure until this is patched.

divega commented 8 years ago

@anpete just confirm, is the bug fix included in 1.1.0-preview1?

anpete commented 8 years ago

No, didn't make it.

schwietertj commented 8 years ago

@anpete Is there a time frame for the patch release and if there is a work around? Is this an issue with IDisposable or just the implementation with EF?

anpete commented 8 years ago

@schwietertj The problem was with EF. You can see the details in this PR https://github.com/aspnet/EntityFramework/pull/6819

I think there may be a workaround, which is to Detach all entities from the leaked DbContext. I.e. The context instance that was used to initially execute the query. cc @ajcvickers who should be able to confirm.

ajcvickers commented 8 years ago

@schwietertj @anpete That should work to release the vast majority of the memory.

schwietertj commented 8 years ago

@ajcvickers @anpete Thank you for pointing me in a good direction. I will try it out at work tomorrow and let you know what I find.

schwietertj commented 8 years ago

@ajcvickers @anpete I believe it worked when a variable was declared and set to the entity. In the following instance there would not be an entity to detach and thus a connection will still persist.

dynamic result = new ExpandoObject();
using (var ctx = new CompanyDbContext())
            {
                result.employees = await ctx.Employees.Where(x => x.CompanyId == company_id && x.Active).CountAsync();
            }

I am thinking I will have to wait for the patch to be released and monitor production. The only other work around I can think of would be to set up a service that queries azure for any connection that has been quiet for 30 seconds and kill it.

anpete commented 8 years ago

@schwietertj You can enumerate all of the entities via DbContext.ChangeTracker.Entries()

divega commented 8 years ago

@schwietertj are you saying that you are seeing database connections leaked? The original issue that was reported here and that was fixed and for which the workaround applies is about a memory leak and not related to database connections.

schwietertj commented 8 years ago

@divega Very good point. I have experienced both memory leaks and connections that should be disposed that are not being disposed of in the application.

After a request is made in the mvc project, I can look at the activity monitor in ssms and still see an active process. The process should not be active since the db context has been disposed.

I originally thought it was due to DI the db context. I removed the injection and refactored with the "using" statement.

Please correct me if I am wrong but the database connection should be closed after the using block has been executed.

If need be I can open a new issue if it is deemed to be a separate issue than this one.

divega commented 8 years ago

@schwietertj It could be #6581 which causes some connections to be left open. It has been fixed for 1.0.2 (not yet released) and in 1.1.0-preview1. My understanding is that the workarounds for it are (a) enable MARS in the connection string and (b) avoid using Include() with collections and use multiple queries to retrieve the same data instead.

If you think that is not what you are hitting then yes please create a new issue.

schwietertj commented 8 years ago

@divega I just tried enabling MARS and the connection count increased 8 fold per active user. We also have to use includes since lazy loading isn't implemented. Is there an ETA on the 1.0.2 release?

Eilon commented 8 years ago

This patch is approved, please ensure it is merged into the correct branch and building as part of the patch train.

joshmouch commented 6 years ago

@rowanmiller I know this is an old thread, but about halfway through you ask:

Are you running in release mode?

Does running in debug mode cause some sort of known memory leak? I'm trying to track down an EF memory leak and am grasping at straws.

ajcvickers commented 6 years ago

@joshmouch No known issue like that,