simpleinjector / SimpleInjector

An easy, flexible, and fast Dependency Injection library that promotes best practice to steer developers towards the pit of success.
https://simpleinjector.org
MIT License
1.22k stars 152 forks source link

Fix memory leak caused by use of ThreadLocal<bool> inside CyclicDependencyValidator #956

Closed dotnetjunkie closed 2 years ago

dotnetjunkie commented 2 years ago

Simple Injector's internally creates a CyclicDependencyValidator instance per registration. Each CyclicDependencyValidator uses its own ThreadLocal<bool> instance, but this causes severe memory pressure (leak) when uses in combination with a large amount of registrations or with a large amount of container instances.

Related:

A ThreadLocal<T> causes a memory leak in case its not disposed of properly. Although the amount of memory isn't that big, it will count up in case many ThreadLocal<T> instances are created, as is the case with the CyclicDependencyValidator. Each InstanceProducer has its own CyclicDependencyValidator intance, and although the InstanceProducer removes the reference to its CyclicDependencyValidator when its no longer of use, the underlying ThreadLocal<bool> is not disposed of.

This causes memory issues as reported by @andreminelli here:

We took a memory dump and analyze it. We have spotted very large arrays (4 Million in length, in some cases) of ThreadLocal<bool>.LinkedSlotVolatile on LOH, the majority of them "attached" to CyclicDependencyValidator. This sample had about 6GB, and LOH allocated spaced (which was very fragmented) was about 2GB.

dotnetjunkie commented 2 years ago

The tricky part here is to ensure CyclicDependencyValidator instances are disposed in cases where GetInstance is not called on the InstanceProducer, which will be the majority of cases. Most registrations are dependencies (not root types) and for those dependencies InstanceProducer.BuildExpression() is called, but not InstanceProducer.GetInstance(). After calling BuildExpression the CyclicDependencyValidator is not removed, because:

only if GetInstance has been called we can know for sure that there's no cyclic dependency. There could be a 'runtime cyclic dependency' caused by a registered delegate that calls back into the container manually. This will not be detected during building the expression, because the delegate won't (always) get executed at this point. [source: comment on line 361 of InstanceProducer.cs]

andreminelli commented 2 years ago

I think this issue about ThreadLocal is probably related to this

andreminelli commented 2 years ago

I think this issue about ThreadLocal is probably related to this

And btw, the issue's author suggested a new (better?) implementation here - which a haven't take a look yet, I must admit...

andreminelli commented 2 years ago

Awesome work.