search

zzzprojects / System.Linq.Dynamic.Core

The .NET Standard / .NET Core version from the System Linq Dynamic functionality.
https://dynamic-linq.net/
Apache License 2.0
1.57k stars 228 forks source link

Possible Memory Leak with ConstantExpressionHelper #764

Closed TWhidden closed 9 months ago

TWhidden commented 9 months ago

Description We are currently investigating a growing leak in our net8 app. Doing a dotnet-dump, and using JetBrains dotMemory, we found over 761k objects in the ConcurrentDictionary<object,Expression>, and 695k ConcurrentDictionary<Expression, string> both found in the ConstantExpressionHelper. We believe this is what is causing our leak. Analysis of all the instances show this class overwhelming on top of any other type instance.

Usage: Our application makes heavy use of the dynamic library - where expression trees are created from input string data provided by outside calls. This has worked well for many years, going back to the early introduction of the dynamic linq library in the framework days.

Inputs change very regularly, based on the applications requirements. Constants are introduced that may only be used once in the application lifetime.

Possible Solution: After cloning the project, was able to see the ConcurrentDictionary in ConstantExpressionHelper grow without any way to remove collected constant expressions. Proposal would be to use a sliding cache with a reasonable short lifetime (minutes? hours?), instead of an ever-lasting private static ConcurrentDictionary, allowing the system to remove constants that are no longer being called, which will then allow the GC to remove the tress of referenced objects.

Instance Details Process running under Ubuntu Linux Container, net8.0 framework, approximately 9 days of uptime.

Trends monitoring: image

Screen Shots of dotMemory outputs, arranged by size descending: image

Event showing 18 megs of Int32 constants image

The use of the ConstantExpressionHelper makes total sense, so I can appreciate why it's there. The proposal is just to have a way to let it clear itself out, especially for highly dynamic, long running services. Self-managing is desired, no reason for the developer to write code responsible to clear these things out.

I am more than willing to work on this and create a PR, downside is I don't have all the frameworks installed on my dev machine. Just getting it to compile required changing some csproj to remove all the old legacy things. If you would like a PR, let me know and ill work on that.

TWhidden commented 9 months ago

Ok, that was not so bad - I created a PR for review. Let me know if you want me to adjust or change anything.

https://github.com/zzzprojects/System.Linq.Dynamic.Core/pull/765

StefH commented 9 months ago

@TWhidden Preview version can be tested: 1.3.9-preview-01

StefH commented 9 months ago

@TWhidden Did you have time to test this?

TWhidden commented 9 months ago

Got into prod last week.I'll dump the process today and do an analysis.

TWhidden commented 9 months ago

I'd say we made a huge difference. This is 7 days of uptime on a busy server. Do I close this issue or do you? image

StefH commented 9 months ago

You can close it. Or I will.

Thanks for everything.

TWhidden commented 9 months ago

We detected a runtime problem with our Sentry logging. Since this PR was already merged, I created a new issue and a new PR #769 - I hope that is the right way to handle it. Glad it was just a pre-release!