n-api: add API for asynchronous functions #17809

gabrielschulhof · 2017-12-21T22:52:00Z

Bundle a uv_async_t and a napi_ref to make it possible to call into JS from
another thread. The API accepts a void data and context pointer, an optional
native-to-JS function argument marshaller, and a JS-to-native return value
marshaller.

Fixes: #13512

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

n-api

jasnell · 2017-12-22T17:46:13Z

doc/api/n-api.md

+-->
+```C
+NAPI_EXTERN napi_status
+napi_create_async_function(napi_env env,


hmm.. calling this create_async_function might confuse developers who are using promises since async function() has a very specific meaning in JS

@jasnell napi_async_call?

napi_threadsafe_function?

threadsafe works for me :)

addaleax · 2017-12-23T04:33:06Z

doc/api/n-api.md

+- `[in] env`: The environment that the API is invoked under.
+- `[in] func`: The JavaScript function to call from another thread.
+- `[in] data`: Optional data to attach to the resulting `napi_async_function`.
+- `[in] context`: Optional context associated with `data`.


At a glance, there doesn’t appear to be any difference between data and context?

You're right - I think they pretty much go hand-in-hand.

addaleax · 2017-12-23T04:48:07Z

doc/api/n-api.md

+
+### napi_call_async_function
+<!-- YAML
+added: UPDATEME


REPLACEME is picked up by tooling automatically :) I think you should also update the NAPI_VERSION #define in node_api.cc?

Aaaah! Forgot the magic word :)

gabrielschulhof · 2017-12-23T16:36:06Z

@jasnell @addaleax I have addressed your review comments. I went with napi_threadsafe_function.

gabrielschulhof · 2017-12-23T16:37:47Z

I also removed the context parameter, and updated the YAML tag and the NAPI_VERSION.

gabrielschulhof · 2017-12-23T16:41:36Z

Whoops! Forgot to remove the context parameter in the API documentation.

gabrielschulhof · 2017-12-23T17:19:29Z

... and I documented the function pointer types, and updated the version test to expect version 3.

cjihrig · 2017-12-27T16:52:55Z

doc/api/n-api.md

+function context and its arguments from the native data associated with the
+thread-safe function and store them in `recv` and `argv`, respsectively.
+Callback functions must satisfy the following signature:
+typedef napi_status(*napi_threadsafe_function_marshal)(napi_env env,


The typedef here and below should be formatted as C code.

cjihrig · 2017-12-27T16:53:54Z

doc/api/n-api.md

@@ -203,6 +204,30 @@ typedef void (*napi_async_complete_callback)(napi_env env,
                                             void* data);
 ```

+#### napi_threadsafe_function_marshal


I'm not sure if these headers should be italicized or not. It's inconsistent throughout the file.

cjihrig · 2017-12-27T16:58:29Z

doc/api/n-api.md

