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

[JSRT] Promise API enhancements #2541

Closed
jaredwy opened this issue Feb 16, 2017 · 28 comments
Closed

[JSRT] Promise API enhancements #2541

jaredwy opened this issue Feb 16, 2017 · 28 comments

Comments

@jaredwy
Copy link

jaredwy commented Feb 16, 2017

I apologies if this is the wrong place to ask this question. I couldn't find a more appropriate place.

I am looking to create a function from C++ that will return a promise to the callee in JS. Then be able to resolve/reject it when a async task has been completed natively. Something similar to v8::Promise::Resolver

I can't see anything in the docs, nor the source. Is there a way to do such a think in ChakraCore at the moment?

@liminzhu
Copy link
Collaborator

I don't believe we have it in JSRT now. These's an active discussion about this on Gitter.

@galvesribeiro @ReubenBond @boingoing @dilijev maybe we should continue in this issue?

@jaredwy
Copy link
Author

jaredwy commented Feb 16, 2017

Great thanks. I can ask my follow up question in gitter.

@galvesribeiro
Copy link

For the record, that is the API we are reffering to https://v8docs.nodesource.com/node-7.4/d3/d8f/classv8_1_1_promise.html

@liminzhu
Copy link
Collaborator

liminzhu commented Feb 16, 2017

Let me post a short summary for the conversation with Orleans folks (@galvesribeiro @ReubenBond) and @jaredwy on Gitter from here to here and let's discuss further matters here to preserve context. And folks, feel free to suggest edits if I miss anything.

There're generally a few asks:

  1. Ability to create a promise from the native side. Something to the effect of JsCreatePromise(JsValueRef resolve, JsValueRef reject, JsValueRef *promise) which mirrors new Promise(resolveFunc, rejectFunc) in JS. This is already do-able in JSRT by running a small script which returns a Promise, but the code gets really messy if handlers are native function. For what it's worth, V8 has similar APIs.
  2. Similar to 1), ability to resolve/reject from native.
  3. Task/Promise interop between C# and JavaScript. Essentially, support for this pattern (copying stuff from @ReubenBond),

We provide some asynchronous methods to JS code

public interface IDatabase
{
  Task<object> GetValue();
  Task SetValue(object value); 
}

The JS code uses it like this

function jsFunc(db) {
  var value = await db.GetValue();
  value.count++;
  await db.SetValue(value);
  return value.count;
}

So it needs to be executed like this (C# pseudo code):

var count = await RunChakraScript(script);

Looping in all interested parties and a few ChakraCore gurus, @galvesribeiro @ReubenBond @jaredwy @boingoing @dilijev @curtisman @bterlson @kfarnung

@liminzhu liminzhu changed the title Question: Creating and resolving promises from JSRT api? [JSRT] Promise API enhancements Feb 16, 2017
@jaredwy
Copy link
Author

jaredwy commented Feb 17, 2017

Hi all. Going to attempt 1 and 2 from at least a c++ side this weekend. See how it feels and make some recommendations for any changes based on that.

Been a while since I've written any c# so may need a hand with 3 but I will see what I can do.

@ReubenBond
Copy link

ReubenBond commented Feb 17, 2017

Thanks, @liminzhu! We aren't asking for C# interop specifically, just a path for us to create better interop ourselves.
Promise & Task/TaskCompletionSource are a natural fit, even if they're inverted a little.
They approximately map as follows:
In JavaScript:

var task = new Promise(taskCompletionSource => { ... });

In C#, the construction is the like so:

var promise = new TaskCompletionSource<T>(); // can be resolved/rejected
var task = taskCompletionSource.Task; // the future which can be read / awaited / continued from

Of course, C# and JS also have their own alternate syntaxes for these things using async/await, but those are the primitives at the core level.

So our interop would be like so:

When creating a Promise from C#, we would create an object which receives a Task/Task<T> and signals JS when that task resolves/rejects.

When receiving a Promise from JS, we would turn that into a Task/Task<T> which internally uses a TaskContinuationSource<T> to resolve/reject when the JS Promise resolves/rejects

@kfarnung
Copy link
Contributor

kfarnung commented Feb 17, 2017

@jaredwy I have a prototype I put together that should make a good starting point.

kfarnung@jsrtpromise

Basically it adds two functions:

  • JsCreatePromise - Creates a new promise object that can be resolved/rejected in native code
  • JsCreateThenPromise - Takes a source promise (from either native or script) and exposes fulfilled/rejected handlers

I also have a hacked up sample that attempts to test it out:

kfarnung/Chakra-Samples@jsrtpromise

The sample has examples of both:

  • Creating a promise in native and continuing it in script
  • Creating a promise in script and continuing it in native

@jaredwy
Copy link
Author

jaredwy commented Feb 17, 2017 via email

@dilijev
Copy link
Contributor

dilijev commented Feb 17, 2017

@kfarnung I'm not sure I understand the naming of JsCreateThenPromise -- could you explain a bit more?

@ReubenBond
Copy link

I think JsCreateContinuation would be a more suitable name, taking an antecedentPromise and returning a continuationPromise (via an output param).

To keep naming consistent, the fulfillmentHandler could have resolve in its name somewhere. They could be called onResolve and onReject, but I'm not sure what's consistent with this codebase.

@kfarnung
Copy link
Contributor

@dilijev @ReubenBond I just mirrored the internal naming for now to see if this concept would work for people. I fully intend to update the names of the methods and parameters if this were to become a real pull request, for now it's just a prototype.

For reference the internal method signature is:
static Var CreateThenPromise(JavascriptPromise* sourcePromise, RecyclableObject* fulfillmentHandler, RecyclableObject* rejectionHandler, ScriptContext* scriptContext);

@kfarnung
Copy link
Contributor

@jaredwy Only naming and documentation really, assuming this unblocks the scenario.

@liminzhu Assuming this solves the issue are there any blockers to adding these two APIs (with naming cleanup) to the API surface?

@liminzhu
Copy link
Collaborator

liminzhu commented Feb 17, 2017

Thanks for the clarification @ReubenBond!

Adding my take on all 3 asks based on my perceived JSRT design principle (providing a small interface that has the basic building blocks for more complex use cases),

  1. JsCreatePromise - this can be done with current APIs by running a script that returns a Promise. But given the code for attaching native handlers is long and messy, I can see that having an extra API makes sense. Ability to add .then/.catch to the chain is similar.
  2. Resolve/reject - the you-can-already-do-this argument gets stronger with a simple JS script. There's also .race and .all for completeness, but they're definitely not as much used.
  3. Task and promise interop,

When creating a Promise from C#, we would create an object which receives a Task/Task<T> and signals JS when that task resolves/rejects.

I would say this is already doable. The basic idea is to create a Task in C# for the task continuation received in a JsPromiseContinuationCallback.

When receiving a Promise from JS, we would turn that into a Task/Task<T> which internally uses a TaskContinuationSource<T> to resolve/reject when the JS Promise resolves/rejects

You can implement the host in a way s.t. the host signals to the Task when the message pump gets empty after script execution (a.k.a. no more tasks stored in the queue from JsContinuationCallback, a.k.a. all promises are resolved).

@jaredwy
Copy link
Author

jaredwy commented Feb 17, 2017

@kfarnung I will try it out and see how it feels for my needs. I can turn into a c++ sample.

@liminzhu
Copy link
Collaborator

@kfarnung I don't see any big issue (assuming sign-off from a few other Chakra folks and adding proper tests ofc 😄), but I'd like to hear from the folks asking for this whether this fits their needs.

@liminzhu
Copy link
Collaborator

Btw any additional APIs we implement in ChakraCore for promises, I probably would want to ship them to Chakra.dll on Windows as well. So until I get sign-off from Windows folks, they will remain experimental in ChakraCore and may have breaking changes depending on the feedback I get.

@ReubenBond
Copy link

ReubenBond commented Feb 17, 2017

@liminzhu efficiency is important to us - particularly when it comes to GC pressure and marshalling, so any reductions in allocations and managed<->native<->JS transitions are valuable.

That being said, if you think the suggested solution is workable today, then perhaps it's sufficient. We'd have to make another attempt at adding support using that method to see if it's suitable.

@dilijev
Copy link
Contributor

dilijev commented Feb 17, 2017

@liminzhu I think it makes sense to mark these APIs as experimental for now so that we can also react to community feedback about these APIs. It goes both ways :)

@liminzhu
Copy link
Collaborator

liminzhu commented Feb 17, 2017

@ReubenBond It probably can work with my mental model. The only problem I see is that a runtime can only be on one thread at a time, which creates some complication for an implementation that may fetch promise continuations to different threads (is that how Task works?) and have to move runtime around. You need to implement a message pump and a host that's more complicated than what I had in Chakra-samples to get more asynchrony. Basically any attempt to mess with the single-thread nature of JS may get ugly.

