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

Passing char pointer #501

Closed
adnan-kamili opened this issue Jun 20, 2019 · 17 comments
Closed

Passing char pointer #501

adnan-kamili opened this issue Jun 20, 2019 · 17 comments

Comments

@adnan-kamili
Copy link

I have a simple C function with the following signature:

int GetMetadata(const char* key, char* value, size_t length);

I want to wrap this function using napi. I tried the following:

Napi::Value getMetadata(const Napi::CallbackInfo &info)
{
    Napi::Env env = info.Env();
    // some validations...
    string arg0 = info[0].As<Napi::String>();
    uint32_t arg2 = info[2].As<Napi::Number>().Uint32Value;
    char metadataValue[256] = "\0";
    Napi::Number status = Napi::Number::New(env, GetMetadata(arg0.c_str(), metadataValue, arg2));
    // how to return the value of metadataValue to calling function
    return status;
}

From Javascript, I would be passing in a buffer of length 256. How do I assign metadataValue to buffer passed in through JS?

const buffer = Buffer.alloc(256);
MyAddon.GetMetadata("someKey", buffer);
@sziraqui
Copy link

You can use Uint8Array instead of Buffer.
JS:

let arr = new Uint8Array(256); // filled with zeros
//.. edit the arr here if needed
const result = MyAddon.GetMetadata("someKey", arr);

Cpp:

Napi::Value getMetadata(const Napi::CallbackInfo &info)
{
    Napi::Env env = info.Env();
    // some validations...
    string arg0 = info[0].As<Napi::String>();
    Napi::Uint8Array arr = info[1].As<Napi::Uint8Array>();
    size_t length = arr.ElementLength(); // 256
    char* metadataValue = reinterpret_cast<char *>(arr.ArrayBuffer().Data()); 
    //.. Do whatever you want
}

Hope this helps.

@adnan-kamili
Copy link
Author

Thanks a lot.

What should we do for *int and *function pointers?
Callaback signature:

typedef void (int *CallbackType)(uint32_t);

Functions:

int GetUses(const char* key, uint32_t* value);
int SetCallback(CallbackType callback);

@sziraqui
Copy link

sziraqui commented Jun 20, 2019

You can use TypedArrayOf
Read this
Some of your doubts will be solved if you read the documentation. Then try to understand the code examples

@adnan-kamili
Copy link
Author

The code examples link you shared points to the previous link. However, I tried the following for uint32_t pointer case and it worked fine:

In JS:

let uses = new Uint32Array(1);
addon.GetUses(uses);
console.log(uses[0]);

In C++:

Napi::Uint32Array array = info[0].As<Napi::Uint32Array>();
uint32_t *arg0 = reinterpret_cast<uint32_t *>(array.ArrayBuffer().Data());

Can you point to any docs for function pointer?

Also, TypedArrays were introduced in node 8, is it possible to use buffer instead so that it works with older versions too.

@adnan-kamili adnan-kamili changed the title Passing buffer reference Passing char pointer Jun 21, 2019
@sziraqui
Copy link

I updated the link. All resources are provided in this repos README. If you need compatibility with older nodejs versions, you will have to use Native Abstractions of Node (NAN) api AFAIK.

@adnan-kamili
Copy link
Author

adnan-kamili commented Jun 21, 2019

The callback example doesn't fit the use case. It is calling the callback function immediately. But I need to call the callback, when the internal c callback has finished (inside c callback). So it seems I need to store some global variables.

Napi::Env env;
Napi::Function callback;
void MyCallback(uint32_t status)
{
     callback.Call(env.Global(), {Napi::Number::New(env, status)});
}

Napi::Value setCallback(const Napi::CallbackInfo &info)
{
    env = info.Env();
    callback = info[0].As<Napi::Function>();
    return Napi::Number::New(env, SetCallback(MyCallback));
}

Is there any way to avoid global variables, and in case user sets many callbacks I will need to store them in a global list.

@mhdawson
Copy link
Member

@adnan-kamili you should probably look at the AsyncWorker functionality.

@adnan-kamili
Copy link
Author

Hi @mhdawson ,

