jint icon indicating copy to clipboard operation
jint copied to clipboard

Native async implementation of ExecuteAsync and InvokeAsync

Open Ellested opened this issue 4 years ago • 29 comments

Unit tests are good, but needs an implementation that shares them with the sync version. I thought you could maybe just have two separate engines, and have RunTest execute the code on each of them. You could then tell by the stack trace which of them failed, or have an outer assertion that pointed this out. Let me know what you think.

Ellested avatar May 08 '20 13:05 Ellested

I need this. Can somebody review it and merge?

3da avatar May 19 '20 02:05 3da

I am trying to use this in my project. And I have found that it doesn't work if you don't write var before variable. For example: var data = load();

So it works correct. And data has correct type. But if you write data = load();

Then data has type Task<T> which is incorrect, I guess.

3da avatar May 19 '20 04:05 3da

@3da When you get into this situation, it's because the code branched over to the sync path - so there's still some cases that fall through. If you set a breakpoint on you CLR Task method, then look in the stacktrace from Jint (the CLR stacktrace), it should point out where it branched over to the sync code path. Just look where it starts to not call into the async versions anymore.

I made a commit the other day, that addressed your issue (I think). Try to update and see if it still persist. Otherwise drop the JS code here, and I'll take a look.

Ellested avatar May 19 '20 12:05 Ellested

Regarding the PR it's not ready to be merged - it still needs some work to make the tests shared between the two code paths. I need some input on that part.

But I think in general the maintainers have to decide whether it's to big a burden to maintain two variants of the same code - alternatively drop the sync path and only maintain an async version (which can be used by sync code as well, with some minor performance penalties though).

That's a huge question, so I think I would probably decline it, unless they see a future demand for async - or people starts a riot, because they need it as badly as I do :-)

Ellested avatar May 19 '20 13:05 Ellested

I included your branch as source code in my project. Because I can't wait while it will be implemented in perfect way.

I will fix small bugs myself.

Can we replace Task with ValueTask for better performance?

3da avatar May 19 '20 18:05 3da

Can we replace Task with ValueTask for better performance?

ValueTask has some shortcomings, and it can go wrong if you fx. query the result twice and similar, so in some circumstances, it might need a little more attention. I would suggest for this to be an optimization that could follow a stable version - where all the fall through cases are spotted and fixed.

Jint is also cross platform/framework, and ValueTask is a .net core feature only AFAIK (which would then require conditional compilation). I think dot.net 5 will solve all of this in a much prettier way :-)

Ellested avatar May 19 '20 20:05 Ellested

Nice work @Ellested ... do you know when you have time to finalize this?

SebastianStehle avatar Jul 01 '20 09:07 SebastianStehle

Hey Guys, I've been away from development for a while, but now I'm back again.

@russlank - I'll fix the issues you've found. Thanks, and good job :-)

@SebastianStehle - I'm not sure I will ever finish this, as I've realized I cannot use Jint for my project. The project that I'm working on, requires Jint to be re-entrant, which would be way to hard for me to work out alone (I've tried in various ways).

So I've switched to the ClearScript library, though I discarded this to begin with, as it has a lot more moving parts. But I keep hitting into the wall with Jint, though I love the whole concept around it. It also performs better in general, as it operates directly on the CLR types. ClearScript is a lot slower here, as it requires marshaling.

Regarding this fork, the implementation still needs some work on the unit tests, so they can run the same suite as the existing sync code, so that's one task that needs some attention. But my focus has never been on taking care of this implementation alone. I will need some others to fulfill the mission, and I don't think the Jint maintainers are interested in this - which I fully understand.

So if someone would like to join as maintainers, or take it over, let me know.

Ellested avatar Jul 06 '20 09:07 Ellested

I had a look to your code and I am not sure if I would maintain 2 paths as it is very easy to miss some paths. I think the only option is to make everything async and then you have to go for ValueTask and you get a lot of changes.

SebastianStehle avatar Jul 06 '20 09:07 SebastianStehle

Hi @Ellested & @SebastianStehle,

I can help and contribute to some extent, but I am not very sure about my continuous commitment; it all depends on how much of my spare time I can manage to afford for the contribution, however I would be glad to participate anyway.

Frankly, I have not gone so deeply in reviewing current Jint’s code and understand it fully much myself, so as the beginning I may contribute in start testing, at least in the beginning, however, I am still interested in making it work in async mode, as its current state prevents me from moving forward and using it for all types of my projects, so I believe it worth having async version in place.

Yes, it is sad that the current team members do not include async in their considerations, at least at the moment, as much as they are focusing on performance issues, and this will complicate things a little, because that will require kind of periodic visits to the original developed branch and updating the async path code.

russlank avatar Jul 06 '20 12:07 russlank

I've updated the implementation with a merge of the upstream dev branch, just to see how difficult it was.

It's not easy, and the main issue is that the sync path is not aware/concerned about the async path. Some of the sync methods are also pretty large, so it needs quite a lot of duplicate code, as the sync functionality is not always separated into fine grained methods. It's not possible to break up the sync code either as this will just create maintenance overhead on that part. So maintenance will be quite hard (though not impossible).

I've spend 3 hours on this, so that's not something I will be continuing to do, as I'm not using Jint at the moment.

I made the sync engine unit test suite, run the async variants as well. It's a little brutal - though better than nothing ;-)

Finally I've changed the virtual async base methods to abstract, to make sure implementations are not overlooked. As always, stack traces are your friend in finding problems, if the execution path switches to the sync code. Hopefully the abstract methods are preventing most of this (or they at least fail fast if not implemented).

Ellested avatar Jul 07 '20 19:07 Ellested

Just a small heads up.

I've actually got back to work on Jint, as I solved the re-entrance issue that I've struggled with earlier (time away = fresh mind). I'm now running my project on both engines, which also helps in locating problems.

I still think Jint has some outstanding features, that I simply can't let go of. So I take the challenge in keeping this fork up to date, until further notice.

Ellested avatar Jul 15 '20 09:07 Ellested

Are there any plans to complete this? Would be an awesome feature. Some kind of easy integration of async calls would be great. Either Promises or Async execution. I think I can also sponsor this feature.

SebastianStehle avatar Feb 05 '21 08:02 SebastianStehle

Are there any plans to complete this? Would be an awesome feature. Some kind of easy integration of async calls would be great. Either Promises or Async execution. I think I can also sponsor this feature.

Hi Sebastian,

I don't think Sebastian Ros and Marko Lahma are interested in the current implementation, as it requires two almost identical code paths to be maintained.

I'm using the code as it is right now, and haven't synchronized it for a while. Mostly because it's seems to work quite stable at this point, and secondly because I'm working on something else :-)

I was thinking about implementing Promises on top of this, but I saw another implementation which is already pending as a PR. I couldn't personally use that implementation, as I'm using the engine in a single threaded synchronization context (which basically translates to "you cannot await another thread"). The code in the PR therefore also use locking mechanics to avoid re-entrance, and this would be fine in most circumstances. Locking could however be avoided using a pure async implementation and a single threaded synchronization context.

So for now I decided not to do anything, and leave the await keyword unimplemented. Instead I use a wrapper method await(() => ), which saves the execution context, and restores it on return from the async method.

Now that .net 5.0 is here, I think it would make sense to upgrade to ValueTasks, but it would disable the support for classic .net framework.

So many "big" questions and decisions quickly arises, like framework support and so on. A full implementation would have a rather large impact on the main branch, and probably to much concern at the moment.

The best I can do is probably to keep this branch in sync with main, and the maintainers could maybe take a look someday and use some of it for inspiration (when they are ready to make the move).

I see two ways to overcome the duplicate code paths problem. The easiest is just to skip the sync path and only keep the async version. Alternatively you could restructure/refactor the existing code, to minimize the code overlap between the two paths, mainly by refactoring methods into smaller parts (but inline them). But it should be done so it makes logical sense and keeps the readability.

Here's my await() wrapper method (exposed in JS):