I don't think you'll be keeping objects alive for more than you should though I can see your point about marshalling if you need to create native promises all the time. I see the promise problem as more of a confusion and implementation nicety issue, but again, a host has to implement its own message pump because this is outside the scope of the JavaScript VM.

@ReubenBond
Copy link

@liminzhu

The only problem I see is that a runtime can only be on one thread at a time, which creates some complication for an implementation that may fetch promise continuations to different threads (is that how Task works?) and have to move runtime around

Our intention is to execute this JS code from within actors in Orleans and each actor is effectively single-threaded. We will need to ensure that we fulfil promises on the OS thread which they were started on (is that right? I assume there's no way around it). We don't intend to mess with the single-threaded nature of JS, in fact it fits very well with our model.

@liminzhu
Copy link
Collaborator

liminzhu commented Feb 17, 2017

Got it (that's a relief 😄).

Is part of the problem that you guys want to be able to await on JS execution (that's a generalization of resolving a promise), but with promises, JS execution doesn't necessarily finish when JsRunScript returns? That should be solved with my suggestion.

Exposing APIs to handle promises natively is a separate issue (in fact, even if we expose a JsResolvePromise function, you will still need to do the same magic in the host as long as host owns the message pumping), which we can discuss further.

@jaredwy
Copy link
Author

jaredwy commented Feb 20, 2017

@liminzhu this is indeed part of my problem.

I have some work on a second thread that i need to then let JS know has finished. Your suggestion will work but it is messy and non obvious to implement.
But this api certainly helps make it a little cleaner so a great first step.

@jaredwy
Copy link
Author

jaredwy commented Feb 21, 2017

The JsCreatePromise seems to suit my immediate needs creating and resolving promises from C++. Much cleaner! I need to change things up with how I handle passing between threads to enable resolving on the thread that chakra is on (i am hopeful i can just borrow the idea from the sample in posting a new task to the queue to resolve it with the data), i will have some more time tomorrow but as a first pass this looks to solve my needs.

Haven't found a use for CreateThenPromise yet :D

@jaredwy
Copy link
Author

jaredwy commented Feb 22, 2017

Using @kfarnung code.

JsValueRef DoSomethingPromisey(JsValueRef callee, bool isConstructCall, JsValueRef *arguments, unsigned short argumentCount, void *callbackState) 
{
    JsValueRef resolve;
    JsValueRef reject;
    JsValueRef promise;
    JsValueRef ret;
    std::string result("delayed hello world");
    JsCreateString(result.c_str(), result.length(), &ret);
    
    JsCreatePromise(&promise, &resolve, &reject);
    std::thread fakeWork([&]()  {
        std::this_thread::sleep_for(std::chrono::milliseconds(1000));
        {
            std::lock_guard<std::mutex> lock(queueMutex);
            taskQueue.push(new Task(resolve, 0, promise, ret));
        }
        cond.notify_one();
    });
    fakeWork.join();
    return promise;
}

Calling code looks like "var a = application.doSomethingPromisey(); (()=>{return a.then((b)=>make{application.log(b);});})()"

It does mean that i need to carry around the resolve/reject functions but that isn't too bad. I can typedef a std::pair for that. It certainly feels like it fits with the rest of the API.

I have no complaints. Marking it as experimental sounds like a great idea!

I can also probably turn this into a sample file if i clean up the code and remove that fakeWork.join() would people find that valuable?

@kfarnung
Copy link
Contributor

Thanks @jaredwy! We'll discuss a timeline on our end and report back.

A sample would be great! Once we have the feature submitted we can add it to the samples repository.

@kfarnung
Copy link
Contributor

There is a PR out now. I removed JsCreateThenPromise since you can just query for the "then" property and continue it that way.

JavaScriptPropertyId thenId = JavaScriptPropertyId.FromString("then");
JavaScriptValue thenFunction = promise.GetProperty(thenId);

JavaScriptValue resolveFunc = JavaScriptValue.CreateFunction(ResolveCallback);
JavaScriptValue rejectFunc = JavaScriptValue.CreateFunction(RejectCallback);
JavaScriptValue thenPromise = thenFunction.CallFunction(promise, resolveFunc, rejectFunc);

@kfarnung
Copy link
Contributor

The PR was committed to release/2.0 as 96cb103.

@kfarnung
Copy link
Contributor

kfarnung commented Mar 3, 2017

Resolved by #2594 and slated to be part of the 2.0 release.

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

No branches or pull requests

6 participants