paulbartrum / jurassic

A .NET library to parse and execute JavaScript code.
MIT License
873 stars 122 forks source link

Promises #156

Closed jcdickinson closed 5 years ago

jcdickinson commented 5 years ago

This pull request completes the promise functionality. Closes #54.

jcdickinson commented 5 years ago

Oof. I noticed that these must be deferred until there is no code on the stack. Updating this to a WIP.

jcdickinson commented 5 years ago

This now conforms to the specification, ready for review.

kpreisser commented 5 years ago

Hi, thank you very much for the PR! I'm looking forward to having Promises fully implemented in Jurassic, so that I no longer need to use a polyfill for that functionality in our application.

I have skimmed through the PR, and added some comments. Overall, I think the PR looks good. Thank you!

jcdickinson commented 5 years ago

@kpreisser Thanks for the feedback! I went ahead and made sure that the implementation matches the Promises/A+ spec, so it now works alongside polyfilled promises.

paulbartrum commented 5 years ago

Thanks for this. I made a few minor changes (removed the EventLoop class, made ExecutePendingCallbacks internal, etc) but otherwise I merged in it's entirety.

kpreisser commented 5 years ago

Hi @paulbartrum,

made ExecutePendingCallbacks internal

note that this method is currently only called from GlobalMethodGenerator.Execute() and EvalMethodGenerator.Execute(). However, e.g. if the script has registered some callbacks (FunctionInstance) which I want to call later (after Execute() returned), I would call FunctionInstance.CallLateBound(), but that does not execute the pending callbacks which the script registered during execution of that function.

Therefore, I would need to call ExecutePendingCallbacks() manually after calling the FunctionInstance, so that method would need to be public.

(The other option I could imagine would be do do this directly in the FunctionInstance, but as you mentioned in #54, this is called a lot and could decrease performance.)

What do you think?

Thanks!

paulbartrum commented 5 years ago

note that this method is currently only called from GlobalMethodGenerator.Execute() and EvalMethodGenerator.Execute(). However, e.g. if the script has registered some callbacks (FunctionInstance) which I want to call later (after Execute() returned), I would call FunctionInstance.CallLateBound(), but that does not execute the pending callbacks which the script registered during execution of that function.

Not sure I understand this. If ScriptEngine.Execute() has been called then the pending callbacks list is empty. If you need to call a function and also execute all pending callbacks, then just use Execute or Compile.

paulbartrum commented 5 years ago

I made AddPendingCallback internal for now. I would rather people use the (stable, documented) promise APIs rather than interface with the engine directly.

kpreisser commented 5 years ago

Hi @paulbartrum,

Not sure I understand this. If ScriptEngine.Execute() has been called then the pending callbacks list is empty. If you need to call a function and also execute all pending callbacks, then just use Execute or Compile.

Sorry, I'm not sure if I follow here. How would I call a FunctionInstance using ScriptEngine.Execute() or Evaluate()?

For example, let's assume I want to write a script host providing a global setTimeout() function, where a script can register a callback to be executed later (and when it is called, some value is passed as argument to that callback).

I can implement that function in the host as follows:

    var engine = new ScriptEngine();
    engine.ForceStrictMode = true;
    engine.SetGlobalValue("console", new FirebugConsole(engine));

    object scriptSyncRoot = new object();

    int someValue = 0;
    engine.SetGlobalFunction("setTimeout", new Action<FunctionInstance, int>((handler, timeout) =>
    {
        // Run a new task to ensure the part after the "await" is never
        // executed recursively from this setTimeout method.
        Task.Run(async () => {
            await Task.Delay(timeout);

            lock (scriptSyncRoot) {
                someValue++;
                handler.CallLateBound(engine.Global, someValue);
                // TODO: Need to call engine.ExecutePendingCallbacks() here...
            }
        });
    }));

(Edit: Changed to use an async function, so that the callback is never called recursively from the setTimeout function, but it is still called directly from the thread that completes the Task.)

Now, assume I want to write a script that uses the setTimeout() to implement a Promise-based delayAsync() method (similar to Task.Delay() in .NET). With async functions (ECMAScript 2017), I could write the JavaScript code like this (which can then be downlevel-compiled to ECMAScript 5 e.g. by TypeScript):

function delayAsync(timeout) {
    return new Promise(resolve => setTimeout(resolve, timeout));
}

(async function () {
    for (let i = 0; i < 10; i++) {
        // Wait 1 second.
        let someValue = await delayAsync(1000);
        // Print the value.
        console.log('Some Value: ' + someValue);
    }
})();

I can then execute the equivalent ES5 code (similar to the one in the following string literal) with Jurassic:

    lock (scriptSyncRoot)
    {
        engine.Execute(@"

function delayAsync(timeout) {
    return new Promise(function (resolve) { 
        setTimeout(resolve, timeout);
    });
}

(function () {
    var i = 0;
    function loop() {
        if (i < 10) {
            // Wait 1 second.
            delayAsync(1000).then(function (someValue) {
                // Print the value.
                console.log('Some Value: ' + someValue);
                i++;

                loop();
            });
        }
    }
    loop();
})();

");
    }

However, when I run that, nothing happens, because the pending callbacks are not executed after the call to the supplied FunctionInstance returns. To fix this, I would need to call engine.ExecutePendingCallbacks() at the place marked by the comment. After that change, you can see that numbers 1 to 10 are printed to the console (with a delay of 1 second).

Is there another way to do this without having to make ExecutePendingCallbacks() public? Thank you!

paulbartrum commented 5 years ago

This should work: engine.Execute("") :-)

paulbartrum commented 5 years ago

Your event loop code could look something like this:

var engine = new ScriptEngine();
engine.ForceStrictMode = true;
engine.SetGlobalValue("console", new FirebugConsole(engine));

int someValue = 0;
engine.SetGlobalFunction("setTimeout", new Action<FunctionInstance, int>((handler, timeout) =>
{
    Task.Delay(timeout).ContinueWith(_ =>
    {
        taskQueue.Add(() =>
        {
            someValue++;
            handler.CallLateBound(engine.Global, someValue);
            engine.Execute("");
        });
    });
}));

taskQueue.Add(() =>
{
    engine.Execute("// script goes here");
});

Task.Run(() =>
{
    foreach (var task in taskQueue.GetConsumingEnumerable())
    {
        task();
    }
});

(This pattern avoids locking but you may prefer using locks instead, up to you.)

paulbartrum commented 5 years ago

I committed a change to make this pattern (Execute("")) more efficient (f6de4691011d738b0e529ac357aad5170f5df4e0).

kpreisser commented 5 years ago

Hi @paulbartrum,

This should work: engine.Execute("") :-)

You're right, that works. It still looks a bit like a work-around (though with your latest change it at least doesn't compile and execute that empty code any more, so I think I can use it).

Edit: I noticed that you protected execution of ExecutePendingCallbacks() to do nothing if it is already being called. However, there is still a chance that e.g. when I call a FunctionInstance directly, that one might call another CLR method which does Execute("") to execute pending callbacks, even if the outer FunctionInstance is still running.

Maybe there could be some additional Execute method on the ScriptEngine that can wrap a call to a Function instance, e.g. Execute(FunctionInstance function, object thisObj, params object[] arguments). This method could initially set a boolean variable, and then later call ExecutePendingCallbacks only if that no outer Execute() is currently running.

Then, instead of

    handler.CallLateBound(engine.Global, someValue);
    engine.Execute("");

I would do

    engine.Execute(handler, engine.Global, someValue);

Thanks!

paulbartrum commented 5 years ago

This is not really general enough unfortunately. There's plenty of ways to execute code other than by calling a function directly (for example, you could call ArrayInstance.Every with a callback).

I could do something like:

engine.Execute(() => handler.CallLateBound(engine.Global, someValue))

But I think what we have is fine for now.

By the way, the reason why I'm trying to avoid exposing functionality related to pending callbacks list is because I think there's a reasonable chance I will have to build full event loop support at some point, and anything I build now is likely not going to work in the general case. For details on how pending callbacks (officially called a "microtask queue") are related to event loops, see here: https://javascript.info/microtask-queue

jcdickinson commented 5 years ago

You could make the script parameter for Execute optional (or add an overload) so it becomes the slightly less hacky looking: engine.Execute().