JasperFx / lamar

Fast Inversion of Control Tool and Successor to StructureMap
https://jasperfx.github.io/lamar
MIT License
566 stars 117 forks source link

Memory leak with Nested Containers #232

Closed lilasquared closed 4 years ago

lilasquared commented 4 years ago

This was brought up in another project and I think I narrowed it down to Lamar registrations being the issue. I'm not going to repeat all the details here but essentially when using a nested scope the entity framework contexts and tracked objects are not getting disposed. In the below project i have two methods one to register the dependencies using the built in service collection and one that registers them with Lamar. If you comment out the Microsoft one and use Lamar you see an immediate memory leak, where as the Microsoft DI flat-lines around 55-60mb. Is there something wrong with the registrations that might be causing this?

link to original issue: https://github.com/jbogard/MediatR/issues/488 link to repro repo: https://github.com/lilasquared/hosted-service-test

PaitoAnderson commented 4 years ago

@lilasquared Thank you for sharing this, I was about to deploy upgrade from StructureMap to Lamar and I use MediatR, now you have me worried.

lilasquared commented 4 years ago

@PaitoAnderson I don’t think it has anything to do with MediatR, I will remove that layer and see if the results are the same.

PaitoAnderson commented 4 years ago

@lilasquared I use nested containers as well for jobs... 🤔

using (var nested = _container.GetNestedContainer())
{
    nested.Inject<PerformContext>(context);

    var mediator = nested.GetInstance<IMediator>();
    await mediator.Send(request);
}
lilasquared commented 4 years ago

I've updated the sample repo and removed MediatR, memory issue still exists.

lilasquared commented 4 years ago

After digging around in the closed issues it looks like this might be the root cause #215. If it is then it seems a fix is already in progress maybe?

jeremydmiller commented 4 years ago

@lilasquared What version of Lamar were you using?

lilasquared commented 4 years ago

4.1.0 - also there is a link to the repro project in the original comment.

jeremydmiller commented 4 years ago

@lilasquared I don't see anything in there where Lamar would be hanging on to anything. The original issue you link to suggests it's an issue with a single EF Core DbContext being used throughout and its internal identity map endlessly expanding.

lilasquared commented 4 years ago

If you look in Program.cs I provided a way to swap out which container is being used, switching out with structuremap or microsoft DI works as expected.

lilasquared commented 4 years ago

Does https://github.com/JasperFx/lamar/issues/215 have any impact? if Lamar is creating duplicates of the EF Core DbContext but then only disposing the one that is mapped to the direct implementation rather than the IDisposable then it would just hold it in memory perhaps?

jeremydmiller commented 4 years ago

215 was in 4.1, and that was a totally different scoping issue. Even so, that would have appeared as a lot more concurrency errors, but not the memory leak you're describing. And the DbContext isn't actually IDisposable as I recall.

lilasquared commented 4 years ago

looks like it does implement it https://github.com/dotnet/efcore/blob/ac4633559b117b92156d75f07b7ef81dc920b4fd/src/EFCore/DbContext.cs#L49

but if its not related then it doesn't really matter

jeremydmiller commented 4 years ago

Yeah, I'm not believing this is because Lamar isn't releasing the DbContext and disposing it. We've got users that slam Lamar with heavy concurrency and they'd be screaming bloody murder if that were the case.

jeremydmiller commented 4 years ago

The issue you're referring to was mostly a concurrency issue and not a cleanup or memory leak thing

lilasquared commented 4 years ago

I'm not sure what else it could be, given simply swapping out the DI fixes it. But yeah I imagine others would see this. Unless there is something special about the way Hosted Services are registered and handle scope.

jeremydmiller commented 4 years ago

The hosted service is a singleton

lilasquared commented 4 years ago

I'm not sure why that merits closing the issue?