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

async/await and callback support #514

Open
christianrondeau opened this issue Jun 18, 2018 · 58 comments
Open

async/await and callback support #514

christianrondeau opened this issue Jun 18, 2018 · 58 comments
Labels

Comments

@christianrondeau
Copy link
Contributor

christianrondeau commented Jun 18, 2018

I was surprised to find no issues nor pull requests to support C#/JS async/await and Task support.

In a perfect world, I could make that transparent and "halt" the calling method (like #192), or use a callback mechanism (which would make scripts much more complex however).

I'd prefer to avoid using return task.Result and friends, so that I don't get into a deadlock eventually.

Before investigating this further, did anyone actually attempt any async/await support in Jint? @sebastienros is this something you'd be interested in supporting, e.g. through the native ECMA-262 async/await (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function)? Any failed or partial attempt?

To avoid any confusion, given this C# code:

public async Task<string> loadSomething(string path) {
  return await File.ReadAllTextAsync(path);
}

I'd consider any of these JavaScript equivalents to be correct (in order of preference):

// Pausing (easiest for script writers):
var x = loadSomething('test.txt');
// Async Function (concise)
var x = await loadSomething('test.txt');
// Promises (well known)
var x;
loadSomething('test.txt').then(function(result) { x = result });;
// Callbacks (worst case)
var x;
loadSomething('test.txt', function(result) { x = result });;

Any ideas or feedback on that is welcome; I'd like to avoid spending any time on this if it would not be merged, or if the effort is too large for what I can afford.

@ackava
Copy link

ackava commented Jun 25, 2018

Can you not convert simple callback into promise using following?

     public class AsyncService {
          public void LoadAsync(JValue input, JValue then, JValue failed) {
                 Task.Run(async () => {
                         try {  
                                 var result = await ..... async processing in C#
                                 then.Invoke( null, result );
                         } catch(Exception ex) {
                                 failed.Invoke(null, ex.ToString());
                         }
                  });
          }      
     }
             loadAsync( input ) : Promise<any> {
                  return new Promise((resolve,reject) => {
                           asyncService.loadAsync( input, resolve, reject);
                  });
             }


     // now you can use loadAsync method with async await

@christianrondeau
Copy link
Contributor Author

@ackava thanks for sharing your ideas. It looks like you are using TypeScript, which would explain why using the Promise constructor, the => syntax and eventually using await would work for you :) Using a transpiler is an idea worth investigating though.

I realize that supporting async/await would pretty much be supporting ES2017, which has it's own open issue: #343 - this would probably (I can only guess) solve my issue, depending on how @sebastienros decided to implement "await" for C#-backed functions. For now, the Promise constructor looks like it's not implemented in the 3.0 branch, so I guess we'll have to wait and see. If I had more time, I'd try contribute this back in Jint, but I don't think I'll be able to take the lead on this for some time.

If someone else wonders how to do transpiling in Jint: #274 (did not try it yet)

@ta10212
Copy link

ta10212 commented Aug 16, 2018

Hi everyone, I echo @christianrondeau in the desire for async/await support.

While promises would work, it would be even simpler if the Engine could just "await" a function delegate passed to "SetValue" - if that delegate wrapped an asynchronous method.

Suppose we had the following C# code:

Engine engine = new Engine(cfg => cfg.AllowClr());
Func<string, Task<string>> thisFunc = DoOperation;
engine.SetValue("DoOperation", thisFunc);

//C# method of DoOperation
public async Task<string> DoOperation(string input)
{
   //assume that SomeAsynchronousOperation returns a string...

   return await SomeAsynchronousOperation();
}

The ideal scenario would then be for an "ExecuteAsync" method on the engine like:

engine.ExecuteAsync("DoOperation('my_input');");

...where internally the engine would "await" the function delegate at the time of invocation.

@sebastienros , is this at all possible? If not, are there any other workarounds you might suggest?

Jint is a phenomenal piece of software though in my particular case, I'm dealing with heavy network I/O. As such, async/await support is critical.

@ellern
Copy link

ellern commented Nov 12, 2018

Has anybody had any progress on this issue that's worth sharing? I'm in a situation where I need to call some async methods.

@jbourny
Copy link

jbourny commented Nov 29, 2018

Hello,
I've maybe a solution for you, it doesn't use await syntax but you can use then(...) :

C# function to call from JS :

public static Promise<bool> alert(string msg)
        {
            return new Promise<bool>(Task.Run<Task<bool>>(new Func<Task<bool>>(async() => { 
            
                bool isFinished = false;
                Device.BeginInvokeOnMainThread(async () =>
                {
                    await App.Current.MainPage.DisplayAlert("Message", msg, "Ok");
                    isFinished = true;
                });
                while (!isFinished) await Task.Delay(10);
                return true;
            })));
        }

C# Class "Promise" :

public class Promise<T>
    {
        Task<T> tache;
        public Promise(Task<T> tache)
        {
            this.tache = tache;
        }
        public Promise(Task<Task<T>> tache)
        {
            this.tache = tache.Result;
            
        }
        public void then(Action<T> onfinish)
        {
            this.tache.ContinueWith(rep =>
            {
                onfinish.Invoke(rep.Result);
            });
        }

    }

Javascript code :

var repAlert=alert("valeur interne = "+valeur);
	repAlert.then(function(rep){
		console.log("journal ok rep="+rep); // return Boolean value (true)
	});
	console.log("journal ok repAlert="+repAlert); // Return Promise object

The catch function is not implemented but you'll can do with this example...

This code is a part of a Xamarin project

@ChrML
Copy link

ChrML commented Mar 31, 2021

Bumping this one for a question: Since Jint is an interpreter, are there any reasons that C# methods cannot be async/return Task, but made look synchronously to the user-script?

I have a case with very many small simple scripts, that should call async methods. I'd like to avoid the complexity of introducing await/Promise objects to the user scripts, because none of the scripts have parallelism requiring actual async code.

@ayende
Copy link
Contributor

ayende commented Apr 1, 2021

You want to be able to do await, then? But do that automatically.
One of the purposes of the Task API is to let you run things in the background, after all.

@ChrML
Copy link

ChrML commented Apr 1, 2021

You want to be able to do await, then? But do that automatically.
One of the purposes of the Task API is to let you run things in the background, after all.

Yes, but the user scripts in my case don't really need it. Still, the host process needs it to avoid blocking the thread.

@ayende
Copy link
Contributor

ayende commented Apr 1, 2021

That might actually be a good idea to support, so the API isn't async based to the script, but is async for the usage.

@lahma lahma added the es6 label May 17, 2021
@lahma
Copy link
Collaborator

lahma commented May 17, 2021

Promise support has been implemented, but no support for async/await keywords yet.

@John0King
Copy link

John0King commented Aug 12, 2022

May I ask what is block on this ?
C# support callling any type that implment GetAwater() and result that implement ICriticalNotifyCompletion, and we can use TaskCompletionSource or IValueTaskSource to wrap any type/method that have callback action.

I wasn't see any problem from C# side (both js->c# and C# -> js)

@ackava
Copy link

ackava commented Aug 12, 2022

@John0King Its been long since I answered, I have created my own JavaScript engine in C# which supports most of features of ES6 through ES2019. You can try and let me know.

@lahma
Copy link
Collaborator

lahma commented Oct 20, 2022

Jint now has async/await support (Promise-wise). I'm not sure if it answers the need in this issue though? Anyone want to extend the support to interop side (task.GetAwaiter().GetResult() probably)? Or can we close this as "too generic"?

@viceice
Copy link
Contributor

viceice commented Oct 20, 2022

so Jint supports native async / await, but doesn't convert c# Task and others to Promise and vice versa yet?

@lahma
Copy link
Collaborator

lahma commented Oct 20, 2022

Native for .NET doesn't exist yet, only the JS fullfill/reject chaining. Some brave soul could implement wrapping Task into Promise.

@viceice
Copy link
Contributor

viceice commented Oct 20, 2022

ok, will try to have a look at it, as that's now the missing blocker for async usage 😅

@lahma
Copy link
Collaborator

lahma commented Oct 20, 2022

Probably something like adding task with GetAwaiter().GetResult() to event loop and resolving to its result.

@christianrondeau
Copy link
Contributor Author

Pretty please don't do that :) The whole point of await is freeing the thread and allowing the use of synchronization contexts. Really the only desirable implementation will be painful but very valuable : async all the way (TM) meaning ExecuteAsync, AwaitAsync and friends. If you want I can provide more material to back this, we've been through this in the past too :)

@lahma
Copy link
Collaborator

lahma commented Oct 20, 2022

@christianrondeau got your attention, now waiting for the PR 😉

@viceice
Copy link
Contributor

viceice commented Oct 20, 2022

but we need to make sure there are no real parallel tasks, as it's not expected from JavaScript. there should only be one thread at a time, otherwise it would make interoperability much more complicated.

@lahma
Copy link
Collaborator

lahma commented Oct 20, 2022

I think that would mean adding the tasks to main event loop and blocking and processing with RunAvailableContinuations, even if that sounds horrible, it's JS after all.

@christianrondeau
Copy link
Contributor Author

christianrondeau commented Oct 20, 2022

Haha yeah I'd love to, but I'm also struggling with time :) This being said, the reason I didn't even try is because I don't understand promises internally, and I know it'll be another huge PR with tons of breaking signature changes.

Or duplicate every involved method to avoid the (minimal) overhead of the Task.

JS can be single threaded, but the promise itself can definitely wait on a C# task that is bound to multiple async tasks (Task.WhenAll). What's important is that after the promise completes, control is given back to the synchronous single threaded javascript code (like node).

My idea that I didn't validate yet was to have a duplicated Execute and Evaluate methods as well as a duplicated RunAvailableContinuations (async and not async), and throw in the non async RunAvailableContinuations if a promise is bound to a Task that is not completed. Not sure if it's that simple in practice though.

I guess the very first question to ask is whether you want to offer only async execute and evaluate methods and pay the small overhead, or provide both signatures and potentially have duplicated methods and tests.

@christianrondeau
Copy link
Contributor Author

I can definitely support @viceice and maybe even try a prototype branch, since I'm the one who opened the issue one year ago... And to be honest, I have to do some gymnastics to work around this so I'd definitely still make use of it. Can't promise anything though.

@lahma
Copy link
Collaborator

lahma commented Oct 20, 2022

I do have interest in performance and solutions supporting it, but I'm afraid that the true async paths and dispatch to pool would lose the benefits just by the overhead of the interpreter.

Of course hard to measure when only sync path available (in Jint). Async shines in high concurrency but I'm unsure if Jint or any JS interpreting runtime will benefit of it.

@christianrondeau
Copy link
Contributor Author

christianrondeau commented Oct 20, 2022

Actually the main value is to use IO ports instead of locking a thread in the ThreadPool (asp.net being the most obvious example). That was also the great selling point of NodeJS when it came out, not concurrency (well, concurrency of the server, not of the script itself).

It's also very true for Jint, as right now loading a file or accessing resources on a blob storage or any remote location (or use SQL etc) will lock the web thread and eventually kill your web server scalability if you use GetAwaiter as a workaround to provide synchronous methods to Jint. So implementing fake async using GetAwaiter would actually be a trap for novice and uninformed developers using Jint, who may discover they suddenly require more machines with idle CPUs to support the same load.

