Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
dotnetjunkie opened this issue Sep 27, 2022 · 4 comments
Labels
Milestone

Comments

@dotnetjunkie
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link

I think this issue about ThreadLocal is probably related to this

@andreminelli
Copy link

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...

dotnetjunkie added a commit that referenced this issue Sep 28, 2022
…alidator. Instead, it now stores thread ids. Fixes #956
dotnetjunkie added a commit that referenced this issue Sep 28, 2022
@dotnetjunkie dotnetjunkie added this to the v5.4.1 milestone Sep 29, 2022
@dotnetjunkie dotnetjunkie changed the title Ensure CyclicDependencyValidator properly disposed ThreadLocal<bool> Fix memory leak caused by use of ThreadLocal<bool> inside CyclicDependencyValidator Sep 29, 2022
@andreminelli
Copy link

Awesome work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants