C++ implementation of promise

[rolftimmermans]

I noticed #126 was open and decided to implement it because I needed it for my experiments in converting an existing project to N-API. Especially an AsyncWorker-like class that I'd like to open a separate PR for once this is accepted. (Implementation is already done)

I don't know what the policy is for syncing node_api.h/node_api.cc etc, but they are also updated to a more recent version.

The API allows you to create a Promise::Resolver Promise::Deferred that you can use to retrieve the associated Promise; as well as resolve/reject that promise.

[jasongin]

LGTM.

@gabrielschulhof Can you take a look at this, since you implemented the C APIs for promises?

For updating node_api.h / node_api.cc, the process has been ad-hoc so far. We just have to be careful how we manage breaking changes, so that this library doesn't depend on changes that are not yet in a released version of node. But in this update the changes are only additive, so that's less of a concern.

[jasongin]

I like the idea of the AsyncResolver class too... however I'm wondering if it might make sense to merge it with AsyncWorker. As in, a single class that can work with either promises or callbacks depending on how you use it. I think that might be less confusing than two classes that do almost the same thing. Or maybe not, what do you think?

[rolftimmermans]

I'm wondering if it might make sense to merge it with AsyncWorker

Depends on what the goal is? To share code we can also just extract common code into a common superclass. Especially if we want minimal overhead. Of course a single implementation that can use both promises and callbacks would make the scenario where a user wants to use both possible.

Personally I feel that promises are the future and a class that supports it with minimal overhead is worth separating from callbacks. But I imagine there are many arguments that can be made... :)

[rolftimmermans]

It would be nice if this could get merged so we can continue with adding higher level abstractions using promises.

[mhdawson]

Would like @gabrielschulhof input. @gabrielschulhof do you think you'll be able to get to this soon ?

[corymickelson]

@mhdawson Do you have any idea when this will be added?

[mhdawson]

@corymickelson will try to talk to @gabrielschulhof in the weekly meeting this Thursday to see when/if he is going to be able to review.

[JamesMGreene]

@mhdawson @gabrielschulhof: Any updated ETA? I would really like to see this (or something similar) land.

[addaleax]

I’d propose just landing this in the next few days if there’s no further discussion here

[JamesMGreene]

Would you expect that any use of these V8 Promises results in asynchronous fulfillment on the JavaScript side? Because that is what I am seeing even when my C++ code is synchronous.

Don't get me wrong, in my opinion that is a good behavior for Promises... I was just surprised by it.

I'm assuming that is because when creating/resolving a V8 Promise object, it is automatically scheduled to be fulfilled with the event loop on the next tick (or similar end-of-this-iteration/start-of-next-iteration concept beyond the "tick" time slice) rather than allowing for immediate fulfillment. Sound about right?

[JamesMGreene]

P.S. Specific synchronous code in question:
https://github.com/JamesMGreene/napi-async-promise-example/blob/bfaad4fb9df5053b2b3f00819153850e00272085/addon.cc#L4-L40

[JamesMGreene]

Reading up a bit more, I see that Node handles Promise fulfillment with a "promises microtask queue" (immediately following the "nextTick microtask queue"). 👍

As such, with any Node-native Promise implementation whether in C++ or JavaScript, it would be impossible to react to the fulfillment of any Promise within the same synchronous flow as its creation/fulfillment.

Edified. 🎓

[mhdawson]

@gabrielschulhof agreed to review soon in our last Thrusday call

[mhdawson]

LGTM

[mhdawson]

I'll look to land this week, just going to give @gabrielschulhof a couple more days to comment, otherwise will go ahead and land.

[gabrielschulhof]