I hope this makes sense, I can try and provide some more thorough explanation and references if you're not convinced yet :)

@lahma
Copy link
Collaborator

lahma commented Oct 20, 2022

I think a demo using Jint with different approaches would be great. That would clarify the benefits and probably show the benefits. I have no idea what percent of users jump to actual waiting I/O from engine.

@christianrondeau
Copy link
Contributor Author

christianrondeau commented Oct 20, 2022

I'll try and do two things then (I knew this would come back and bite me!)

  1. I'll at least try to see if I can make an incomplete and poorly tested PR to help scope the actual work and changes involved, and;
  2. Make a case for the need to have a good implementation of async

What I cannot do is estimate how many people would benefit, but anyone who runs Jint in a web server and has any kind of file, http, tcp wait will greatly benefit under scale (not under low volume though).

@lahma
Copy link
Collaborator

lahma commented Oct 21, 2022

Awesome! One thing to note is that we probably cannot have async/await as c# constructs at the heart of runtime (expression and statement evaluation), that would probably break both performance and ability have deeply nested calls (stack grows a lot with async state machines).

@lahma
Copy link
Collaborator

lahma commented Oct 21, 2022

Also linking earlier discussion here: #1081

@christianrondeau
Copy link
Contributor Author

Thanks for the link, I can see I'm not the only one who thought of this ;)

But I just thought of another MUCH simpler approach... I'll try and confirm if it could work but I don't see why not.

  1. Replace the foreach in JintStatementList by an IEnumerator (while(statements.MoveNext());)
  2. Set that enumerator into Engine as an internal property
  3. When hitting an await statement in the enumerator function...
  4. If the result is a resolved promise, just continue
  5. If the result is an unresolved promise, do context.Engine.Waiting = theAwaitableObject, and stop enumerating
  6. The current implementation of  RunAvailableContinuations would simply check whether theAwaitableObject has been resolved, and if so, simply resume the enumerator

And that's it. If I didn't miss something, this would rid of all the complexity surrounding the event loop. After all, .NET already has a lot of the mechanisms we need for this.

Then, adding actual async to this becomes quite simple, we only need to have a Promise implementation that contains the Task object and await it.

Do you see something wrong with this? I'll try and see if it's as simple as that in practice.

@christianrondeau
Copy link
Contributor Author

Wait a second, you already implemented some of it as part of #1323 - that'll make things easier :)

@lahma
Copy link
Collaborator

lahma commented Oct 21, 2022

When touching JintStatementList my main concern is performance. Might be a good idea to first try changing to enumerator (and other required constructs) inside of it and then check what for example DromaeoBenchmark reports after that. It's a very hot code path. There was a painful optimization spree recently too where stack size was tuned - there's a test case for that too ShouldAllowReasonableCallStackDepth.

Maybe also worth a look how other engines handle such things, like Nill.JS.

Wait a second, you already implemented some of it as part of #1323 - that'll make things easier :)

Oh did I 😆

@christianrondeau
Copy link
Contributor Author

Soooo I made a quick and dirty test, and changed the implementation of JintStatementList to something like this:

                using var enumerator = EnumerateStatements(context).GetEnumerator();
                while (enumerator.MoveNext())
                {
                    c = enumerator.Current;
                    if (c.Type != CompletionType.Normal)
                    {
                        return new Completion(c.Type, c.Value, c._source);
                    }
                    lastValue = c.Value;
                }

and

        private IEnumerable<Completion> EnumerateStatements(EvaluationContext context)
        {
            foreach (var pair in _jintStatements!)
            {
                var s = pair.Statement;
                var c = pair.Value.GetValueOrDefault();
                yield return s.Execute(context);
            }
        }

So, nothing fancy but this should capture the state of the executing code and therefore have most of the memory impact.

With simple foreach

| Method | Prepared |             FileName |       Mean |      Error |      StdDev |     Median |       Gen0 |       Gen1 |       Gen2 |     Allocated |
|------- |--------- |--------------------- |-----------:|-----------:|------------:|-----------:|-----------:|-----------:|-----------:|--------------:|
|    Run |    False |      dromaeo-3d-cube |  37.707 ms |  0.1477 ms |   0.1382 ms |  37.690 ms |  1071.4286 |   214.2857 |          - |    6736.09 KB |
|    Run |    False |    dromaeo-core-eval |   8.631 ms |  0.0276 ms |   0.0258 ms |   8.632 ms |    62.5000 |    15.6250 |          - |     321.64 KB |
|    Run |    False | dromaeo-object-array |  94.079 ms |  0.3010 ms |   0.2668 ms |  94.056 ms | 21833.3333 |  1333.3333 |   166.6667 |  101909.16 KB |
|    Run |    False | droma(...)egexp [21] | 527.562 ms |  2.6257 ms |   2.4561 ms | 528.352 ms | 21000.0000 |  8000.0000 |  5000.0000 |  179734.59 KB |
|    Run |    False | droma(...)tring [21] | 689.923 ms | 36.5190 ms | 107.6771 ms | 714.966 ms | 52000.0000 | 23000.0000 | 20000.0000 | 1326278.65 KB |
|    Run |    False | droma(...)ase64 [21] |  91.320 ms |  0.4886 ms |   0.4570 ms |  91.336 ms |  1500.0000 |   166.6667 |          - |    7658.71 KB |
|    Run |     True |      dromaeo-3d-cube |  36.319 ms |  0.1105 ms |   0.0980 ms |  36.326 ms |  1357.1429 |   214.2857 |          - |     6441.6 KB |
|    Run |     True |    dromaeo-core-eval |   8.715 ms |  0.0204 ms |   0.0190 ms |   8.716 ms |    62.5000 |          - |          - |        309 KB |
|    Run |     True | dromaeo-object-array |  93.430 ms |  0.3545 ms |   0.3316 ms |  93.404 ms | 21833.3333 |  1333.3333 |   166.6667 |     101870 KB |
|    Run |     True | droma(...)egexp [21] | 310.598 ms |  2.1691 ms |   2.0290 ms | 310.822 ms | 20000.0000 |  7000.0000 |  4000.0000 |  175229.21 KB |
|    Run |     True | droma(...)tring [21] | 658.091 ms | 50.1232 ms | 147.7893 ms | 724.897 ms | 61000.0000 | 32000.0000 | 29000.0000 | 1326124.82 KB |
|    Run |     True | droma(...)ase64 [21] |  91.512 ms |  0.5171 ms |   0.4837 ms |  91.476 ms |  1500.0000 |   500.0000 |          - |    7569.46 KB |