private async Task<JsValue> Await(JsValue call)
{
    // Store and leave the current execution context while performing an async operation
    var saveLexEnv = _engine.ExecutionContext.LexicalEnvironment;
    var saveVarEnv = _engine.ExecutionContext.VariableEnvironment;
    _engine.LeaveExecutionContext();
    try
    {
        if (call is ArrowFunctionInstance afi) return await afi.CallAsync(call, new JsValue[] { });
        throw new ScriptEngineException("Error: only arrow functions (closures) are allowed within await()");
    }
    finally
    {
        // Return to the original context when continuing
        _engine.EnterExecutionContext(saveLexEnv, saveVarEnv);
    }
}

Ellested avatar Feb 06 '21 01:02 Ellested

I see it like you. A consistent refactoring to use ValueTask would solve the problem. Then promises are not needed anymore for most uses, you can just have async calls in the script that look like synchronous calls.

SebastianStehle avatar Feb 06 '21 14:02 SebastianStehle

I see it like you. A consistent refactoring to use ValueTask would solve the problem. Then promises are not needed anymore for most uses, you can just have async calls in the script that look like synchronous calls.

Yes - though I still think Promises should be implemented on top, for compatibility with both tools and code. I also think it would be easier to do if the engine was async. I think it only requires handling of succes/failure branching on top of the async call mechanics.

I might try to do an implementation later, I think I can look at the PR I mentioned for a lot of inspiration. I like that PR in many ways, but an async implementation would probably be more straight forward as there would be no thread management.

Ellested avatar Feb 07 '21 03:02 Ellested

I will synchronize this branch within a month or so. But I just updated the method wrapper that I use from the JavaScript code to allow ICallable's instead.

        private async Task<JsValue> Await(JsValue call)
        {
            // Store and leave the current execution context while performing an async operation
            var saveLexEnv = _engine.ExecutionContext.LexicalEnvironment;
            var saveVarEnv = _engine.ExecutionContext.VariableEnvironment;
            _engine.LeaveExecutionContext();
            try
            {
                var thisObj = saveVarEnv.Record.GetThisBinding();
                if (call is ICallable ca) return await ca.CallAsync(thisObj, new JsValue[] { });
                throw new JavaScriptException("Error: only callable functions and methods are allowed as arguments to await()");
                //if (call is ArrowFunctionInstance afi) return await afi.CallAsync(call, new JsValue[] { });
                //throw new JavaScriptException("Error: only arrow functions (closures) are allowed within await()");
            }
            finally
            {
                // Return to the original context when continuing
                _engine.EnterExecutionContext(saveLexEnv, saveVarEnv);
            }
        }

Add the method to the engine: _engine.SetValue("await", new Func<JsValue, Task<JsValue>>(Await));

From JS you need to use it when you call other JS methods from within you async method.

function method1Async() {
    // like
    await(method2Async);
    // or
    await(() => method2Async());
    
    // for async CLR methods just call them like
    Task.Delay(1000);
    // or
    await(() => Task.Delay(1000));
    // this will fail
    await(Task.Delay(1000));
}

function method2Async() {
    // do stuff
}

I'm a little in doubt whether I can always access the .GetThisBinding() like this, but currently my async JS code works.

Ellested avatar Feb 26 '21 01:02 Ellested

Updated the branch with latest dev, and moved ALL async variants into separate files, which makes it a lot easier to manage in the future. Just required fiddling with the Build props file, to enable the VS2019 feature to dynamically fold files. Great feature by the way to organize larger classes, even small things like members are sometimes nice to just have in a separate file, when you reach a certain number. It's the perfect feature for the async code path, the code is clear and isolated now 😀

I have more to come, and already found an issue in this implementation that should be fixed. Namely the stack reset call in the ExecuteAsync method, which prohibits reentrancy. It's fine to remove it, I've tested over a long period. But I need to update my own project first, to make it compatible, as exercising Jint from this project is a really good test. I also spotted some new interesting Engine methods that I need to hook into and test along the way. I will include my JavaScript await wrapper as well, and I think I will probably also try to see if I can implement the native JavaScript async/await into this branch. I will need it anyway, as I can't use a Task.Run() based implementation, which seems the normal goto choice. (I only have a single thread available).