Using common.mustNotCall() and common.mustCall()` from Node might be a better way of asserting correct asynchronous behaviour.

[rolftimmermans]

By using, do you mean copying the functions to this project? Because it's not exported by Node, is it?

[gabrielschulhof]

Yeah, we'd pretty much have to copy them. TBH, we should be copying the whole test/addons-napi directory, because we are diverging somewhat from the implementation present in Node.js, so we need to make sure that those tests pass too. Of course that's waaay beyond the scope of this PR, but that one piece of functionality (mustCall() and mustNotCall()) would come in handy already.

[mhdawson]

I'd suggest that copying those might make sense, I don't think we necessarily need to delay this PR for that unless @rolftimmermans believes its relatively easy to do.

[rolftimmermans]

Sure, I added the test helpers that are useful for this test case. I imagine it only makes sense to add others once they're needed.

[gabrielschulhof]

I don't feel too strongly about this, but what about s/Resolver/Deferred/g? The only reason I mention it is that this makes things look too much like V8. IMO using "Deferred" distances us from the engine.That's also why I used "deferred" in the C API.

[rolftimmermans]

That makes sense. I did copy Resolver from v8.

[mhdawson]

@rolftimmermans thanks for your patience looking very close.

[mhdawson]

@gabrielschulhof I think your comments have been addressed except the suggestion to pull in some of the test framework. I'd like to land this tomorrow unless you object.

[mhdawson]

landing

[mhdawson]

I'm getting merge conflicts which I think are due to interveening changes as github reports that there are no conflicts. @rolftimmermans could you squash down to 1 commit ?

[rolftimmermans]

Sure, squashed & pushed!

[mhdawson]

@rolftimmermans thanks ! You always respond so quickly :)

[mhdawson]

Landes as 8ffea15. Thanks for taking the time to do this.

[JamesMGreene]

@rolftimmermans: Were you going to create a PR with your AsyncResolver implementation as well?

[rolftimmermans]

@JamesMGreene Yes, but while working on it in my project I found some critical (for me) usability issues with the AsyncWorker class and my AsyncResolver derivative:

1. Any error is must be a string which is converted to a JS Error. There is no possibility to use a different error class or set custom properties on the error object.
2. All different usages require a subclass, which is kind of "heavy". For a pure promise-based API it might be nice to have something that is more lightweight in terms of source code footprint.

Problem 1 can be solved a (breaking?) API change for AsyncWorker/AsyncResolver.

Problem 2 is more tricky. The way I solved this now is to create a different implementation that takes two lambdas and converts them to std::function; however there is some theoretical overhead associated with that if I'm not mistaken.

Does anyone have any thoughts on this? Ideally AsyncWorker and AsyncResolver (or any alternative) would work with a similar API of course.

[ebickle]

@rolftimmermans Just stumbled across your post regarding the usability of AsyncWorker and the AsyncResolver derivative; I ran into the exact same issues and had been working on a similar implementation in parallel.

Came to the same conclusions - the only way to properly fix the issues would be a breaking change to AsyncWorker. Unless I'm missing something, AsyncWorker didn't even allow passing a return value back out to the callback function.

My code is plugging into some custom encryption logic that returns custom Error objects with additional metadata on failures; the current design "string as error" design prevents me from doing that.

Currently thinking something along these lines:

1. Breaking change to AsyncWorker; its current design is a bit dead in the water without any ability to return a value to the callback.
2. Option A: change AsyncWorker to an abstract base class and remove the callback-specific portions of it (receiver and callback). Create an AsyncCallbackWorker subclass to replace the current AsyncWorker and add the ability to return a value, additionally create an AsyncPromiseWorker subclass to bridge to a Promise/Deferred.
3. Option B: Modify AsyncWorker to support both callback and promise use-cases within the same object.
4. Consider changing Queue() and Cancel() to be static functions or otherwise improve them; right now we have a situation where two parties are responsible for the memory management of the same object - the caller instantiates the memory, the object self-destructs later on. It's a bit out of place and doesn't fit within normal C++ memory management patterns. We might want to make them static functions, so at least we don't have the scenario of a method (eventually) triggering the deletion of itself.
5. Add support for Error objects instead of the string. I'm not sure if there are any cross-thread implications, but I suspect that's where the new async scope functionality comes into play?

Rolf, let me know if you're interested in teaming up on any of this and pitching a new async worker API pattern to the node-addon-api group.

[rolftimmermans]

Sounds good, I'll have a look to see if I can distill what I'm currently using in the C++ code.

Larger APIs may have many different async tasks so I'd be in favour of something that doesn't require defining classes for every single scenario; something that accepts lambdas would be nice.

[rolftimmermans]

So what I have now is this:

class Work {
    /* Simple unique pointer suffices, since uv_work_t does not require
       calling uv_close() on completion. */
    std::unique_ptr<uv_work_t> work{new uv_work_t};

    std::function<void()> execute_callback;
    std::function<void()> complete_callback;

public:
    template <typename... Args>
    static inline uint32_t Queue(Args&&... args) {
        auto work = new Work(std::forward<Args>(args)...);
        return work->Exec();
    }

    inline Work(std::function<void()>&& execute, std::function<void()>&& complete)
        : execute_callback(std::move(execute)),
          complete_callback(std::move(complete)) {
        work->data = this;
    }

    inline uint32_t Exec() {
        auto err = uv_queue_work(uv_default_loop(), work.get(),
            [](uv_work_t* req) {
                auto& work = *reinterpret_cast<Work*>(req->data);
                work.execute_callback();
            },
            [](uv_work_t* req, int status) {
                auto& work = *reinterpret_cast<Work*>(req->data);
                work.complete_callback();
                delete &work;
            });

        if (err != 0) delete this;

        return err;
    }
};

It allows you to do this:

struct MyContext {
    // other data
    uint32_t error = 0;
};

Napi::Value MyClass::MyFn(const Napi::CallbackInfo& info) {
    auto res = Napi::Promise::Deferred::New(Env());
    auto ctx = std::make_shared<MyContext>(...);

    Work::Queue(
        [=]() {
            // background tasks
            // optionally set ctx->error = ...
        },
        [=]() {
            // cleanup
            CallbackScope scope(Env());

            if (ctx->error != 0) {
                res.Reject(ErrnoException(Env(), ctx->error).Value());
                return;
            }

            res.Resolve(Env().Undefined());
        }
    );

    return res.Promise();
}

Advantages I found this approach has:

1. All logic can be 100% inline in the method definitions by virtue of lambdas.
2. Making no assumptions about error scenarios allows very flexible error object extensions.

Disadvantages I have found so far:

1. The std::shared_ptr is required to keep context around between the "background" lambda and the "cleanup" lambda. Otherwise any stack variables will be released by the time the function returns (while the background task is still in progress) which causes a segfault.
2. Lambdas must capture by value.

Perhaps these issues can be solved, I'm not sure yet.

[ebickle]

I just created #231 so that we can discuss async worker improvements in a single spot; feel free to toss ideas there if you think it will be useful.

Comments and thoughts:

• Should we use std::invoke()? It's a relatively new C++ standard library feature (C++17) but it would allow the execute and complete callbacks to use any Callable and function in a similar manner to std::async(). I wasn't sure whether it was the right path to go down compared to std::function or more traditional options.
• I wasn't sure about using std::shared_ptr myself. I'm not sure whether Node.js uses the standard library smart pointers anywhere, but so far node-addon-api doesn't appear to expose them anywhere in its API surface area. Object lifetime management across threads is a bit of a nightmare.

The biggest constraint I have at the moment is the need for two callbacks/lambdas - one for the execution, and another for the completion. The completion runs in the main Node.js event loop, so it has access to the full scope of Javascript types and operations that can be executed.

Looking at #223, I wonder if something similar could be used to return Error objects and return types directly out of the Execute callback somehow.

Regardless of whether or not that's possible, I think we can wrap the "complete callback" with some magic that 1) accepts a napi_value as from the complete function, 2) maps thrown exceptions to Javascript Errors 3) recognizes when Javascript Error is "thrown" with C++ exceptions turned off.

The idea would be the "complete callback wrapper" would then somehow bridge the return value and/or Error to a Promise or Callback.

[rolftimmermans]

Having a second callback/lambda is required to create a custom error because a v8/n-api error object cannot be created in a background thread. I'm fine with having a default implementation that does the sane thing 90% of the time, but to overcome the custom error issue it needs to be possible to override it.

I will look at std::invoke to see if I can apply it in my current implementation!

[AlexandreBossard]

Hi,
I tried to resolve a promise inside the completion handler with a code that is pretty much the same as show above by @rolftimmermans, but with not luck. Everything compiles and runs, but my promise was not resolved when in call Deferred::Resolve().

Then I created an AsyncContext and a CallbackScope after the first HandleScope inside the handler and everything seems to be fine. What i am missing ? Is this the correct way to do it ?

[JamesMGreene]

@AlexandreBossard I'd recommend opening a new issue if you are experiencing problems. Also, please include more of your actual code for context when you create it. Thanks!