ClearScript

ClearScript copied to clipboard

A hacky start is at: https://github.com/microsoft/ClearScript/pull/399

But it seems it's still an issue if the engine raises an exception.

I've spent a few hours trying different things out and looking at dotMemory output (not sure what's the best tool for this), yet I don't have in-depth .Net knowledge to understand well what I am being presented with.

This is the snapshot of object changes while running "Create ALC, load the assembly, create engine, throw exception, try unloading" loop:

Some call trees of objects that survived that I believe to be suspects:

Also a set of Finalizable objects:

My main requirement is to be able to unload my assemblies using ClearScript, modify and reload them, as I rely on ALC as a plugin mechanism that allows quick iteration cycles. Failing to unload them sadly prevents them from being modified meaning that it hinders the iteration cycles. I am not that concerned with leaking unmanaged resources as part of this, as this is purely for development.

Given it's the exception that is causing the issue, I spent mostly focusing there.

At the surface, it looks like we are getting V8ObjectImpl in V8SplitProxyManaged.ScheduleScriptEngineException converting that to V8ScriptItem which needs to be disposed, but it looks like it only needs to be disposed to free up V8 resources, so I guess we end up leaking things, but it should not prevent unloading. I tried disposing of the script object and the inner exception immediately in the constructor of ScriptEngineException, but that didn't seem to do much.

I have verified in this case that FreeLibrary does get called, and with my PR applied the method table is free'd, so it's not that, also that path seems to work fine if there are no exceptions.

I am not yet familiar with the interaction between the components, so any insight would be useful.

Hi @AudriusButkevicius,

Thanks for reporting and researching this issue.

We're already testing delegate handle release as discussed in #394. Sorry about #399; we weren't expecting your PR and went ahead with our own implementation.

As for the issue apparently related to exceptions, how did you first encounter it? Can you post a minimal code sample?

Thanks!

To reproduce, in the diff that I provided:

https://github.com/microsoft/ClearScript/discussions/394#discussioncomment-3072768

Add an additional

try {
  engine.Execute("doesNotExist();");
} 
catch (Exception e) 
{
}

went ahead with our own implementation.

Is that available on some branch?

Is that available on some branch?

No. At the moment our fix is similar to yours, except that it stores the delegate handles as pointers in a raw memory buffer to avoid accessing a managed collection on the GC thread (although that's probably overkill) and has a few minor tweaks for unusual and legacy deployments. It doesn't yet address the exception issue.

Hi @AudriusButkevicius,

We appear to have a fix for the exception issue. Interestingly, the Unloading sample with your modifications now works even without manually unloading the native library.

We're still testing, but if we don't run into any new issues within a day or two, we'll put out a preview build that includes the fix. If you're interested, we could push the code changes before then.

Thanks again!

I'd love to know what the changes are, and apply them locally to my own build asap to continue working on my code ahead of the fix being released. Could you by any chance provide a branch?

@AudriusButkevicius We've pushed the change to the main branch. Please let us know if it works for you. Thanks!

Thanks, I'll give that a go this evening.

This sort of example still prevents unloading:

        public class Dummy
        {
            public int Mul(int i)
            {
                return i * 2;
            }
        }

        public string GetMessage()
        {
            using var eng = new V8ScriptEngine("xx");
            eng.AddHostObject("print", new Action<object>(Console.WriteLine));
            eng.AddHostObject("Dummy", new Dummy());
            eng.Execute("print(Dummy.Mul(2));");
            return "";
        }

I also noticed while digging into this a few days ago, that sometimes the number of calls to V8ProxyHelpers.AddRefHostObject and V8ProxyHelpers.ReleaseHostObject does not match, but even tracking that and freeing stuff upon an attempt to unload does not seem to work.

Hi @AudriusButkevicius,

This sort of example still prevents unloading:

Well, that's unfortunate.

The root problem is that .NET's dynamic infrastructure – the subsystem that supports C#'s dynamic type and similar features in other languages – is fundamentally incompatible with unloading.

If we take ClearScript out of the picture completely, the following works perfectly:

public string GetMessage() => "Greetings!";

But this blocks unloading:

public string GetMessage() => (dynamic)"Greetings!";

When execution initially passes through a dynamic call site – even a redundant cast – the infrastructure sets up an inline cache that holds unloading-unfriendly data.

The exception issue occurred because ClearScript had a small number of dynamic interactions with the ScriptException property. Those were easy to remove; we just added a non-dynamic version of that property.

The problem now is that ClearScript also uses the dynamic infrastructure for host method calls. Specifically, it relies on the C# invocation binder to select the method based on the call signature. The more commonly used default binder lacks support for essential features such as generic methods.

ClearScript does support the default binder, but only as an optional fallback when dynamic binding fails. Our final move here would be to allow hosts to bypass the dynamic binder and rely on the fallback. We've confirmed that the default binder is compatible with unloading.

Is that something you'd consider using? Please let us know.

Uff, that's disappointing that a runtime feature breaks another runtime feature.

I guess I don't understand well enough what is at stake here, but I do not or plan to use generic methods, so I'd love to try out anything that might fix this, as unloading is more important than having to write type wrangling code.

My usecase does rely on variadic non-fixed type arguments, i.e, js calling Foo(1, 2, 3, 4) and Foo(5, instanceOfHostType), but my plan to handle that is to just have param object[] args and do a bunch of type assertions to figure out what type of call signature was used.

I wonder if me exposing the JavaScriptLikeObject discussed in the other thread from the host will also cause simillar issues? I assume anything dynamic anywhere will cause simillar issues?

Do you have a diff for disabling the dynamic binder? I could try that myself and see if I can find other unrelated issues.

Hi @AudriusButkevicius,

Uff, that's disappointing that a runtime feature breaks another runtime feature.

Indeed. We were going to report this, but the underlying issue is already on the books and isn't likely to be fixed:

Unfortunately the dynamic infrastructure was designed in an era where we didn't have fine grained Assembly unloading. It was designed when we only had to worry about AppDomain unloading (for which this works). The way that caching works here ends up holding onto many types, at many different levels, that will essentially make an Assembly unloadable [sic].

Fixing this is not a simple bug fix, it's essentially a feature request because it would require us to pretty substantially change our caching structure here. At the moment this area is only opening for servicing level bug fixes and is not taking feature requests.

my plan to handle that is to just have param object[] args and do a bunch of type assertions to figure out what type of call signature was used.

There's probably no need for that. The default (reflection-based) binder does support overloads and default arguments.

I wonder if me exposing the JavaScriptLikeObject discussed in the other thread from the host will also cause simillar issues?

It shouldn't. We avoided dynamic in that sample, which we now do whenever we can. The dynamic type is extremely convenient, especially when dealing with a script language, but unless x.Fire() must work regardless of whether x is Employee or x is Revolver, it's probably not worth it 🤣

Do you have a diff for disabling the dynamic binder?

We've pushed the new property to the main branch. Please send any additional thoughts or findings our way.

Seems that this doesn't fix all faults. There are things that use dynamics internally in the engine that effectively hinder the ability to unload.

I think that anything that uses engine.Script effectively prevents unloading. For example seems that engine.GetStackTrace() is one of the offenders.

I can see there are also internal uses of it for invoking methods, executing scripts via EngineInternal, which I am not sure how to access it not via dynamic (to be able to remove to use of it).

There's probably no need for that. The default (reflection-based) binder does support overloads and default arguments.

The problem is that I need a global function that supports multiple overloads. As far as I understand I can't just do

Engine.AddHostObject("getFoo", FooTakesTwoInts);
Engine.AddHostObject("getFoo", FooTakesOneIntOneUnit);

Ok, I think I have a patch removing all uses of .Script internally in the engine, effectively replacing stuff like: Script.EngineInternal.command = command with (Global.GetProperty("EngineInternal") as ScriptObject).SetProperty("command", command) etc.

Seems to work nicely. Would you consider accepting this PR?

Also, I wonder if the engine has DisableDynamicBinding=true, throwing on accessing ScriptException or Script might make sense?

Another random question. I am returning string[] from my host function, seems that this ends up being an array of C# strings, oppose to an array of javascript strings. What's the right way to return the correct type/converted to javascript?

Seems the examples are very "this is how you create and use host types in javascript" heavy, and very lacking to explain how to create a simple javascript object type, or return the right types.

Hi @AudriusButkevicius,

Ok, I think I have a patch removing all uses of .Script internally in the engine

Yep, we're doing that as well. This issue has triggered an audit of all dynamic use in ClearScript (except for test code). We're also overhauling the reflection-based method binder; until now it was really only intended for – and tested in – specific VBScript scenarios and could use some enhancements for more general use. Also, ClearScript has always used reflection for constructor and indexer binding, so a unification seems in order to ensure consistency of behavior. We're on it.

Also, I wonder if the engine has DisableDynamicBinding=true, throwing on accessing ScriptException or Script might make sense?

Hmm, at first glance, a separate switch – or maybe one with a different name – seems like a better approach, API-wise. We'll give it some thought for the next release. Would it be a huge problem for you if we ended up renaming DisableDynamicBinding?

I am returning string[] from my host function, seems that this ends up being an array of C# strings, oppose to an array of javascript strings. What's the right way to return the correct type/converted to javascript?

ClearScript favors easy access to host objects from script code (and vice versa) over automatic conversion. There are exceptions, but only when the lack of conversion would make ClearScript unusable (e.g., primitive values) or the conversion is particularly useful but tricky to perform manually (e.g., tasks/promises).

The easiest way to convert a C# array to a JavaScript array is via Array.from.

Seems the examples are [...] very lacking to explain how to create a simple javascript object type, or return the right types.

Sorry about the documentation. The best way – and ultimately the only way – to create script objects is in script code, and the converse is true for host objects. ClearScript tries to make it easy for the host and script engine to work together. If you have a question about a specific scenario, please don't hesitate to ask.

Thank you!

Is there an easy way to call Array.from from C#? Effectively all of the JS code I am dealing with is owned by an external party, so I can't really modify it, I have to make sure that C# passes the right JS types.

I assume there isn't some hook I could implement for me to do conversions automatically for all sorts of collections?

I assume for a generic "object" type I can just use PropertyBag?

Hi @AudriusButkevicius,

Is there an easy way to call Array.from from C#?

Sure:

var hostArray = new object[] { 123, "foo", 456.789, "baz" };
var arrayCtor = (ScriptObject)engine.Global.GetProperty("Array");
var scriptArray = arrayCtor.InvokeMethod("from", (object)hostArray);

The cast to object forces the compiler to treat hostArray as an argument rather than an argument array. In many cases it isn't necessary. And of course dynamic users can simply do engine.Script.Array.from(hostArray).

I assume there isn't some hook I could implement for me to do conversions automatically for all sorts of collections?

No, sorry.

I assume for a generic "object" type I can just use PropertyBag?

Sure, or you can use a script object:

var objectCtor = (ScriptObject)engine.Global.GetProperty("Object");
var obj = (ScriptObject)objectCtor.Invoke(true);

The best choice depends on the expected access pattern for the object. The host-script boundary is relatively expensive, so if performance is a concern, try to minimize the number of hops across it.

Cheers!

Hi @AudriusButkevicius,

Oops, sorry, we forgot to address one of your comments:

The problem is that I need a global function that supports multiple overloads.

You can do the following:

engine.AddHostObject("foo", HostItemFlags.GlobalMembers, new ObjectWithOverloadedMethods());

Thanks!

Hi @AudriusButkevicius,

Also, I wonder if the engine has DisableDynamicBinding=true, throwing on accessing ScriptException or Script might make sense?

Our understanding is that it wouldn't be effective. Invoking a member that returns dynamic sets up a so-called dynamic call site that thwarts unloading. By the time that member throws an exception, it's already too late.

Thanks!

Version 7.3.2 addresses this issue by minimizing dynamic infrastructure engagement throughout ClearScript and adding DisableDynamicBinding and ScriptExceptionAsObject.

Note that the cast host function still uses dynamic as a last resort when all other approaches fail, but that's the only remaining known case of dynamic infrastructure engagement that isn't under the application's control.

Please feel free to reopen this issue if you have additional questions, concerns, or findings related to this topic. Thank you!