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

Memory leaks in StrongNameUtil #594

Closed
arashjfz opened this issue Aug 14, 2021 · 2 comments
Closed

Memory leaks in StrongNameUtil #594

arashjfz opened this issue Aug 14, 2021 · 2 comments

Comments

@arashjfz
Copy link

arashjfz commented Aug 14, 2021

In my project I found a memory leaks in StrongNameUtil class

 internal static class StrongNameUtil
     {
   	  private static readonly IDictionary<Assembly, bool> signedAssemblyCache = new Dictionary<Assembly, bool>();

It seams that signedAssemblyCache has a problem and nothing is removed from it.
My memory analyzer says that There is millions of reference from signedAssemblyCache key to AssemblyBuilder assembly
As I checked signedAssemblyCache was the only reference to that instance of AssemblyBuilder assembly. So a ConditionalWeakTable may fix this kind of problem

@stakx
Copy link
Member

stakx commented Aug 14, 2021

Hi @arashjfz, thanks for reporting this potential issue, and for the accompanying PR.

It seams that signedAssemblyCache has a problem and nothing is removed from it.
My memory analyzer says that There is millions of reference from signedAssemblyCache key to AssemblyBuilder assembly

Am I understanding correctly that your memory analyzer claims that there are millions of AssemblyBuilder objects? If so, something is seriously going wrong: there shouldn't be more than at most two of those per ProxyGenerator. So my first suspicion would be incorrect usage of ProxyGenerator – they are designed to be reused for many proxy creations; one shouldn't instantiate a new generator per proxy.

At least with the .NET Framework, the assembly cache in StrongNameUtil shouldn't be a problem because assemblies cannot be unloaded from memory... at least not before unloading the whole AppDomain, which would also reclaim the cache.

(There have recently been changes in .NET Core that means assemblies may be garbage-collected, but we don't currently support any of that; see e. g. #473.)

I'm suspecting that your PR, which changes StrongNameUtil's cache to be based on a ConditionalWeakTable instead of a Dictionary, may therefore not help much (because the cache isn't the only thing holding on to loaded assemblies; they wouldn't be collected in either case), but could in fact cause a performance degradation (at the very least because ConditionalWeakTable is likely less optimized for performance than Dictionary).

So I am not convinced yet that we should make the change. I think it would be important to first know a little more about your memory analysis. Do you have a small repro code and instructions on how you performed the memory analysis?

@arashjfz
Copy link
Author

@stakx Thanks for your reply, You are correct. I Found the problem in my code and fix the situation that cause multiple assembly generation.

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

No branches or pull requests

2 participants