With IEnumerator

| Method | Prepared |             FileName |      Mean |     Error |     StdDev |    Median |       Gen0 |       Gen1 |       Gen2 |  Allocated |
|------- |--------- |--------------------- |----------:|----------:|-----------:|----------:|-----------:|-----------:|-----------:|-----------:|
|    Run |    False |      dromaeo-3d-cube |  39.96 ms |  0.245 ms |   0.229 ms |  40.01 ms |  1538.4615 |   230.7692 |          - |    8.65 MB |
|    Run |    False |    dromaeo-core-eval |  10.23 ms |  0.061 ms |   0.057 ms |  10.23 ms |   656.2500 |   203.1250 |          - |       3 MB |
|    Run |    False | dromaeo-object-array |  96.60 ms |  0.459 ms |   0.429 ms |  96.60 ms | 22000.0000 |  1333.3333 |   333.3333 |  100.81 MB |
|    Run |    False | droma(...)egexp [21] | 318.88 ms |  1.924 ms |   1.800 ms | 318.78 ms | 21000.0000 |  7000.0000 |  4000.0000 |  180.29 MB |
|    Run |    False | droma(...)tring [21] | 570.04 ms | 40.221 ms | 118.591 ms | 569.87 ms | 52000.0000 | 22000.0000 | 19000.0000 | 1298.68 MB |
|    Run |    False | droma(...)ase64 [21] |  96.51 ms |  0.398 ms |   0.372 ms |  96.56 ms |  2333.3333 |   500.0000 |          - |   11.15 MB |
|    Run |     True |      dromaeo-3d-cube |  38.73 ms |  0.111 ms |   0.098 ms |  38.77 ms |  1846.1538 |   384.6154 |          - |    8.36 MB |
|    Run |     True |    dromaeo-core-eval |  10.30 ms |  0.025 ms |   0.021 ms |  10.30 ms |   656.2500 |   156.2500 |          - |    2.99 MB |
|    Run |     True | dromaeo-object-array |  94.29 ms |  0.279 ms |   0.261 ms |  94.34 ms | 22000.0000 |  1333.3333 |   333.3333 |  100.77 MB |
|    Run |     True | droma(...)egexp [21] | 310.69 ms |  1.196 ms |   1.060 ms | 310.66 ms | 22000.0000 |  9000.0000 |  5000.0000 |   181.1 MB |
|    Run |     True | droma(...)tring [21] | 650.93 ms | 51.067 ms | 150.572 ms | 725.84 ms | 52000.0000 | 22000.0000 | 19000.0000 | 1298.66 MB |
|    Run |     True | droma(...)ase64 [21] |  99.81 ms |  0.271 ms |   0.240 ms |  99.82 ms |  2400.0000 |   400.0000 |          - |   11.06 MB |

But I think the IEnumerator idea still makes sense, but by implementing it manually in every JintStatement class would have pretty much zero impact, and might be necessary anyway.

The problem is that JintStatementList is called from multiple, potentially deep places, and we need to halt execution in the middle of something. So everything that can run a JintStatementList needs to also pause and return back to the Engine.

So, here's what I'm going to try next:

  1. On every JintStatement, instead of Completion Execute(EvaluationContext) I'll have bool MoveNext() (which does the actual execution) and Completion Current {get;} (which returns whatever was the last completion).
  2. Test for perf again, though that part does not worry me at all. It's just going to be a big commit.
  3. Everywhere we run a JintStatementList, after calling MoveNext, check if we should wait (await + unresolved promise or parent of) and if so, return, only keeping the index of the statements array.

I'm still unclear as to what the event loop implementation does though. Maybe it'll be useless after this or maybe I'm completely off track. I'll report back.

@lahma
Copy link
Collaborator

lahma commented Oct 21, 2022

I think you are nearing the generators problem zone (unfinished PR). Statement list needs to be able to pause and snapshot state to continue from when called next time. Generally "resume from index 3".

@christianrondeau
Copy link
Contributor Author

christianrondeau commented Oct 21, 2022

Aaah so you're saying that instead of actually pausing the JintStatementList and resuming it, that would be breaking the whole parsed script in chunks up front, each chunk being a piece of code that can run until an await statement? I hope that made sense, I'm not aware of how generators work but I didn't think it would span across functions. Do you believe I should stop what I'm currently doing?

For example, you should be able to do things like:

let x = 0;
function fn1() {
  await something;
  x++;
}
await fn1();
if(x != 1) throw;

So the await can actually return control back to C# and resume later.

Oh and by the way, this is mean:

let fn;
fn = function(t) {
  if(t > 0) {
    await fn(t--);
  }
}
await fn(5);

Right now, the JintFunctionDefinition is reused in recursive calls, which complicates things a little (e.g. it broke the Prism test). Instead of having a state directly in JintStatementList, I'll need to create a JintStatementListInvocation (or something) so multiple instances of it can keep the state. But otherwise, I think it'll work (unless you tell me to drop it) but it will (again) involve quite a lot of code.

