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

Every CSharpScript.EvaluateAsync() call generates a new assembly that does not get unloaded #41722

Open
zeroskyx opened this issue Feb 15, 2020 · 18 comments
Labels
Area-Interactive Concept-Design Debt Engineering Debt, Design Debt, or poor product code quality Interactive-ScriptingLogic
Milestone

Comments

@zeroskyx
Copy link

Using the following code:

while(true)
{
	await Microsoft.CodeAnalysis.CSharp.Scripting.CSharpScript.EvaluateAsync("4 + 8");
	System.Console.WriteLine($"Loaded assemblies: {System AppDomain.CurrentDomain.GetAssemblies().Count()}");
}

Output:

Loaded assemblies: 49
Loaded assemblies: 50
Loaded assemblies: 51
Loaded assemblies: 52
Loaded assemblies: 53
Loaded assemblies: 54
...

The scripting API generates a new assembly for every call that does not get unloaded, resulting in an ever-increasing number of loaded assemblies and thus in an increasing amount of memory consumption.

There does not appear to be a way to unload the generated assemblies since they are not collectible (How to use and debug assembly unloadability in .NET Core).

How can we unload these generated assemblies?

@tmat tmat added this to the Backlog milestone Feb 15, 2020
@tmat
Copy link
Member

tmat commented Feb 15, 2020

Unloading is currently not supported.

@sharwell
Copy link
Member

Possibly related to #2621

@sharwell sharwell added Area-Interactive Concept-Design Debt Engineering Debt, Design Debt, or poor product code quality labels Feb 19, 2020
@jasekiw
Copy link

jasekiw commented Aug 13, 2020

I think this is also related to #42134

@tmat Is there any plans to support unloading the assembly? This really defeats the purpose of having a dynamic code execution system.

Exposing an a handle to allow the user to unload the assembly when they know they are finished with it sounds like the most logical approach.

@tmat
Copy link
Member

tmat commented Aug 13, 2020

@jasekiw We would accept a contribution that would add this capability on .NET Core.

@jasekiw
Copy link

jasekiw commented Aug 16, 2020

@tmat

I would love to contribute this functionality. However, I have some obligations I have to look after for the time being so it might take some time before I can fully dig in.

Do you have any recommendations of material to read (other than the contributing guide) that can help familiarize myself with the concepts used in the scripting api codebase?

I assume that this namespace (https://github.com/dotnet/roslyn/tree/master/src/Scripting/Core/Hosting/AssemblyLoader) is that namespace I need to be paying attention to, am I correct in that assumption?

Thanks!

@lucasmaj
Copy link

Same here. CSharpScript.EvaluateAsync effectively causes a memory leak every time it is used. We are resorting to caching scripts from CSharpScript.Create and maybe event this suggestion #22219 (comment)

Am I missing something? How to execute C# scripts without memory leak?

@AraHaan
Copy link
Member

AraHaan commented Mar 13, 2021

I think another way for this is to generate overloads to Run, RunAsync, Evaluate, EvaluateAsync that runs generated scripts (or even loads them) to take in AssemblyLoadContexts that the caller has to create subclasses to on their own, and that way they can enable the ability to unload the contexts on their end if they want.

And then have Roslyn just use those passed in contexts.

But for the cases of people using Roslyn Scripting with .NET Framework I would suggest that for .NET Framework to replace AssemblyLoadContext with AppDomain since the resolver for the load contexts are .NET 5 or newer only, while the load contexts are .NET Core 3.x or newer only.

Then this can solve the problem entirely.

But then it would possibly cause another issue: the generated code would have to load the assemblies they depend on from the program's directory of the program's assembly itself, and then look in the GAC (Global Assembly Cache) for the other assemblies the scripts might depend on.

@mitselplik
Copy link

For what it's worth, I was able to overcome this hurdle by off-loading the compilation and execution to a separate assembly and executing the necessary functions from a different domain context.

It should be noted that my specific needs were rather particular. I am allowing the user to essentially specify the guts of a function that takes some dynamic objects, calculates a value, and returns a double as a result. However, someone may be able to take my approach and make it more generic still.

namespace ScriptingAssembly
{
    public class ScriptEngine : MarshalByRefObject
    {
        public double CalculateCustomFieldValue(string functionBody, dynamic subject, List<dynamic> allComps, dynamic settings)
        {
            string compileExpression = null;
            compileExpression = $@"
    (subjectDynamic, compListDynamic, settingsDynamic) => 
    {{ 
        dynamic subject = subjectDynamic;
	List<dynamic> allComps = compListDynamic;
	dynamic settings = settingsDynamic;
	{(functionBody.Contains("return ") ? string.Empty : "return")} {functionBody};
}}
";
            System.Reflection.Assembly assembly1 = System.Reflection.Assembly.GetAssembly(typeof(ExpandoObject));
            System.Reflection.Assembly assembly2 = System.Reflection.Assembly.GetExecutingAssembly();
            Func<dynamic, List<dynamic>, dynamic, double> func = null;
            try
            {
                func = CSharpScript.EvaluateAsync<Func<dynamic, List<dynamic>, dynamic, double>>(
                    compileExpression,
                    ScriptOptions.Default
                        .WithReferences(assembly1, assembly2)
                        .WithImports("System", "System.Linq", "System.Collections.Generic", 
                            "System.Math", "System.Dynamic", "Microsoft.CSharp", "ScriptingAssembly")
                ).Result;
                if (func != null)
                {
                    double result = func(subject, allComps, settings);
                    return result;
                }
                else
                {
                    throw new Exception("The code provided failed to compile.");
                }
            }
            catch (Exception ex)
            {
                throw new Exception($"An error occurred compiling the code: {ex.Message}");
            }
        }
    }
}

I'm able to make use of it without a memory leak by loading the assembly into a separate app domain, executing the code, and then unloading the app domain.

AppDomain domain = AppDomain.CreateDomain("Temp Domain");
string assemblyName = typeof(ScriptingAssembly.ScriptEngine).Assembly.FullName;

ScriptingAssembly.ScriptEngine scriptEngine = (ScriptingAssembly.ScriptEngine)domain.CreateInstanceAndUnwrap(
	assemblyName, "ScriptingAssembly.ScriptEngine");

double result = scriptEngine.CalculateCustomFieldValue(functionBody, subject, compList, settings);

AppDomain.Unload(domain);

@seruminar
Copy link

I may be wrong from my somewhat naïve viewpoint, but I believe a part of the issue is that the private class LoadContext here: https://github.com/dotnet/roslyn/blob/main/src/Scripting/Core/Hosting/AssemblyLoader/CoreAssemblyLoaderImpl.cs#L45 does not use the base constructor which takes a bool isCollectible parameter, and then later there might not be a call to Unload(). I don't know if that helps, but it makes the issue less opaque for me.

@AraHaan
Copy link
Member

AraHaan commented Apr 28, 2022

I agree, perhaps it should default to them being collectible so that way there would be a call to Unload() automatically when the scripts finish running so that way they get cleaned up as expected.

@BasketGolfer
Copy link

Just adding on in hopes this will get addressed. This makes the feature unusable for our needs. I need to execute scripts repeatedly based on dynamic conditions.

@AraHaan
Copy link
Member

AraHaan commented Jul 23, 2022

Currently I think the only other way to bypass this is to have the code that calls the scripting apis itself be in it's own colletible ALC and hope it uses the "current ALC" for it which would be the ALC that the scripting apis were called in.

@AraHaan
Copy link
Member

AraHaan commented Jul 23, 2022

At least until oneone makes a patch to their LoadContext private class.

@nelson-pires
Copy link

Has this issue been fixed?

I've been using a delegate as it seems it doesn't hold compilation resources:

var script = CSharpScript.Create<string>(code, scriptOptions);
ScriptRunner<string> runner = script.CreateDelegate();
result = await runner();

Not sure if this is safe but I also tried:

await Task.Run(async () =>
{
    try
    {
        returnValue = await CSharpScript.EvaluateAsync<string>(code, scriptOptions);
    }
    catch (Exception ex)
    {
        result = $"InnerEx: {ex}";
    }
    finally
    {
        GC.Collect();
        GC.WaitForPendingFinalizers();
    }
});

In an attempt to get GC to free resources, however I'm not sure how to test the effectiveness of this, maybe someone can help clarify.

@atakanertrk
Copy link

I faced with same issue, rather using CSharpScript, I created new compilation and custom AssemblyLoadContext with Collectible.
Now I can run and unload assembly. No memory consumption remains.

                var syntaxTree = CSharpSyntaxTree.ParseText(sourceCodeSb.ToString());
                var compilation = CSharpCompilation.Create("GeneratedExpressionAssembly", new List<SyntaxTree>() { syntaxTree })
                                                   .WithReferences(assemblies)
                                                   .WithOptions(new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary));

                using (var ms = new MemoryStream())
                {
                    var emitResult = compilation.Emit(ms);
                    ms.Seek(0, SeekOrigin.Begin);
                    var alc = new CustomAssemblyLoadContext();
                    var assembly = alc.LoadFromStream(ms);
                    var assemblyType = assembly.GetType("GeneratedExpressionClass");
                    alc.Unload();
                    return assemblyType;
                }