The reason that I haven't fixed the above immediately, is that I have another branch that I'm actually using for my own project, that corrected some of this. But it has some overrides and hacks around that wouldn't work for others. Some of the Jint classes that I need to customize, are internal and sealed which hinders direct subclassing and customization from outside the source code. So it makes me run with a mix of code that is not suited for everyone on a separate branch, and I therefore don't go back this branch with my latest findings.

But I think I can improve on that by just subclassing my points of interest with public wrappers in the jint source, and have the bulk part of the code modifications reside in my own projects. This will keep a minimal footprint in the Jint source, so I can keep this branch more up to date. It will depend on how much member promotion I need to do.

Talking about up to date, it's fantastic to see all the great features they've added since my last update - this project rocks 😀 I will go straight in now and make some JS classes 🥇

PS. ValueTask is in consideration, but then it's only .net 5 & .net core. afaik

Ellested avatar Mar 01 '21 03:03 Ellested

@Ellested Any news on this? I've tried your branch and it works fine so far. Just curious whether you've progressed further :)

lofcz avatar Sep 05 '21 02:09 lofcz

Hi Matěj, I have done some progress on a special project, where I'm using ValueTasks and implemented JS async/await in the engine. But this branch is not "compatible", with the vision of Jint anymore, as it will not support the old frameworks. Only .net core and .net 5 and up.It's also not stable to use outside my case, as I'm calling into the engine from Blazor, and Blazor itself, uses a single threaded synchronization context, which is the only way it works (without placing locks everywhere).But I will share the branch when I release the project, as there are still some bugs that needs to be ironed out.Promises can be implemented in javascript itself when you have async await support, but I also have an idea on how to solve that in the engine natively, which will actually improve the current implementation a lot. I just don't have time at the moment to do it, but I think it will be in 2021 at least 😀However, I'm not sure that my implementation will end up being useful to others, unless they have the same setup as I do with Blazor, but maybe it could be used as a starting point to make a more generalized implementation. I think the engine needs a dedicated task dispatcher itself, running in a single threaded synchronization context. That's how I would probably make a more generalized version of it.Best RegardsKenneth -------- Oprindelig besked --------Fra: Matěj Štágl @.> Dato: 05.09.2021 04.15 (GMT+01:00) Til: sebastienros/jint @.> Cc: Ellested @.>, Mention @.> Emne: Re: [sebastienros/jint] Native async implementation of ExecuteAsync and InvokeAsync (#731) @Ellested Any news on this? I've tried your branch and it works fine so far. Just curious whether you've progressed further :)

—You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or unsubscribe.Triage notifications on the go with GitHub Mobile for iOS or Android.

Ellested avatar Sep 05 '21 07:09 Ellested

@Ellested I wonder whether you moved to another adventures after so much time has passed. There was 3.0 release of Jint a while back and many parts were shifted around a bit. Is the current code in your Async branch the final version of your work? There are a few tests that don't pass. I'm running Blazor based setup too and while Jint now has rudimentary Task-Promise conversion there are still issues with it, so I'd like to bring your branch up to date.

lofcz avatar Feb 18 '24 08:02 lofcz

The async branch is pretty old, so its gonna be hard job to get it up to date.

However, I've been working on another project since then, which is soon to be released. I, have a more current Jint async version that I use internally for this, and while not exercising all parts of Jint, it's quite stable. I think the last time i synced it with Jint was 3.00-2046.

This version is supposed to be released back into github, but some parts of it are still tailored for my project, though I marked all places where I have this sort of tailoring in the code. I think it would be a better starting point, and the plan was to make the tailored parts less intrusive, so it could also be used by other projects. I'd like to cooperate on this, so if you like we could maybe start off there.

The project I'm working on is not using promises, but it has an async implementation that is native to CLR, so you can await .NET async tasks directly inside Jint. The current Jint implementation is not optimal for this kind of use, and most of the original async and promise stuff is therefore replaced with a native approach.

What kind of usage do you need it for, by the way. It's interesting you mention Blazor, as my project is also a mix of Jint and Blazor. It's actually called Jzor, and will be out in an alpha release within a month or so. My project has a built-in TypeScript compiler, which transpiles to JS, which ends up in Jint, and it uses a React like framework and approach to build server side web applications with direct integration to the CLR world. This works very well and has been quite stable for at least 6 months of development, where I haven't touched the custom Jint implementation. So it's actually this version I'd like to share and it would be great to have more people maintaining it.

Let me know what you think.

Ellested avatar Feb 18 '24 10:02 Ellested

Thanks for a fast and frankly, better than expected reply! While 3.00-2046 is about a year old build, it would be a much better starting point for sure. I agree with the current upstream implementation being suboptimal and I also like how in this PR the need to await calls is not forced on end-users, making the language more accessible. We designed it the same way in WattleScript.

As for my use case, I'm currently working on an "AI engine" app in which Human-LLM interactions are scripted in JS, here is a quick preview: https://www.youtube.com/watch?v=d20lTP-ToiU The app aims to abstract all UI away from end users and enable fast creation of scripts that do things from managing the self-supervised work of students to acting as a natural language terminal. The code editor used is called Monaco and I've implemented a good chunk of DAP for it using Jither's work as a starting point.

The reason this is JS-based and not TS is to enable debugging as currently, Esprima (probably to be replaced by Acornima) doesn't currently support sourcemaps. The app itself is server-sided Blazor, I have a protocol developed for it allowing for the efficient two-way binding of long strings (such as code files) between client and server, based on DMP which I could contribute in case Jzor targets Server and doesn't have this solved already.

If you would like my help with anything from updating Jint to creating more tests, let me know and I'll contribute!

PS: back in 2021 when I inquired about the async support I worked on a very similar use case to this:

It's actually called Jzor, and will be out in an alpha release within a month or so. My project has a built-in TypeScript compiler, which transpiles to JS, which ends up in Jint, and it uses a React like framework and approach to build server side web applications with direct integration to the CLR world.

Ended up designing and using WattleScript back then due to various reasons, but for this project using a standard language and a way better IDE-like tooling available with it overweights the benefits WS brings in. The amount of effort constantly poured into Jint solved quite a few problems that were show-stoppers for me since then.

Instead of React my solution used plain Scriban templates (no reactivity) and was quite clunky to use due to very limited debugging capabilities.

lofcz avatar Feb 18 '24 10:02 lofcz

Thanks for a fast and frankly, better than expected reply! While 3.00-2046 is about a year old build, it would be a much better starting point for sure. I agree with the current upstream implementation being suboptimal and I also like how in this PR the need to await calls is not forced on end-users, making the language more accessible. We designed it the same way in WattleScript.

The current version I have, do actually conform and require you to use the await/async patterns. However, it might be possible to add an option that could await "automagically", like the old implementation had.

As for my use case, I'm currently working on an "AI engine" app in which Human-LLM interactions are scripted in JS, here is a quick preview: https://www.youtube.com/watch?v=d20lTP-ToiU The app aims to abstract all UI away from end users and enable fast creation of scripts that do things from managing the self-supervised work of students to acting as a natural language terminal. The code editor used is called Monaco and I've implemented a good chunk of DAP for it using Jither's work as a starting point.

Interesting - I've also implemented most of the DAP, and Jzor can debug multiple applications running on the host simultaneously. I haven't been working on the Jzor debugger for a while, and there's some small issues I need to address when using the debugger command line, but for now it's pretty stable and will do fine for the alpha version.

I also started out with Jithers work, and combined it with the DAP protocol from another library, and ended up with a somewhat tailored version again, as each Jzor application runs on it's own debugger thread in VSCode. This needed a bit of customization to get working, and again I'm not sure it will work for everyone. But I plan also to released this back to github, as I think it could be a good starting point to create a fully working implementation for Jint. I have covered most of the basic DAP stuff, so it's the more exotic features that are missing now, but I think it shouldn't take too long to finish.

The reason this is JS-based and not TS is to enable debugging as currently, Esprima (probably to be replaced by Acornima) doesn't currently support sourcemaps. The app itself is server-sided Blazor, I have a protocol developed for it allowing for the efficient two-way binding of long strings (such as code files) between client and server, based on DMP which I could contribute in case Jzor targets Server and doesn't have this solved already.

If you would like my help with anything from updating Jint to creating more tests, let me know and I'll contribute!

PS: back in 2021 when I inquired about the async support I worked on a very similar use case to this:

It's actually called Jzor, and will be out in an alpha release within a month or so. My project has a built-in TypeScript compiler, which transpiles to JS, which ends up in Jint, and it uses a React like framework and approach to build server side web applications with direct integration to the CLR world.

Ended up designing and using WattleScript back then due to various reasons, but for this project using a standard language and a way better IDE-like tooling available with it overweights the benefits WS brings in. The amount of effort constantly poured into Jint solved quite a few problems that were show-stoppers for me since then.

Instead of React my solution used plain Scriban templates (no reactivity) and was quite clunky to use due to very limited debugging capabilities.

Sounds a lot like Jzor's destiny, It also started as JavaScript only, but I started over and made it TypeScript+React instead :-) I'm working on the Jzor documentation now, and you can take a look when I'm done (in abouth a month or so) to see if it there's anything you like. At least it would be nice with some feedback :-)