+-->
+```C
+NAPI_EXTERN napi_status napi_get_threadsafe_function_data(napi_threadsafe_function func,
+                                                     void** data,


Can you line up the function arguments.

cjihrig · 2017-12-27T17:00:03Z

src/node_api.cc

+                                                 const char* message) {
+  napi_value result, js_message;
+  if (napi_create_string_utf8(env, message, NAPI_AUTO_LENGTH, &js_message)
+      == napi_ok) {


The == should be on the previous line I think.

cjihrig · 2017-12-27T17:00:28Z

src/node_api.cc

+    }
+  }
+
+  napi_fatal_error("N-API async function", NAPI_AUTO_LENGTH,


Should async be threadsafe here?

bnoordhuis · 2017-12-27T17:21:45Z

doc/api/n-api.md

+If an exception was thrown during the execution of the JavaScript function, it
+will be made available in the `error` parameter. The `result` parameter will
+have the function's return value.
+gives users of `napi_threadsafe_function` an oppor


Stray line.

bnoordhuis · 2017-12-27T17:23:54Z

doc/api/n-api.md

+
+Since `uv_async_t` is used in the implementation, the caveat whereby multiple
+invocations on the secondary thread may result in only one invocation of the
+JavaScript function also applies to `napi_threadsafe_function`.


There's no reason it must be that way. Add a mutex and a list or queue, then the uv_async_t becomes like an event-driven condition variable.

@bnoordhuis @nodejs/addon-api This is indeed value that we could add and this was indeed requested, however, implementing this requires answers to a host of design questions, as the linked comment reveals towards the end.

So, we need to discuss how far beyond exposing libuv and Node functionality we wish to go.

If we decide to implement this queuing, then, in addition to the questions raised by the comment, we need to decide whether to drain the queue using a busy loop or an idle loop, and what limit we should place on the length of the queue. Do we expose both lock and trylock for the mutex so that users might choose whether to block the secondary thread when the queue gets full? How do we dispose of the void* data once the JavaScript function has been called? How close would using such a queue bring this implementation to that of a pipe, and is it worth at that point starting down this path?

TBH, I was going in the opposite direction with this PR (that is, providing less value beyond the essential, rather than more), but I haven't yet had a chance to submit the changes. Instead of a marshal_cb and process_return_cb, I was just going to go with

typedef void (*napi_threadsafe_function_call_js)(napi_env env, napi_value func, void* data);

thereby unifying the marshalling and return value processing step. I did that because I realized that we have a lot of if (status != napi_ok) { napi_throw_error(); } and if (status != napi_ok) { napi_fatal_error(); } on the N-API side of the code, and that's perhaps not how the user would want to deal with such failures. Having a single function pointer would push this handling onto the user. Nevertheless, the function pointer would be called from withing a node::CallbackScope so we wouldn't have to remind the user to call the function via napi_make_callback().

I suppose the deeper question is whether we want users using libuv at all. After all, the void* could be a struct { uv_mutex_t mutex; /* useful and thread-safe stuff here; */ }; and then the user would be implementing their own queuing, or any other semantics.

In fact, my only reason for originally splitting the marshalling and return value processing into two steps was to ensure that the actual calling of the JavaScript function was under my control, whereby I could ensure that the call happened via napi_make_callback(). Yet in that incarnation I wasn't using any Node internal APIs, meaning that this whole PR could be implemented externally to Node.

bnoordhuis · 2017-12-27T17:26:13Z

src/node_api.cc

+
+static void napi_threadsafe_function_cb(uv_async_t* uv_async) {
+  napi_threadsafe_function async =
+      reinterpret_cast<napi_threadsafe_function>(uv_async);


Use ContainerOf().

bnoordhuis · 2017-12-27T17:27:46Z

src/node_api.cc

+  napi_handle_scope handle_scope;
+  napi_status status = napi_open_handle_scope(async->env, &handle_scope);
+  if (status == napi_ok) {
+    napi_value js_cb, recv, argv[async->argc], js_result = nullptr,


VLAs are not 100% portable and prone to blow up the stack when argc is large. You should probably use a std::vector here.

Style: each definition should go on its own line (and separated with ;, not ,.)

bnoordhuis · 2017-12-27T17:31:12Z

src/node_api.cc

+
+done:
+    async->process_result_cb(async->env, async->data, exception, js_result);
+    napi_close_handle_scope(async->env, handle_scope);


There's a C++ equivalent with proper RAII, right? Why don't you use that?

bnoordhuis · 2017-12-27T17:49:38Z

test/addons-napi/test_threadsafe_function/test_basic.c

+
+static napi_value StartThread(napi_env env, napi_callback_info info) {
+  const char* error;
+  size_t argc = 2;


arraysize(argv)? I realize this is a C file but is there any reason it needs to be?

That's a node internal, right? We shouldn't be using those in tests.

bnoordhuis · 2017-12-27T17:49:53Z

test/addons-napi/test_threadsafe_function/test_basic.c

+    goto throw;
+  }
+
+  TestAsyncData* async_data = malloc(sizeof(TestAsyncData));


sizeof(*async_data)

bnoordhuis · 2017-12-27T17:53:15Z

test/addons-napi/test_threadsafe_function/test_basic.c

+    goto delete_async_function;
+  }
+
+  if (uv_thread_create((uv_thread_t*)async_data, BasicTestThread,


&async_data->thread - I've pointed out similar issues several times now and I'll stop doing that from here on. Just stop using this dangerous idiom from now on.

bnoordhuis · 2017-12-27T17:55:49Z

test/addons-napi/test_threadsafe_function/test_basic.c

+    {"StartThread", NULL, StartThread, NULL, NULL, NULL, napi_enumerable, NULL},
+    {"StopThread", NULL, StopThread, NULL, NULL, NULL, napi_enumerable, NULL}
+  };
+  init(env, exports, sizeof(props)/sizeof(props[0]), props);


arraysize(props)

bnoordhuis · 2017-12-27T17:56:22Z

test/addons-napi/test_threadsafe_function/test_empty.c

+    {"StopEmptyThread", NULL, StopEmptyThread, NULL, NULL, NULL,
+        napi_enumerable, NULL}
+  };
+  init(env, exports, sizeof(props)/sizeof(props[0]), props);


Bundle a uv_async_t and a napi_ref to make it possible to call into JS from another thread. The API accepts a void data and context pointer, an optional native-to-JS function argument marshaller, and a JS-to-native return value marshaller. Fixes: nodejs#13512

gabrielschulhof · 2018-01-11T19:15:27Z

Closing this in favour of #17887.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 21, 2017

gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Dec 21, 2017

jasnell requested a review from addaleax December 22, 2017 17:45

jasnell reviewed Dec 22, 2017

View reviewed changes

bnoordhuis mentioned this pull request Dec 23, 2017

Calling n-api methods from own separate thread nodejs/help#1035

Closed

addaleax reviewed Dec 23, 2017

View reviewed changes

gabrielschulhof force-pushed the napi-async-function branch from 5e335bb to 945568a Compare December 23, 2017 16:35

jasnell approved these changes Dec 23, 2017

View reviewed changes

gabrielschulhof force-pushed the napi-async-function branch from 945568a to eff8bb3 Compare December 23, 2017 17:18

cjihrig reviewed Dec 27, 2017

View reviewed changes

bnoordhuis reviewed Dec 27, 2017

View reviewed changes

gabrielschulhof mentioned this pull request Dec 27, 2017

n-api: add API for asynchronous functions (simpler version) #17887

Closed

4 tasks

gabrielschulhof force-pushed the napi-async-function branch from eff8bb3 to 65e8f05 Compare December 27, 2017 22:39

gabrielschulhof closed this Jan 11, 2018

gabrielschulhof deleted the napi-async-function branch June 6, 2018 02:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

n-api: add API for asynchronous functions #17809

n-api: add API for asynchronous functions #17809

gabrielschulhof commented Dec 21, 2017

jasnell Dec 22, 2017

gabrielschulhof Dec 23, 2017

gabrielschulhof Dec 23, 2017

jasnell Dec 23, 2017

addaleax Dec 23, 2017

gabrielschulhof Dec 23, 2017

addaleax Dec 23, 2017

gabrielschulhof Dec 23, 2017

gabrielschulhof commented Dec 23, 2017

gabrielschulhof commented Dec 23, 2017

gabrielschulhof commented Dec 23, 2017

gabrielschulhof commented Dec 23, 2017

cjihrig Dec 27, 2017

cjihrig Dec 27, 2017

cjihrig Dec 27, 2017

cjihrig Dec 27, 2017

cjihrig Dec 27, 2017

bnoordhuis Dec 27, 2017

bnoordhuis Dec 27, 2017

gabrielschulhof Dec 27, 2017

bnoordhuis Dec 27, 2017

bnoordhuis Dec 27, 2017

bnoordhuis Dec 27, 2017

bnoordhuis Dec 27, 2017

gabrielschulhof Dec 27, 2017

bnoordhuis Dec 27, 2017

bnoordhuis Dec 27, 2017 •

edited

bnoordhuis Dec 27, 2017

bnoordhuis Dec 27, 2017

gabrielschulhof commented Jan 11, 2018

n-api: add API for asynchronous functions #17809

n-api: add API for asynchronous functions #17809

Conversation

gabrielschulhof commented Dec 21, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielschulhof commented Dec 23, 2017

gabrielschulhof commented Dec 23, 2017

gabrielschulhof commented Dec 23, 2017

gabrielschulhof commented Dec 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis Dec 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielschulhof commented Jan 11, 2018

bnoordhuis Dec 27, 2017 •

edited