@AraHaan
Copy link
Member

AraHaan commented Oct 30, 2023

I faced with same issue, rather using CSharpScript, I created new compilation and custom AssemblyLoadContext with Collectible. Now I can run and unload assembly. No memory consumption remains.

                var syntaxTree = CSharpSyntaxTree.ParseText(sourceCodeSb.ToString());
                var compilation = CSharpCompilation.Create("GeneratedExpressionAssembly", new List<SyntaxTree>() { syntaxTree })
                                                   .WithReferences(assemblies)
                                                   .WithOptions(new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary));

                using (var ms = new MemoryStream())
                {
                    var emitResult = compilation.Emit(ms);
                    ms.Seek(0, SeekOrigin.Begin);
                    var alc = new CustomAssemblyLoadContext();
                    var assembly = alc.LoadFromStream(ms);
                    var assemblyType = assembly.GetType("GeneratedExpressionClass");
                    alc.Unload();
                    return assemblyType;
                }

Honestly I feel like an overload of the roslyn scripting apis would be a safer option.

@jons-bakerhill
Copy link

Just chiming in that I very nearly sent this to production for a long lived API instance, that would have been a nightmare to debug. If this API is going to effectively leak memory on every call there really needs to be a big giant warning about it at least here https://github.com/dotnet/roslyn/blob/main/docs/wiki/Scripting-API-Samples.md and probably also in the library doc comment for the function. ... or just fix it/provide a safe endpoint that's as easy to use but that doesn't seem likely given the number issues and how they've been open around this.

@adambennett55
Copy link

I faced with same issue, rather using CSharpScript, I created new compilation and custom AssemblyLoadContext with Collectible. Now I can run and unload assembly. No memory consumption remains.

                var syntaxTree = CSharpSyntaxTree.ParseText(sourceCodeSb.ToString());
                var compilation = CSharpCompilation.Create("GeneratedExpressionAssembly", new List<SyntaxTree>() { syntaxTree })
                                                   .WithReferences(assemblies)
                                                   .WithOptions(new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary));

                using (var ms = new MemoryStream())
                {
                    var emitResult = compilation.Emit(ms);
                    ms.Seek(0, SeekOrigin.Begin);
                    var alc = new CustomAssemblyLoadContext();
                    var assembly = alc.LoadFromStream(ms);
                    var assemblyType = assembly.GetType("GeneratedExpressionClass");
                    alc.Unload();
                    return assemblyType;
                }

Honestly I feel like an overload of the roslyn scripting apis would be a safer option.

How?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Interactive Concept-Design Debt Engineering Debt, Design Debt, or poor product code quality Interactive-ScriptingLogic
Projects
None yet
Development

No branches or pull requests