I'll find the branch with my current async Jint implementation this week, and put it up. I then suggest you take a look at it, and we can see how we could proceed, and figure what can be done to make it open to both Jzor and other uses.

Ellested avatar Feb 18 '24 15:02 Ellested

Looking forward to your async branch, as currently, the issues with upstream async methods implementation force me to use out-of-process V8 in lieu of Jint. Could you please let me know when it's up? Thanks a lot :-)

lofcz avatar Feb 18 '24 23:02 lofcz

@lofcz I extensively use async await with jint and just made a PR to make it a bit more easy to expose async Task returning member methods to jint. Which part of the async/await implementation is not working for you?

tom-b-iodigital avatar Feb 21 '24 19:02 tom-b-iodigital

@tom-b-iodigital thanks for your PR! I've tested it and the main issue with async implementation I had - CLR tasks not being awaited correctly seems to be fixed. I'm still very interested in the possibility of freeing the executing thread while the task is running (native ExecuteAsync) but at least I can use Jint now.

lofcz avatar Feb 21 '24 21:02 lofcz

@lofcz my PR honestly didn't change that much, the entire task to promise and UnwrapIfPromise structure was already in place. I was just tired of always wrapping my objects with static functions and using the engine setvalue method to pass them. My PR made that part easier, now I have a fetch interface that is 99% compatible with the Javascript browser implementations and is completely async.

I think most of the confusion stems from the fact that the docs are not that clear on how to use async/await/tasks/promises together. So I think I will add that part to the Readme with some examples

tom-b-iodigital avatar Feb 21 '24 21:02 tom-b-iodigital

@lofcz I extensively use async await with jint and just made a PR to make it a bit more easy to expose async Task returning member methods to jint. Which part of the async/await implementation is not working for you?

I use Jint with the Blazor framework, which is single threaded in nature. So I cannot use features like Task.Wait and Task.Run at all (which I believe the current implementation still does), as this will lead to a thread lock.

Instead I switch the engine context when awaiting a CLR task inside the engine, which hands the thread back to C#, which is then free to call into the engine again (re-entrance, and still using the same thread). This also mean the implementation is completely lock free,

For large scripts (like executing the TypeScript compiler) the async path inside the engine is noticably slower, but I use it for small scripts like event handlers. I have not done any optimizations, but I think it could be optimized a great deal. Especially if the GetValue had two variants (a fast and slow), as I recall making this async, impacted almost all code inside the engine.

I don't know if this approach has any interest outside my scope, but I'll dump a copy when I have had time to clean it up :-)

Ellested avatar Feb 21 '24 22:02 Ellested