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

Issue: Memory leak in ConstantExpressionHelper.cs #167

Closed
KentRollins opened this issue May 20, 2018 · 6 comments
Closed

Issue: Memory leak in ConstantExpressionHelper.cs #167

KentRollins opened this issue May 20, 2018 · 6 comments
Assignees
Labels

Comments

@KentRollins
Copy link

KentRollins commented May 20, 2018

I believe I've discovered a memory leak in the parsing of literal values (numerics at least) in expressions. I have a server that runs 24/7 and receives workunits. The workunits contain expressions that are compiled when received. I noticed a memory leak and used JetBrains dotMemory to investigate. It indicated that there was a ConcurrentDictionary that was slowly accumulating objects. My app does not use ConcurrentDictionary. I subsequently narrowed this down to ConstantExpressionHelper. After 30 minutes of execution the ConcurrentDictionary contained 89K nodes.

Below is an app that will reproduce this problem. Because it is stripped down to the bare minimum required to reproduce the problem and is multithreaded you can watch the Visual Studio memory monitor for 30 seconds and you will see it slowly creeping up. This app starts out for me at about 32MB of memory and after 2 minutes it is at 84MB.

This app is currently using an expression consisting of a single number (simplest possible repro), but I found this with a much more complex numeric expression involving about 6 calls to Math.X operations and some floating point literals.

using System;
using System.Linq.Dynamic.Core;
using System.Linq.Expressions;
using System.Threading;

namespace Utility.SoakTest
{
	class Program
	{
		static void Main(string[] asArgs)
		{
			RunThreads(8, RawMemoryLeak);
		}

		private static void RunThreads(int nThreads, Action actThreadStart)
		{
			for (int i = 0; i < nThreads; i++)
			{
				Thread thread = new Thread(new ThreadStart(actThreadStart));
				thread.Start();
			}

			while (true)
			{
				Thread.Sleep(5000);
			}
		}

		private static void RawMemoryLeak()
		{
			while (true)
			{
				string sExpr = "1234567890";
				LambdaExpression expr = DynamicExpressionParser.ParseLambda(new ParameterExpression[0], typeof(double), sExpr);
			}
		}
	}
}
@StefH
Copy link
Collaborator

StefH commented May 20, 2018

Thank you for analyzing this issue. I was able to reproduce and find the leaky code.

I've created a new PR which includes this fix:
#168

If you like you can review the code and use this source code branch for testing. Else I can also create a NuGet for you to test your scenario.

@StefH StefH self-assigned this May 20, 2018
@StefH StefH added the bug label May 20, 2018
@KentRollins
Copy link
Author

Stef, the code update looks good. I am currently on Visual Studio 2015 and am not in a position to upgrade so I can't build the library. This fix looks very simple so I have high confidence in it and I am happy to wait for an official build. However, if you would like me to try an early build in my environment to confirm, I would be happy to accept a nuget.

@StefH StefH changed the title Leaky Literals Issue: Memory leak in ConstantExpressionHelper.cs May 21, 2018
@StefH
Copy link
Collaborator

StefH commented May 21, 2018

I'll create a new NuGet.

@StefH StefH closed this as completed May 21, 2018
StefH added a commit that referenced this issue May 21, 2018
* Fixed ConstantExpressionHelper.cs (#167)

* 1.0.8.8
@KentRollins
Copy link
Author

Stef, I want to let you know that I downloaded and rebuilt with 1.0.8.8 which contains the ConstantExpressionHelper fix. After almost 24 hours, I could not detect a memory leak by looking at Task Manager. However, I ran the process in dotMemory and the data there seem to indicate that there is still a leak in this area. Things are improved by about an order of magnitude. The test case I submitted previously works without leaks and I have not been able to tweak it to induce this remaining problem. I will continue to try and create a reproducible case and report back as soon as I have something useful.

@KentRollins
Copy link
Author

False alarm. The remaining leak was user error on my part. I failed to update a folder in one of the QA systems during a test. After I updated it with 1.0.8.8 and ran the system under dotMemory it showed stable memory use over 1 hour. This fix is good.

@StefH
Copy link
Collaborator

StefH commented May 29, 2018

Ok, thanks a lot for finding this issue.

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

No branches or pull requests

2 participants