Node-addon-api: async_work executor signalling

[skibz]

using the c api, how would one signal to the async work executor that an invocation of a napi_async_execute_callback has failed? so that when the corresponding napi_async_complete_callback is invoked, it has a napi_status value that indicates a failure?

i am a total c(++) greenhorn, so i'm sorry if this question is inane or a waste of your (collective) time. please let me know if this is an inappropriate place to field this kind of question. i apologise if that is the case.

could the solution be to use (parts of) the c++ api, instead?

the addon i have been developing uses the void* data parameter to pass around a pointer to a struct that i could modify to include a value indicating success or failure to be checked when napi_async_complete_callback is invoked. however, if there is already a way to do this, i'd prefer to use that approach instead.

[jasongin]

When using the C APIs, the void* data parameter is the way to do it, as you described. The napi_status parameter to the napi_async_complete_callback is not intended to be an indication of whether the execution callback was successful, merely whether the execution was scheduled and ran (or was cancelled).

However, I'd recommend looking at the AsyncWorker class in the C++ API instead. Async callbacks can be tricky to get correct, and the higher level of abstraction provided by the AsyncWorker class can simplify things a lot. For example it has a SetError() method that's meant to be called during execution, which causes a different completion callback to run: OnError() instead of OnOK(); the default implementation of OnError() invokes the JS callback function with an Error value.

Unfortunately the documentation for that class isn't written yet. (Would anyone like to help?)

[skibz]

thanks @jasongin !
i'll have a crack at AsyncWorker 💃

[koistya]

@jasongin How to pass "data" back to the caller from under the Execute() method?

[jasongin]

If you have some result data to pass back from Execute(), I'd suggest saving it to a member variable of your AsyncWorker subclass. Then in your OnOK() override you can pass that result data back to a JavaScript function.

I suppose we could add to the library something like an AsyncWorker<TResult> class, that has a TResult Result() and maybe a SetResult(TResult result) method. But I don't know if it would very helpful since you need to subclass AsyncWorker anyway as it is to override the OnOK() method. So it's trivial to then define your own result member(s) when needed.

[krystianity]

Sorry for adding momentum to a closed issue, but I was trying to implement the suggested solution that @jasongin mentioned above. Using non Napi::Object values like Napi::Boolean works fine, but I am trying to call the callback function with an object as second parameter and it results in segfaults and funky promises resolving in error messages :dancer:

Any idea what I (not that strong at C++) am doing wrong here?

// callme.cc
#include "napi.h"

namespace hello {

    class HelloWorker : public Napi::AsyncWorker {

    public:

        HelloWorker(Napi::Function callback) : AsyncWorker(callback){
            _test = Napi::Object::New(Env());
        }

        bool _succeed;

    protected:

        void Execute() override {
            if(!_succeed){
                SetError("Hello World Error");
            } else {
                //segfault here
               _test.Set("test", Napi::Boolean::New(Env(), true));
            }
        }

    private:

        Napi::Object _test; 

        void OnOK() override {
            Callback().MakeCallback(Receiver().Value(), std::initializer_list<napi_value>{
               Napi::Value(), //segfault
               _test //promise with: Invalid pointer passed as argument
            });
        }
    };

    void RunCallback(const Napi::CallbackInfo& info){

        bool succeed = info[0].As<Napi::Boolean>();
        Napi::Value data = info[1];
        Napi::Function callback = info[2].As<Napi::Function>();

        HelloWorker* worker = new HelloWorker(callback);
        worker->Receiver().Set("data", data);
        worker->_succeed = succeed;
        worker->Queue();
    }

    Napi::Function InitCallMe(Napi::Env env){
        Napi::Function runcallback_function = Napi::Function::New(env, RunCallback);
        return runcallback_function;
    }
} //namespace hello

[jasongin]

The _test value is invalid by the time OnOK() runs because it got collected by the GC. To prevent that, you should store it as a Napi::ObjectReference:

private:
  Napi::ObjectReference _test;
...
_test = Napi::Persistent(Napi::Object::New(Env()));

Or you can use the Receiver() ObjectReference that is already attached to the AsyncWorker instance, and set/get properties on it:

Receiver().Set("test", Napi::Object::New(Env());

[krystianity]

Thanks for the super fast reply @jasongin , passing the object as callback param works now.
But I still cannot do any actions inside of Execute()

Receiver().Get("any"); is a segfault already :(

// callme.cc
#include "napi.h"

namespace hello {

    const char* RESULT_KEY = "result";

    //AsyncWorker is an abstract helper class for thread-pooled tasks
    class HelloWorker : public Napi::AsyncWorker {

    public:

        HelloWorker(Napi::Function callback) : AsyncWorker(callback){
            Receiver().Set(RESULT_KEY, Napi::Object::New(Env()));
        }

        bool _succeed;

    protected:

        //called when first in queue and dispatched in thread
        void Execute() override {

            if(!_succeed){
                SetError("Hello World Error");
                return;
            }

            Receiver().Get("any"); //segfault

            /*
            Napi::Value value = Receiver().Get(RESULT_KEY);
            Napi::Object result = value.As<Napi::Object>();
            result.Set("test", Napi::Boolean::New(Env(), true)); */
        }

    private:

        //called when Execute() does not call SetError()
        void OnOK() override {
            Callback().MakeCallback(Receiver().Value(), std::initializer_list<napi_value>{
                Env().Null(),
                Receiver().Get(RESULT_KEY)
            });
        }
    };

    void RunCallback(const Napi::CallbackInfo& info){

        bool succeed = info[0].As<Napi::Boolean>();
        Napi::Value data = info[1];
        Napi::Function callback = info[2].As<Napi::Function>();

        HelloWorker* worker = new HelloWorker(callback);
        worker->Receiver().Set("data", data);
        worker->_succeed = succeed;
        worker->Queue();
    }

    Napi::Function InitCallMe(Napi::Env env){
        Napi::Function runcallback_function = Napi::Function::New(env, RunCallback);
        return runcallback_function;
    }
} //namespace hello

[jasongin]

Oh, yes I forgot to mention an important detail. The Execute() method does not run in a JavaScript context, so you can't access any JS types from there. Instead you should save any results as (non-JS) member variables, and convert the results to JS values in the OkOK() callback.

[krystianity]

@jasongin thanks for the hint :100: got it now :)

[ptgolden]

If JavaScript types can only be accessed in the OnOK callback, that means that AsyncWorker cannot handle progressively calling a JS function, correct? For example, calling an onData callback each time a new piece of information is available.

(I'm trying to write an addon that produces (or interfaces with) a Node readable stream).

[gorshkov-leonid]

@jasongin could you reply to @ptgolden's question? I can find neither node-addon-api or n-api functions to implement streaming worker such as Nan::AsyncProgressWorker

There is example of usage from a book: [factorization example | https://github.com/freezer333/nodecpp-demo/blob/master/streaming/examples/factorization/factorization.js].

In my case I need to hook some system events and pass them to js callback.

[mhdawson]

@gorshkov-leonid can you open a new issue. This one is closed and its probably better to discuss in a new issue.

[MD-AZMAL]

The _test value is invalid by the time OnOK() runs because it got collected by the GC. To prevent that, you should store it as a Napi::ObjectReference:

private:
  Napi::ObjectReference _test;
...
_test = Napi::Persistent(Napi::Object::New(Env()));

Or you can use the Receiver() ObjectReference that is already attached to the AsyncWorker instance, and set/get properties on it:

Receiver().Set("test", Napi::Object::New(Env());

I am trying something similar but it gives me an issue saying .Set is not a function