For now I'll try and see how far I can go with it and reach out when I have an actual working example of what I'm going for.

@christianrondeau
Copy link
Contributor Author

christianrondeau commented Oct 21, 2022

So, less words, more code. Here's the idea (just a small piece to make sure I'm not going into a wall). The JintStatementList.Execute function uses an enumerator instead of directly executing the code.

The idea from this point is to replace the JintStatementList.Execute (which enumerates) by JintStatementList.GetEnumerator (Which would return the enumerator): https://github.com/christianrondeau/jint/blob/gh-514-async/Jint/Runtime/Interpreter/JintStatementList.cs#L78

Everything that calls Execute would need to instead get the enumerator and loop through it, returning when it hits an await operation. You can see how it affects quite a lot of code, however most classes would simply return the Completion as-is (e.g. a function call returns a promise, we don't care if it's resolved or not, etc.) so I don't think it's that bad either.

So, what do you think? Is that worth pursuing?

@christianrondeau
Copy link
Contributor Author

To keep you in the loop.

  • I've been reading https://tc39.es/ecma262/#await and really wondering. It says "suspend" and "resume" as if it was magic; I understand from that that while the running evaluation context is determined, how you actually pause execution is up to the implementer. In that case, I think the approach makes sense; resuming would also restore the corresponding evaluation context, but I think that's an easy one.
  • JintAwaitExpression.EvaluateInternal returns a JsValue (object but it's force-casted in JintExpression.GetValue later). Shouldn't this return a Completion instead? I can work around that but I'm wondering if that's how it should be or if it's something that should be changed (and potentially depended on).

For the sake of the prototype, I'll instead make a SuspendPromise that inherits from JsValue but that code will be thrown away.

@christianrondeau
Copy link
Contributor Author

christianrondeau commented Oct 21, 2022

All right I'll have to stop for a while. Instead of returning a weird SuspendPromise, I set engine._suspend = true; engine._suspendValue = promise and return null. That makes sense, however this means EVERY operation that can be followed by await (const x = await something, for(x = await something; x < await... you get the point) needs to check whether the operation has to be halted, and allow resuming.

https://github.com/christianrondeau/jint/blob/gh-514-async/Jint/Runtime/Interpreter/Expressions/JintAwaitExpression.cs#L42

NiL.JS have an elegant solution to this, they wrap (at parse time I think) all those operations in a SuspendableExpression, which takes care of checking whether post-execute, the engine is suspended. However most of Jint expressions would require the ability to deal with resuming anyway, unless we decompose things even more than they currently are.

I'm out for now :) I'll probably continue investigating this sometime in the future though.

@lahma
Copy link
Collaborator

lahma commented Oct 22, 2022

I'm trying to answer/comment at least some bits, please re-ask the parts I'm missing 😉 I'll be mixing generators concepts here too so be aware...

Right now, the JintFunctionDefinition is reused in recursive calls, which complicates things a little (e.g. it broke the Prism test). Instead of having a state directly in JintStatementList, I'll need to create a JintStatementListInvocation (or something) so multiple instances of it can keep the state. But otherwise, I think it'll work (unless you tell me to drop it) but it will (again) involve quite a lot of code.

I was thinking for generators case to use same construct as Nill.JS has, basically a Dictionary<object, object> in engine that would allow script part (like JintStatementList) contain state and restore when needed via engine._suspendData[this].

Generators need this in all funky places, like in try-catch-finally, it's ok to even yield from finally block.

So, less words, more code. Here's the idea (just a small piece to make sure I'm not going into a wall). The JintStatementList.Execute function uses an enumerator instead of directly executing the code.

So far your JintStatementListEnumerator looks like the old for-loop, just with the state. Could the suspendData dictionary usage bring the needed functionality to existing solution without sacrificing performance? So generators could share extend the State class instance tailored for this.

I've been reading tc39.es/ecma262/#await and really wondering. It says "suspend" and "resume" as if it was magic; I understand from that that while the running evaluation context is determined, how you actually pause execution is up to the implementer. In that case, I think the approach makes sense; resuming would also restore the corresponding evaluation context, but I think that's an easy one.

Currently the suspend for await is just letting the engine run it's continuoations as then we should expect all promises to be settled. await is kind of a thread yield I guess...