Thanks, I succeeded in implementing my use case, can you please have a look in case I did something wrong:

class CallbackWrapper : public Napi::AsyncWorker
{
public:
    CallbackWrapper(Napi::Function &callback) : Napi::AsyncWorker(callback)
    {
    }
    uint32_t status;

private:
    void Execute()
    {
    }

    void OnOK()
    {
        Napi::HandleScope scope(Env());
        Callback().Call({Napi::Number::New(Env(), status)});
    }
};

I had to keep a map of AsyncWorker because user can register many callbacks in my case:

map<string, CallbackWrapper*> MyCallbacks;

void MyCallback(uint32_t status)
{
    MyCallbacks[Key]->status = status;
    MyCallbacks[Key]->Queue();
}

Napi::Value setMyCallback(const Napi::CallbackInfo &info)
{
    Napi::Env env = info.Env();
    // some validations
    Napi::Function callback = info[0].As<Napi::Function>();
    LicenseCallbacks[Key] = new CallbackWrapper(callback);
    LicenseCallbacks[Key]->SuppressDestruct(); // needed because callback is invoked periodically by the C++ library
    return Napi::Number::New(env, SetCallback(MyCallback));
}

@mhdawson
Copy link
Member

I think one issue is that it looks like you might be re-using the AsyncWorker. From the N-API docs (which is what the class in node-addon-api uses under the covers) you should only call queue once for a given instance. https://nodejs.org/api/n-api.html#n_api_napi_queue_async_work.

I'm also guessing that you must be calling "MyCallback" on a thread other than the main thread. If you have your own threads in addition to the main JS thread then you should look at using https://nodejs.org/api/n-api.html#n_api_asynchronous_thread_safe_function_calls (we are working on landing a wrapper for these in node-addon-api but it has not yet landed - > #442)

@adnan-kamili
Copy link
Author

@mhdawson

Yes, the MyCallback() function is invoked at an interval repeatedly by the C library from a separate thread, so the queue() function is executed multiple times. I tested this on Windows, Mac, and Linux, and it works fine.

I guess it should be fine because I have no code in the execute function.

@mhdawson
Copy link
Member

@adnan-kamili I think you may just be getting "lucky" or "unlucky" depending on how you look at it since its documented that it will not necessarily work. @gabrielschulhof can you fact check me on this?

@gabrielschulhof
Copy link
Contributor

gabrielschulhof commented Jun 28, 2019

@mhdawson yes, AsyncWorker has a very specific usage pattern, which is to create one and queue it. That's it. A reference to it need not be stored at all, because it will self-destruct after it completes. A new AsyncWorker has to be created for each subsequent submission of work.

@adnan-kamili you mentioned that you need to create a binding for a C function that accepts a function pointer with the following signature:

typedef void (int *CallbackType)(uint32_t);
              ^^^

AFAIK having the int there is not valid. Only the name of the type can appear in brackets, prefixed by a *.

So, I'm assuming the API looks like this:

typedef void(*CallbackType)(uint32_t);
void give_me_an_integer_later(CallbackType cb);

it becomes crucially important to know whether cb gets called synchronously from within give_me_an_integer_later(). If it does, you can create a simple binding like this:

