Taritsyn / JavaScriptEngineSwitcher

JavaScript Engine Switcher determines unified interface for access to the basic features of popular JavaScript engines. This library allows you to quickly and easily switch to using of another JavaScript engine.
Apache License 2.0
439 stars 49 forks source link

Exceptions around EmbedHostObject #74

Closed tamaskaroly closed 5 years ago

tamaskaroly commented 5 years ago

Hi,

First of all, thanks for your great work! I have an issue using EmbedHostObject, I quite often get unhandled exceptions ("A callback was made on a garbage collected delegate"). I'm using .NET Core 2.2 on Linux (Ubuntu 18.04). The NuGet packet version is 3.1.2 The issue doesn't pop up in the development environment (VS 2019). I know this issue has been discussed before, but I can't find any solution. Here is what I'm trying to do:

_consoleMapper = new ConsoleMapper(); _engine.EmbedHostObject("Console", _consoleMapper);

private class ConsoleMapper { public void write(string text) { ... } }

Calling Console.write from JS succeeds for a few times, then I get the exception above on the host side. I tried to pin the _consoleMapper object with GCHandle.Alloc, but it didn't help.

Could you please give some guidelines, tips how to properly use EmbedHostObject?

Thanks, Tamás

Taritsyn commented 5 years ago

Hello, Tamás!

Give an example of the full error message with stack trace.

tamaskaroly commented 5 years ago

Wow, that was quick! :) Here is the stack trace:

FailFast: A callback was made on a garbage collected delegate of type 'JavaScriptEngineSwitcher.ChakraCore!JavaScriptEngineSwitcher.ChakraCore.JsRt.JsSerializedLoadScriptCallback::Invoke'.

at JavaScriptEngineSwitcher.ChakraCore.JsRt.NativeMethods.JsGetAndClearExceptionWithMetadata(JavaScriptEngineSwitcher.ChakraCore.JsRt.JsValue ByRef) at JavaScriptEngineSwitcher.ChakraCore.JsRt.NativeMethods.JsGetAndClearExceptionWithMetadata(JavaScriptEngineSwitcher.ChakraCore.JsRt.JsValue ByRef) at JavaScriptEngineSwitcher.ChakraCore.JsRt.JsErrorHelpers.ThrowIfError(JavaScriptEngineSwitcher.ChakraCore.JsRt.JsErrorCode) at JavaScriptEngineSwitcher.ChakraCore.JsRt.JsValue.CallFunction(JavaScriptEngineSwitcher.ChakraCore.JsRt.JsValue[]) at JavaScriptEngineSwitcher.ChakraCore.ChakraCoreJsEngine+<>cDisplayClass28_0.b0() at JavaScriptEngineSwitcher.ChakraCore.ScriptDispatcher+<>cDisplayClass10_0`1[[System.Canon, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].b__0() at JavaScriptEngineSwitcher.ChakraCore.ScriptDispatcher.StartThread() at System.Threading.Thread.ThreadMain_ThreadStart() at System.Threading.ExecutionContext.RunInternal(System.Threading.ExecutionContext, System.Threading.ContextCallback, System.Object)

Taritsyn commented 5 years ago

I tried to pin the _consoleMapper object with GCHandle.Alloc, but it didn't help.

It's unnecessary.

Give an full source code of the example.

Taritsyn commented 5 years ago

And try to upgrade to version 3.1.3.

Taritsyn commented 5 years ago

How do you work with the pre-compiled scripts?

First of all, you need to keep the pre-compiled script from garbage collection. Save it in the field or call the GC.KeepAlive(precompiledScript) at the end of program. A lot depends on your architecture.

tamaskaroly commented 5 years ago

Upgrading to 3.1.3 didn't solve the issue. I also saved the pre-compiled scipt in a field, but it didn't help either. Below is the class that exhibits the issue. Basically, it is used in an ASP.NET app that allows callers to upload a script, then query/update variable values via the rest API.

using JavaScriptEngineSwitcher.ChakraCore; using JavaScriptEngineSwitcher.Core; using Microsoft.Extensions.Logging; using Newtonsoft.Json; using System; using System.Collections.Generic; using System.Linq; using System.Net; using System.Runtime.InteropServices; using System.Threading;

namespace DynamicUIManager { public class JavaScriptEngine : IScriptEngine { private readonly IScriptEngineHostServices _hostServices; private readonly ILoggerFactory _loggerFactory; private IJsEngine _engine; private IPrecompiledScript _precompiledScript; private int _initialized; private string _sourceCode; private ConsoleMapper _consoleMapper; private DmccMapper _dmccMapper;

    public JavaScriptEngine(IScriptEngineHostServices hostServices, ILoggerFactory loggerFactory)
    {
        _hostServices = hostServices;
        _loggerFactory = loggerFactory;
    }

    public void Initialize(string sourceCode)
    {
        if (Interlocked.CompareExchange(ref _initialized, 1, 0) != 0)
        {
            return;
        }

        IJsEngineSwitcher engineSwitcher = JsEngineSwitcher.Current;

        engineSwitcher.EngineFactories.AddChakraCore();
        engineSwitcher.DefaultEngineName = engineSwitcher.EngineFactories.First().EngineName;

        _engine = engineSwitcher.CreateDefaultEngine();

        _consoleMapper = new ConsoleMapper(this, _hostServices);
        _dmccMapper = new DmccMapper(this, _hostServices);

        _engine.EmbedHostObject("Console", _consoleMapper);
        _engine.EmbedHostObject("Dmcc", _dmccMapper);

        _sourceCode = sourceCode;
    }

    public void ExecuteScript()
    {
        _precompiledScript = _engine.Precompile(_sourceCode);
        _engine.Execute(_precompiledScript);
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    public object GetVariableValue(string variableName)
    {
        var value = _engine.GetVariableValue(variableName);

        // If the variable is a complex type, get its value as a JSON string and serialize it back to a .NET type.
        if (value != null && value.GetType().FullName == "JavaScriptEngineSwitcher.ChakraCore.JsRt.JsValue")
        {
            var json = _engine.Evaluate<string>($"JSON.stringify({variableName});");

            if (!string.IsNullOrEmpty(json) && json.First() == '[' && json.Last() == ']')
            {
                value = JsonConvert.DeserializeObject<Newtonsoft.Json.Linq.JArray>(json);
            }
            else
            {
                value = JsonConvert.DeserializeObject<Newtonsoft.Json.Linq.JObject>(json);
            }
        }

        return value;
    }

    public bool HasVariable(string variableName)
    {
        return _engine.HasVariable(variableName);
    }

    public bool SetVariableValue(string variableName, object variableValue)
    {
        if (!_engine.HasVariable(variableName))
        {
            return false;
        }

        // If the incoming variable is a complex type (JSON), we have to use eval() to inject the value, because
        // the script engine doesn't support complext types.
        if (variableValue != null && (variableValue is Newtonsoft.Json.Linq.JObject || variableValue is Newtonsoft.Json.Linq.JArray))
        {
            _engine.Evaluate($"eval({variableName} = {variableValue.ToString()});");
        }
        else if (variableValue is long)
        {
            _engine.SetVariableValue(variableName, Convert.ToInt32(variableValue));
        }
        else
        {
            _engine.SetVariableValue(variableName, variableValue);
        }

        return true;
    }

    public void CallVariablesUpdatedFunction(string pageId)
    {
        var param = new object[] { pageId };
        var handle = GCHandle.Alloc(param);

        try
        {
            _engine.CallFunction("onVariablesUpdated", param);
        }
        finally
        {
            handle.Free();
        }
    }

    protected virtual void Dispose(bool disposing)
    {
        if (disposing)
        {
            if (_engine != null)
            {
                _engine.Dispose();
                _engine = null;
            }
        }
    }

    private class ConsoleMapper
    {
        private readonly IScriptEngine _scriptEngine;
        private readonly IScriptEngineHostServices _hostServices;

        public ConsoleMapper(IScriptEngine scriptEngine, IScriptEngineHostServices hostServices)
        {
            _scriptEngine = scriptEngine;
            _hostServices = hostServices;
        }

        public void write(string text)
        {
            _hostServices.WriteLog(_scriptEngine, text);
        }
    }

    private class DmccMapper
    {
        private readonly IScriptEngine _scriptEngine;
        private readonly IScriptEngineHostServices _hostServices;

        public DmccMapper(IScriptEngine scriptEngine, IScriptEngineHostServices hostServices)
        {
            _scriptEngine = scriptEngine;
            _hostServices = hostServices;
        }

        public int openConnection(string deviceIp, int port, string userName, string password)
        {
            if (!IPAddress.TryParse(deviceIp, out var ip))
            {
                throw new ArgumentException($"Format of parameter {nameof(deviceIp)} is invalid.");
            }

            var connectionId = _hostServices.OpenDmccConnection(ip, port, userName, password);

            return connectionId;
        }

        public void closeConnection(int connectionId)
        {
            _hostServices.CloseDmccConnection(connectionId);
        }

        public string sendCommand(int connectionId, string command)
        {
            var result = _hostServices.SendDmccCommand(connectionId, command);

            return JsonConvert.SerializeObject(result);
        }
    }
}

}

Taritsyn commented 5 years ago
public void ExecuteScript()
{
    _precompiledScript = _engine.Precompile(_sourceCode);
    _engine.Execute(_precompiledScript);
}

This code doesn't make sense. Re-read the “How to upgrade applications to version 3.X > Script pre-compilation” subsection of the documentation.

Taritsyn commented 5 years ago
var handle = GCHandle.Alloc(param);

This is unnecessary.

Taritsyn commented 5 years ago
IJsEngineSwitcher engineSwitcher = JsEngineSwitcher.Current;

engineSwitcher.EngineFactories.AddChakraCore();
engineSwitcher.DefaultEngineName = engineSwitcher.EngineFactories.First().EngineName;

_engine = engineSwitcher.CreateDefaultEngine();

You can replace all this code with one line:

_engine = new ChakraCoreJsEngine();
Taritsyn commented 5 years ago
_hostServices.WriteLog(_scriptEngine, text);

Why do you call a JS engine from JS engine?

Taritsyn commented 5 years ago

It may be better to use the original Jint or Jurassic libraries for your task?

tamaskaroly commented 5 years ago

I just pass on the log entry to the host. That should be ok.

Sometimes I get the following exception. I guess it is the same as the previous one. It clearly shows that the issue is with CallVariablesUpdatedFunction.

JavaScriptEngineSwitcher.Core.JsFatalException: A fatal exception has occurred in a JavaScript runtime

Engine name: ChakraCoreJsEngine Engine version: 1.11.11 Category: Fatal error Description: A fatal exception has occurred in a JavaScript runtime ---> JavaScriptEngineSwitcher.ChakraCore.JsRt.JsFatalException: A fatal exception has occurred in a JavaScript runtime at JavaScriptEngineSwitcher.ChakraCore.JsRt.JsErrorHelpers.ThrowIfError(JsErrorCode error) at JavaScriptEngineSwitcher.ChakraCore.JsRt.JsValue.CallFunction(JsValue[] arguments) at JavaScriptEngineSwitcher.ChakraCore.ChakraCoreJsEngine.<>cDisplayClass28_0.b0() at JavaScriptEngineSwitcher.ChakraCore.ChakraCoreJsEngine.<>cDisplayClass28_0.b0() at JavaScriptEngineSwitcher.ChakraCore.ScriptDispatcher.<>c__DisplayClass10_01.<Invoke>b__0() at JavaScriptEngineSwitcher.ChakraCore.ScriptDispatcher.StartThread() --- End of stack trace from previous location where exception was thrown --- at JavaScriptEngineSwitcher.ChakraCore.ScriptDispatcher.InnnerInvoke(Func1 del) at JavaScriptEngineSwitcher.ChakraCore.ScriptDispatcher.Invoke[T](Func1 func) at Cognex.DynamicUIManager.JavaScriptEngine.CallVariablesUpdatedFunction(String pageId) in D:\Work\GitRepositories\deadpool\Projects\RTM\Cognex.DynamicUIManager\JavaScriptEngine.cs:line 184 at Cognex.DynamicUIManager.Server.Controllers.WizardsController.UpdateVariablesForForm(String wizardName, String formName, IEnumerable1 variables) in D:\Work\GitRepositories\deadpool\Projects\RTM\Cognex.DynamicUIManager.Server\Controllers\WizardsController.cs:line 247

Taritsyn commented 5 years ago

I recommend that you follow my previous advice.

tamaskaroly commented 5 years ago

I finally figured this out. Actually, it's very simple. The ASP.NET server serves requests on arbitrary threads and the UI made two calls simultaneously. It seems the engine expects requests on the thread it was created on, or at least it is not thread-safe. I created a separate thread that created the engine and then interacts with it and now it's all good.

Thank you for your effort, I really appreciate. Tamas

Taritsyn commented 5 years ago

It seems the engine expects requests on the thread it was created on, or at least it is not thread-safe.

In JavaScriptEngineSwitcher.ChakraCore module, calls are always synchronized by using the ScriptDispatcher class.

In your case, the following code:

if (Interlocked.CompareExchange(ref _initialized, 1, 0) != 0)
{
    return;
}

Prevents only re-initialization, but does not guarantee that the first initialization has completed.

The cause of first error is clearly seen in the following line of error message:

…
A callback was made on a garbage collected delegate of type 'JavaScriptEngineSwitcher.ChakraCore!JavaScriptEngineSwitcher.ChakraCore.JsRt.JsSerializedLoadScriptCallback::Invoke'.
…

And it means that the pre-compiled script was destroyed by the garbage collector. If the ExecuteScript method is called several times, then value of the _precompiledScript field is overwritten each time and reference to the pre-compiled script is lost. In this case, you do not need pre-compilation at all, because it will lead to losses in performance. Precompilation gives a gain only when initializing several JS engines by using a one pre-compiled script (for example, this approach is working well in the ReactJS.NET project). It is possible that for your task it is even better to use the Evaluate method, because it returns a result, perhaps it will allow simplify your architecture.

I also don't understand why you use the call of GC.SuppressFinalize(this) method in a class without a finalizer?

You need to simplify your architecture and understand that each call of the JS engine costs a certain amount of resources (this can be compared to database accesses), and therefore you should reduce the number of such calls.

I recommend you to consider simpler examples of using the JavaScript Engine Switcher library (for example, a Autoprefixer Host for .NET project).

tamaskaroly commented 5 years ago

Hi,

Thank you for your input.

This is part of a server that initializes an instance of my class once. Some code is standard boilerplate code, some others reflect my struggle to make it work. I’ll clean it up. I removed the _precompiledScript and pass the source code string directly to the engine. ExecuteScript is called once.

Looking at the ScriptDispatcher, I implemented a very similar functionality that all engine calls are dispatched to a thread. I also create the engine instance on that thread, I don’t know if you do the same, but it seems it is the key.

Thanks again for your help, I really appreciate! Tamás

From: Andrey Taritsyn notifications@github.com Sent: Thursday, July 18, 2019 10:36 AM To: Taritsyn/JavaScriptEngineSwitcher JavaScriptEngineSwitcher@noreply.github.com Cc: Karoly, Tamas tamas.karoly@cognex.com; Author author@noreply.github.com Subject: Re: [Taritsyn/JavaScriptEngineSwitcher] Exceptions around EmbedHostObject (#74)

It seems the engine expects requests on the thread it was created on, or at least it is not thread-safe.

In JavaScriptEngineSwitcher.ChakraCore module, calls are always synchronized by using the ScriptDispatcherhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Taritsyn_JavaScriptEngineSwitcher_blob_4833f9463a2224f97e149b19b2c3f25d09fefc11_src_JavaScriptEngineSwitcher.ChakraCore_ScriptDispatcher.cs&d=DwMFaQ&c=VCWpAYkS3z1bOCIxc-BPGZarCq9MRCAVxZJE051VqH8&r=CBYyTDoWFr_0hs8DFXwG2iP_lEB80YE8caNt0usL_TI&m=a9GMziwTBIfDhlTIxIygCF7Bde733phqpUnpTsddn38&s=zr8ZUZvk1joOWcA1nsnWI9lKnGVm80RZnhWI_Ah3KhM&e= class.

In your case, the following code:

if (Interlocked.CompareExchange(ref _initialized, 1, 0) != 0)

{

    return;

}

Prevents only re-initialization, but does not guarantee that the first initialization has completed.

The cause of first error is clearly seen in the following line of error message:

A callback was made on a garbage collected delegate of type 'JavaScriptEngineSwitcher.ChakraCore!JavaScriptEngineSwitcher.ChakraCore.JsRt.JsSerializedLoadScriptCallback::Invoke'.

And it means that the pre-compiled script was destroyed by the garbage collector. If the ExecuteScript method is called several times, then value of the _precompiledScript field is overwritten each time and reference to the pre-compiled script is lost. In this case, you do not need pre-compilation at all, because it will lead to losses in performance. Precompilation gives a gain only when initializing several JS engines by using a one pre-compiled script (for example, this approach is working well in the ReactJS.NEThttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_reactjs_React.NET&d=DwMFaQ&c=VCWpAYkS3z1bOCIxc-BPGZarCq9MRCAVxZJE051VqH8&r=CBYyTDoWFr_0hs8DFXwG2iP_lEB80YE8caNt0usL_TI&m=a9GMziwTBIfDhlTIxIygCF7Bde733phqpUnpTsddn38&s=_K9uG1hqnqSRqbpzm9EBM5shL3aN86k4Gz82BQ1NxzE&e= project). It is possible that for your task it is even better to use the Evaluate method, because it returns a result, perhaps it will allow simplify your architecture.

I also don't understand why you use the call of GC.SuppressFinalize(this) method in a class without a finalizer?

You need to simplify your architecture and understand that each call of the JS engine costs a certain amount of resources (this can be compared to database accesses), and therefore you should reduce the number of such calls.

I recommend you to consider simpler examples of using the JavaScript Engine Switcher library (for example, a Autoprefixer Host for .NEThttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Taritsyn_AutoprefixerHost&d=DwMFaQ&c=VCWpAYkS3z1bOCIxc-BPGZarCq9MRCAVxZJE051VqH8&r=CBYyTDoWFr_0hs8DFXwG2iP_lEB80YE8caNt0usL_TI&m=a9GMziwTBIfDhlTIxIygCF7Bde733phqpUnpTsddn38&s=QPhmwhg2miOiVA8vpXpeqiTAHlfdoshKdC5BkkqQAgM&e= project).

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Taritsyn_JavaScriptEngineSwitcher_issues_74-3Femail-5Fsource-3Dnotifications-26email-5Ftoken-3DABRLPESYL7LAD7KWD4FEHFTQAATQVA5CNFSM4IESRNPKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2HYELA-23issuecomment-2D512721452&d=DwMFaQ&c=VCWpAYkS3z1bOCIxc-BPGZarCq9MRCAVxZJE051VqH8&r=CBYyTDoWFr_0hs8DFXwG2iP_lEB80YE8caNt0usL_TI&m=a9GMziwTBIfDhlTIxIygCF7Bde733phqpUnpTsddn38&s=Gn1ReqFP2ne2p7GHGwPMLkDXKclNTBIeHY1vNADSGy4&e=, or mute the threadhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_ABRLPEQFLPAWQMGYRDIK4TDQAATQVANCNFSM4IESRNPA&d=DwMFaQ&c=VCWpAYkS3z1bOCIxc-BPGZarCq9MRCAVxZJE051VqH8&r=CBYyTDoWFr_0hs8DFXwG2iP_lEB80YE8caNt0usL_TI&m=a9GMziwTBIfDhlTIxIygCF7Bde733phqpUnpTsddn38&s=-L0VegBNg3b9DsY5YhIJL1YpgFV5sM1pY-_nkwyzrJo&e=.