JintAwaitExpression.EvaluateInternal returns a JsValue (object but it's force-casted in JintExpression.GetValue later). Shouldn't this return a Completion instead? I can work around that but I'm wondering if that's how it should be or if it's something that should be changed (and potentially depended on).

I think there shouldn't be need for completion type for expression. For yield we will need to signal suspend, but ECMA spec doesn't have suitable return type in CompletionType, at least when I tried in generators branch - so the suspendData + some flag is needed.

christianrondeau/jint@gh-514-async/Jint/Runtime/Interpreter/Expressions/JintAwaitExpression.cs#L42

This is now "competing" a bit with UnwrapIfPromise? I think people need public API to get value out from engine (maybe running the continuoations too). But the change here looks good with extra state checks etc. Conceptually this is a bit hard for me to understand as await should always block until resolve. So the return null seems a bit wrong.

EDIT

Right now, the JintFunctionDefinition is reused in recursive calls, which complicates things a little (e.g. it broke the Prism test). Instead of having a state directly in JintStatementList, I'll need to create a JintStatementListInvocation (or something) so multiple instances of it can keep the state. But otherwise, I think it'll work (unless you tell me to drop it) but it will (again) involve quite a lot of code.

That's a good point. Each invocation has its own execution context so that might help with this, not sure how though. JintFunctionDefinition should only contain static data so if there's per-invocation race it has wrong state persisted.

@christianrondeau
Copy link
Contributor Author

Thanks for taking the time. You can ignore piggybacking on Completion or JsValue, that's clearly wrong (it was more of a way to make a cheap and fast proof of concept).

In the end, the challenge I see is mostly suspending execution. For example, here: https://github.com/sebastienros/jint/blob/main/Jint/Runtime/Interpreter/Expressions/JintUnaryExpression.cs#L70 you'd need to wait for the value before continuing. You have to options. Either you store the execution context with the closure, save the state, and write code to resume it later, OR you take advantage of .NET doing this through Task and async, but you have to pay a small memory and perf cost (maybe acceptable, we'd have to try).

Going the "async all the way" approach (which is what I'd strongly recommend in almost every project using any kind of file or network IO) would solve all problems at once, really. The only "but" is the overhead, and it's significant enough to consider in cases of 5 + 5 versus (await 5) + (await 5) which are both valid. This is what I'd try first, but it's also a breaking change since now Engine functions must be async.

Going the "suspend" approach means every expression must have a state (or instantiate something that holds the state, like I did for the statements list). Whether you use an enumerator-like approach or a state dictionary (enumerators would be better IMHO since you avoid having to clean up state and you don't have to do a hash check at all) works anyway.

If I got this right, "generators" are similar to C#'s "yield" statements on IEnumerable functions; however this is limited to function invocation, it won't actually suspend execution, right? In other words, should I hold for some additional work on Jint or is it worth it to continue exploring? As usual, I want to help, not generate additional noise :)

@ChrML
Copy link

ChrML commented Oct 22, 2022

Great discussion. I have a bit of input:

Please don't go for a synchronous approach (something like GetAwaiter().GetResult). In fact it's better to not support await at all than such approach, or atleast throw a NotSupportedException indicating that ExexuteAsync must be used to execute this expression.

Tasks don't imply concurrency. You can use tasks in a singlethreaded environment. The purpose of a task is to yield the thread for something else to do useful work while the script is waiting for some event to occur (such as a network response), so that the script can continue in the same singlethreaded context (but doesn't need to be the same thread) when completed.

My usage of JINT is running a bunch of small user scripts (1000's). If we support async and the user wants to e.g. call await API.Delay(30000); to wait 30 seconds before doing something, a synchronous approach would starve the application by needing to spawn 1000 threads just idling around doing nothing. When in fact 1 thread could do all the work, and other useful stuff while the scripts are just waiting.

NET has a very powerful system already for handling and queuing tasks. By utilizing it, in practice, a few threads could run 1000 separate JINT scripts, and not neccessarily restricting one thread to one JINT engine. The script could start on one thread, await some network IO, and then continue to run on another thread. Still singlethreaded from the engine/script's point of view because there is no concurrency.

@christianrondeau
Copy link
Contributor Author

Agreed @ChrML. To be clear on the "suspend" approach, it doesn't suspend the thread, it just stops evaluating where it is and returns to the caller. Meaning you could do:

// something is a promise mapped to a C# async function
engine.Evaluate("await something; dosomething();");
await stuffThatResolvesSomethingAsync();
engine.Resume(); // run continuations

So, no thread locking and no actual Task/async/await support in Jint but the ability to suspend would already allow async, despite not in a nice and user friendly way.

Right now to work around this, I'm doing (simplified):

const tasks = engine.Evaluate("user code returning loading commands...")
await actualLoadingOfStuff(tasks);
engine.Evaluate("other user code acting on the loaded stuff...")

It complicates things, but it works. I do think that many people won't hit high server concurrency nor be under the risk of running malicious user scripts, but I still wouldn't make thread locking a default behavior.

Now the challenge is to make it work ;)

@ChrML
Copy link

ChrML commented Oct 22, 2022

@christianrondeau With such approach, one could implement a more user-friendly ExecuteAsync with this pseudo-code:

public Task<JsValue> ExecuteAsync(Script script, CancellationToken ct)
{
    JsValue result = this.Execute(script);
    while (this.HasAsyncContinuation)
    {
        ct.ThrowIfCancellationRequested();
        await this.CallCurrentAwaitedTask(ct);  // Pass ct automatically to the user's async method if it's the last argument for cancellation.
        result = this.Resume();
    }
    return result;
}

@christianrondeau
Copy link
Contributor Author

That's exactly it, yes; this (the suspend approach) has the advantage of not introducing Tasks everywhere in the code (async all the way), but it does increase complexity since you need every expression to be able to suspend and resume (unless there's some generator magic I'm missing ;) )

@ChrML
Copy link

ChrML commented Oct 22, 2022

That's exactly it, yes; this (the suspend approach) has the advantage of not introducing Tasks everywhere in the code (async all the way), but it does increase complexity since you need every expression to be able to suspend and resume (unless there's some generator magic I'm missing ;) )

I suspect the suspend logic will already be needed to implement JS generators, to some extent. So this could be an extension of that.

I agree that tasks all the way would be unneccessary, and probably make the performance lower for scripts that don't need it. So I'm all for the suspend approach.

@ackava
Copy link

ackava commented Oct 25, 2022

I went through lot of answers and couldn't help putting few points, I hope it helps.

  1. Async/Await is a simple wrapper over generator, check how typescript transpiles async/await to ES6 code, await is replaced with yield. https://www.typescriptlang.org/play?target=2#code/G4QwTgBA7g5gpgFwgXgiAzgTwHYGMIAUArmADYCUKAfBAN4BQETEuA9tukmHOimlCACWSAGaJcAC2JlyAbkbM2HJAjgAPJKhADhEbugB0qjQTkKmS9K1JwDpVjALGEZgL5A , in this example, engine needs only a generator.
  2. Generator is different from the IEnumerable in C# as generator can receive the input parameter, so the next awaitable promise can be passed to generator to form a promise chain.
  3. There is no async JSValue, or no AsyncPromise, basically await can be used with any object that has then method. Take a look at the following code.
var a = async () => {
    return await {
        then(n) {
            console.log("Then called");
            n("result");
        }
    };
}
a().then((r) => console.log(r));
  1. Eval must return a simple JSValue, which will contain then, catch and finally methods. It will be caller's responsibility to hook then to create Task. as shown below in an extension method..
  Task<JSValue> ToTask(this JSValue @this) {
        var then = @this["then"];
        if (!then?.IsFunction) {
             return Task.FromResult(@this);
        }
        var tcs = new TaskCompletionSource();
        then.InvokeFunction(
             new JSFunction( ...  => tcs.TrySetResult(arg0) ),
             new JSFunction( ...  => tcs.TrySetException(arg0.ToException()) );
        return tcs.Task;
  }
  1. Generator can be implemented in two ways (running generator on separate thread, and transpiling code to a state machine), I have tried both, first one is quick and easy, but very slow in performance.
  2. Separate thread generator is very easy to implement, pause current thread and run generator on a different thread till it reaches return/fails or it reaches yield. Current thread can continue when generator stops. All the states are easily maintained with only one issue that generator function does not run on main thread, causing problem for UI applications. And performance is slow for deeply nested generators.
  3. Transpiling into a state machine is complex to write but has excellent performance. First step is to change all loops and nested scopes into a simple flat scope, so you can put simple jumps between the statements. Because jumping in and out of nested scopes are not possible. Implementing TypeScript's generator transformation is quite complex.
  4. In case of interpretation, if we can create a concept of virtual thread, implementing generator could be simple. Real threads are very expensive.

@christianrondeau
Copy link
Contributor Author

Thanks, in my personal case, anything that helps me better understand is welcome! I'm not clear on 2 however, I tried to re-read about generators and it looks like C#'s yield return statements. Maybe I should just wait for @lahma to write the generators ;)

I'd argue against using a thread though (I feel like everyone's agreeing already), since it would be worse than GetAwaiter(), since we'd use two threads now instead!

Transpiling in a flat scope sounds like the best option, though I'm not in a position to comment since I'm not familiar of what this actually involves. But it would make pausing and resuming straightforward.

For now, I still feel (emphasis on me not feeling comfortable enough to back this up with verifiable claims) that implementing the enumerable pattern on expressions to track state, and allowing to pause the engine and resume it within each expression would have both a low performance cost and wouldn't require too much refactoring; I also think that waiting to see how generators will be implemented would be wiser than trying to short-circuit that discussion. So for now, I'll watch but will be available-ish if you need more info on my PoC branch's approach (I'll leave it as-is for now).

@EnCey
Copy link
Contributor

EnCey commented Nov 3, 2022

What I cannot do is estimate how many people would benefit, but anyone who runs Jint in a web server and has any kind of file, http, tcp wait will greatly benefit under scale (not under low volume though).

Yes, here! Just to add another vote to please not use GetAwaiter().GetResult() by default, we're building a simple workflow engine for our service based on Jint and async I/O is essential – pretty much like what @ChrML described already.


In case it is relevant to this discussion as an example use case, my approach to supporting async C# code with Jint promises is to capture all C# Tasks in a companion object to the Engine (AsyncContext) so that they can be awaited.

Basically there's an ExecuteAsync method that does this: (prototype code)

public async Task ExecuteAsync() {
  _engine.Execute("...");
  await asyncCtx.WhenAllPromisesAreSettled().ConfigureAwait(false);
}

From what I gathered in the discussion, this is similar to what @christianrondeau describes on a lower level, i.e. storing any "contiuations" that were created by a script and then executing them.

From my point of view, ideally Jint would provide an async overload of Execute/Evaluate for users who want to use async/Tasks ("async all the way" style) and the existing methods simply throw if a Task was captured.

@danbopes
Copy link

danbopes commented Jan 3, 2023

I would love to see something like what EnCey prototyped implemented. Right now, there are no easy ways to schedule task continuations, and wait for them to all finish asynchronously.

@EnCey
Copy link
Contributor

EnCey commented Jan 18, 2023

In case anyone wants to try it: my prototype doesn't work with the async/await keywords (in JavaScript code), but it works with regular JavaScript promises.

The reason is that Jint's await implementation unwraps a promise & throws if it isn't settled, and naturally a promise backed by an async C# Task won't be settled most of the time.
The problem doesn't exist with regular promises because the unsettled promise is returned as result of the synchronous execution, at which point my prototype awaits all Tasks and then executes the promise callbacks.

@danbopes
Copy link

danbopes commented Jan 19, 2023

@EnCey I attempted to tackle this issue on my own, and this was the problem I ran into as well. There isn't some magical way to stop execution in the javascript engine, and asynchronously wait the result. The solution I came up with, unfortunately does use the .GetAwaiter().GetResult() which isn't ideal, but since I've got a thread dedicated to executing the engine, I don't mind it tying up while it's waiting for the result. When I initialize a promise, I pass the task to the promise. When the engine attempts to get the value from a promise that hasn't resolved yet, I make it wait for the task synchronously and then return the result. This still does make it async in nature, because you can fire off multiple tasks in the background, but as soon as the engine needs one of the results, it will wait synchronously for it.

When I attempted to do it the "right way", which involved async ALL the way down, the code became sloppy. All of the .Execute() and .GetValue()'s for everything need to be turned into .ExecuteAsync() and .GetValueAsync(), which complicates things to a big degree. I don't see anyway to "pause" execution, since the .GetValue() is directly trying to get the value of that function, so it's not a simple task to turn one awaiter into an .ExecuteAsync().

@christianrondeau
Copy link
Contributor Author

It might be better to consider using ValueTask instead of task, but yeah either we have async all the way (simple but doubles everything if we really want to keep a non async version) OR we have a mechanism to save execution state and resume it later (this would be the optimal solution but it's not obvious to implement)

@EnCey
Copy link
Contributor

EnCey commented Jan 19, 2023

A third option might be to transpile the JavaScript code first to not use async/await, but regular promises. There's an issue here somewhere that describes using the TypeScript compiler to transform JS code. I've played with that and it did work, allowing me to use async/await in TypeScript code and my prototype to execute the resulting JS code.

One problem here is that without source maps, any error messages are not helpful to users, as they refer to the (messy) generated code. The elephant in the room of course being how (in)efficient it is to run the entire TypeScript compiler with Jint to pre-process code (although once transpiled, the resulting JS code can be cached to reduce that impact).

@lofcz
Copy link
Contributor

lofcz commented Feb 10, 2023

This makes another case for implementing source maps support. Tracked here.

@2763071736
Copy link

This is my code

            var jintEngine = new Engine();
            jintEngine.SetValue("myAsyncMethod", new Func<Task>(async () =>
            {
                await Task.Delay(1000);
                Debug.Log("myAsyncMethod");
            }));
            jintEngine.SetValue("myAsyncMethod2", new Action(() =>
            {
                Debug.Log("myAsyncMethod2");
            }));
            jintEngine.Execute("async function hello() {await myAsyncMethod();myAsyncMethod2();} hello();");

I expect the output to be

myAsyncMethod
myAsyncMethod2

But await does not seem to wait on a C# function.
I got the following wrong result

myAsyncMethod2
myAsyncMethod

I am using v3.0.0-beta-2049

@SGStino
Copy link

SGStino commented May 5, 2023

@2763071736, i believe that's separate from the whole async/await discussion here. It looks to me like the task doesn't get converted to a promise.

Run this in roslynpad:

#r "nuget: Jint, 3.0.0-beta-2049"
using Jint;

var jintEngine = new Engine();
            jintEngine.SetValue("myAsyncMethod", new Func<Task<string>>(async () =>
            {
                await Task.Delay(1000);
                "myAsyncMethod".Dump();
                return "testvalue";
            }));
            
            jintEngine.SetValue("myAsyncMethod2", new Action<string>((string value) =>
            {
                value.Dump();
                "myAsyncMethod2".Dump();
            }));
 

jintEngine.Execute("async function hello() {  const test = await myAsyncMethod(); myAsyncMethod2(test); } hello();"); 

await Task.Delay(2000); // give roslynpad time to wait for the delay 

And you'll get the dumps:
System.Runtime.CompilerServices.AsyncTaskMethodBuilder+AsyncStateMachineBox[System.String,Program+<>c+<<<Main>$>b__0_0>d]
and
myAsyncMethod2

followed by myAsyncMethod a bit later.

but the hint here is the AsyncTaskMethodBuilder.

based on this discussion you can use the TryConvert code to wrap the Task.Delay in a promise:

#r "nuget: Jint, 3.0.0-beta-2049"
using Jint;
using Jint.Native;
using Jint.Native.Promise;

var tcs = new TaskCompletionSource<object>();

var jintEngine = new Engine();
  
jintEngine.SetValue("_resolveFinal", tcs.SetResult);

 Thread.CurrentThread.Dump("starting thread");

jintEngine.SetValue("dump", new Func<object, object>(obj => obj.Dump("jintDump")));
jintEngine.SetValue("myAsyncMethod", new Func<JsValue>(() =>
{
    var (promise, resolve, reject) = jintEngine.RegisterPromise();
    _ = Task.Delay(1000).ContinueWith(_ =>
    {
        Thread.CurrentThread.Dump("continuation thread");
         resolve(JsValue.FromObject(jintEngine, "testvalue"));
         
        "myAsyncMethod".Dump();
    });

    return promise;
}));
  
jintEngine.Execute("myAsyncMethod().then(_resolveFinal)");
 
var final = await tcs.Task;
final.Dump("final");

do mind, you'll be resolving your promise from the timer thread.

That's quickly resolved by using a SynchronizationContext from the nuget Nito.AsyncEx.Context:

#r "nuget: Nito.AsyncEx.Context, 5.1.2"
#r "nuget: Jint, 3.0.0-beta-2049"
using Jint;
using Jint.Native;
using Jint.Native.Promise;
using Nito.AsyncEx;

var tcs = new TaskCompletionSource<object>();

var jintEngine = new Engine();

jintEngine.SetValue("_resolveFinal", tcs.SetResult);


AsyncContext.Run(async () =>
{

    Thread.CurrentThread.Dump("starting thread");
    SynchronizationContext.Current.Dump();

    var scheduler = TaskScheduler.FromCurrentSynchronizationContext();
    jintEngine.SetValue("dump", new Func<object, object>(obj => obj.Dump("jintDump")));
    jintEngine.SetValue("myAsyncMethod", new Func<JsValue>(() =>
    {
        var (promise, resolve, reject) = jintEngine.RegisterPromise();
        _ = Task.Delay(1000).ContinueWith(_ =>
        {
            Thread.CurrentThread.Dump("continuation thread");
            resolve(JsValue.FromObject(jintEngine, "testvalue"));

            "myAsyncMethod".Dump();
        }, scheduler);

        return promise;
    }));
  
    jintEngine.Execute("myAsyncMethod().then(_resolveFinal)");
 
    var final = await tcs.Task;
    final.Dump("final");
});

But when trying to use the await keyword version:
jintEngine.Execute("async function hello(){ const result = await myAsyncMethod(); _resolveFinal(result); } hello()");
instead of jintEngine.Execute("myAsyncMethod().then(_resolveFinal)");

We get a Jint exception that it wants to unwrap the promise before it was resolved, indicating to me that the await isn't fully implemented as of yet?

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

No branches or pull requests