uint32_t last_result;
static void TheCallback(uint32_t result) {
  last_result = result;
}
static Napi::Value bind_give_me_an_integer_later(
    const Napi::CallbackInfo& info) {
  give_me_an_integer_later(TheCallback);
  info[0]
      .As<Napi::Function>()
      .Call({Napi::Number::New(info.Env(), last_result});
}

However, if give_me_an_integer_later() returns without calling TheCallback(), but there is a guarantee that TheCallback() will be called at some later time, then things get very complicated, because you have to

  1. save info[0] for later – easily done with a Napi::FunctionReference
  2. Ensure that, when TheCallback() does get called, it, in turn, calls the right FunctionReference().

This latter part requires that the C API guarantee some order in which it calls TheCallback(). For example, if it's first come, first served, then you can store the FunctionReferences in a queue. Otherwise you won't know which Napi::FunctionReference to call in response to which call to TheCallback.

The situation in which TheCallback() gets called in an unpredictable order can only be handled correctly with an FFI library such as https://github.com/node-ffi/node-ffi. You would use the version of libffi that gets packaged with node-ffi to create, at runtime, a function with the required signature (void (*)(uint32_t)) and then you would write a function

static void TheCallback(uint32_t value, Napi::FunctionReference ref) {
}

which this other function would call, passing through the uint32_t it receives, and attaching the Napi::FunctionReference it stored.

Doing FFI is pretty complicated, but the only way to go when the API is not of this form:

void give_me_an_integer_later(void (*callback)(uint32_t value, void* data), void* data); 

give_me_an_integer_later() has to also accept a void* and then give it back via the second parameter when it calls the callback, because that's where you would pass the Napi::FunctionReference. If the API does not have this shape, and there is no guarantee as to the order in which callbacks happen, then FFI is the only way to go.

I have an example of using FFI:

  1. The infrastructure is here: https://github.com/intel/iotivity-node/blob/412a950c6fb0880bd7e1d4f63ff60396ff921ff6/callback-info.c

  2. The usage is here:

    1. https://github.com/intel/iotivity-node/blob/412a950c6fb0880bd7e1d4f63ff60396ff921ff6/functions.cc#L129
    2. https://github.com/intel/iotivity-node/blob/412a950c6fb0880bd7e1d4f63ff60396ff921ff6/functions.cc#L153
    3. The definition of the function pointer type OCEntityHandler is here: https://github.com/iotivity/iotivity/blob/0.9.0/resource/csdk/stack/include/ocstack.h#L365

In the example, OCCreateResource() is the API that doesn't have the void* that we need, and the function pointer type OCEntityHandler does not accept a void*. callback_info_new() accepts a function that has the signature that we want, and produces a function (in info->resultingFunction) with the signature that we need.

@adnan-kamili
Copy link
Author

@gabrielschulhof

Thanks a lot for such a detailed answer. In my case, the callback is not invoked immediately but later by the C library in a separate thread. And this callback is not invoked once but many times at a specific interval.

Initially, we did use node-ffi to create a wrapper, but node-ffi is not supported for arm architecture and Callback functions segfault on Alpine Linux. Hence, we decided to go with N-API, and now it works on all the platforms/architecture we support.

Instead of using AsyncWorker for implementing our use-case, we did try function references by storing a function reference list globally, but it always segfaulted. We could only get it working using AsyncWorker.

@gabrielschulhof
Copy link
Contributor

@adnan-kamili thanks for your follow-up! I'm glad to hear you were able to find a solution!

@mhdawson
Copy link
Member

mhdawson commented Jul 3, 2019

@gabrielschulhof I'm concerned that the use of AsyncWorker is only working by "good or bad" luck and @adnan-kamili should probably be using the threadsafe functions instead.

@gabrielschulhof
Copy link
Contributor

@mhdawson @adnan-kamili I agree that there is no guarantee in the above implementation of AsyncWorker that status will be faithfully conveyed to JavaScript. The reason is that there is a race condition between the thread assigning the status, and the AsyncWorker's Execute() function terminating.

In order to ensure correctness, a semaphore would have to be associated with each AsyncWorker. Execute() would then wait on the semaphore, and TheCallback would signal after having set the status. That would cause Execute() to resume and to eventually return the status to JavaScript.

A solution thus modified would be correct in the sense that no secondary thread ends up making calls into the main thread and there are no race conditions, however, it would be significantly more resource-intensive than it needed to be, because creating so many AsyncWorker instances is expensive and not their intended use case.

Assuming I have gauged the situation correctly, I believe @mhdawson is right in that a thread-safe function would better suit your needs, because a TSFN is designed to convey multiple calls from multiple threads into JavaScript via a marshalling function that runs on the main thread. #442 has just landed, so the TSFN C++ wrapper should be available in the next release of node-addon-api 👍

@mhdawson
Copy link
Member

mhdawson commented Dec 7, 2020

Closing out since I think the original question was answered and the discussion closed out. Please let us know if that is not the right thing to do.

@mhdawson mhdawson closed this as completed Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants