n-api: make func argument of napi_create_threadsafe_function optional

PR-URL: nodejs#27791 Refs: nodejs#27592 Reviewed-By: Gabriel Schulhof <[email protected]>
gabrielschulhof · Sep 19, 2019 · 116f8ec · 116f8ec
1 parent 4f41e4f
commit 116f8ec
Show file tree

Hide file tree

Showing 4 changed files with 69 additions and 11 deletions.
diff --git a/doc/api/n-api.md b/doc/api/n-api.md
@@ -368,7 +368,8 @@ typedef void (*napi_threadsafe_function_call_js)(napi_env env,
 - `[in] env`: The environment to use for API calls, or `NULL` if the thread-safe
 function is being torn down and `data` may need to be freed.
 - `[in] js_callback`: The JavaScript function to call, or `NULL` if the
-thread-safe function is being torn down and `data` may need to be freed.
+thread-safe function is being torn down and `data` may need to be freed. It may
+also be `NULL` if the thread-safe function was created without `js_callback`.
 - `[in] context`: The optional data with which the thread-safe function was
 created.
 - `[in] data`: Data created by the secondary thread. It is the responsibility of
@@ -4543,6 +4544,11 @@ prevent the event loop from exiting. The APIs `napi_ref_threadsafe_function` and
 
 <!-- YAML
 added: v10.6.0
+napiVersion: 4
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/27791
+    description: Made `func` parameter optional with custom `call_js_cb`.
 -->
 ```C
 NAPI_EXTERN napi_status
@@ -4560,7 +4566,8 @@ napi_create_threadsafe_function(napi_env env,
 ```
 
 - `[in] env`: The environment that the API is invoked under.
-- `[in] func`: The JavaScript function to call from another thread.
+- `[in] func`: An optional JavaScript function to call from another thread.
+It must be provided if `NULL` is passed to `call_js_cb`.
 - `[in] async_resource`: An optional object associated with the async work that
 will be passed to possible `async_hooks` [`init` hooks][].
 - `[in] async_resource_name`: A JavaScript string to provide an identifier for

diff --git a/src/node_api.cc b/src/node_api.cc
@@ -1089,10 +1089,14 @@ class ThreadSafeFunction : public node::AsyncResource {
                                "ERR_NAPI_TSFN_STOP_IDLE_LOOP",
                                "Failed to stop the idle loop") == napi_ok);
       } else {
-        v8::Local<v8::Function> js_cb =
+        napi_value js_callback = nullptr;
+        if (!ref.IsEmpty()) {
+          v8::Local<v8::Function> js_cb =
             v8::Local<v8::Function>::New(env->isolate, ref);
+          js_callback = v8impl::JsValueFromV8LocalValue(js_cb);
+        }
         call_js_cb(env,
-                   v8impl::JsValueFromV8LocalValue(js_cb),
+                   js_callback,
                    context,
                    data);
       }
@@ -4168,15 +4172,18 @@ napi_create_threadsafe_function(napi_env env,
                                 napi_threadsafe_function_call_js call_js_cb,
                                 napi_threadsafe_function* result) {
   CHECK_ENV(env);
-  CHECK_ARG(env, func);
   CHECK_ARG(env, async_resource_name);
   RETURN_STATUS_IF_FALSE(env, initial_thread_count > 0, napi_invalid_arg);
   CHECK_ARG(env, result);
 
   napi_status status = napi_ok;
 
   v8::Local<v8::Function> v8_func;
-  CHECK_TO_FUNCTION(env, v8_func, func);
+  if (func == nullptr) {
+    CHECK_ARG(env, call_js_cb);
+  } else {
+    CHECK_TO_FUNCTION(env, v8_func, func);
+  }
 
   v8::Local<v8::Context> v8_context = env->context();
 

diff --git a/test/addons-napi/test_threadsafe_function/binding.c b/test/addons-napi/test_threadsafe_function/binding.c
@@ -129,6 +129,19 @@ static void call_js(napi_env env, napi_value cb, void* hint, void* data) {
   }
 }
 
+static napi_ref alt_ref;
+// Getting the data into JS with the alternative referece
+static void call_ref(napi_env env, napi_value _, void* hint, void* data) {
+  if (!(env == NULL || alt_ref == NULL)) {
+    napi_value fn, argv, undefined;
+    NAPI_CALL_RETURN_VOID(env, napi_get_reference_value(env, alt_ref, &fn));
+    NAPI_CALL_RETURN_VOID(env, napi_create_int32(env, *(int*)data, &argv));
+    NAPI_CALL_RETURN_VOID(env, napi_get_undefined(env, &undefined));
+    NAPI_CALL_RETURN_VOID(env, napi_call_function(env, undefined, fn, 1, &argv,
+        NULL));
+  }
+}
+
 // Cleanup
 static napi_value StopThread(napi_env env, napi_callback_info info) {
   size_t argc = 2;
@@ -168,20 +181,31 @@ static void join_the_threads(napi_env env, void *data, void *hint) {
       napi_call_function(env, undefined, js_cb, 0, NULL, NULL));
   NAPI_CALL_RETURN_VOID(env, napi_delete_reference(env,
       the_hint->js_finalize_cb));
+  if (alt_ref != NULL) {
+    NAPI_CALL_RETURN_VOID(env, napi_delete_reference(env, alt_ref));
+    alt_ref = NULL;
+  }
 }
 
 static napi_value StartThreadInternal(napi_env env,
                                       napi_callback_info info,
                                       napi_threadsafe_function_call_js cb,
-                                      bool block_on_full) {
+                                      bool block_on_full,
+                                      bool alt_ref_js_cb) {
+
   size_t argc = 4;
   napi_value argv[4];
 
+  NAPI_CALL(env, napi_get_cb_info(env, info, &argc, argv, NULL, NULL));
+  if (alt_ref_js_cb) {
+    NAPI_CALL(env, napi_create_reference(env, argv[0], 1, &alt_ref));
+    argv[0] = NULL;
+  }
+
   ts_info.block_on_full =
       (block_on_full ? napi_tsfn_blocking : napi_tsfn_nonblocking);
 
   NAPI_ASSERT(env, (ts_fn == NULL), "Existing thread-safe function");
-  NAPI_CALL(env, napi_get_cb_info(env, info, &argc, argv, NULL, NULL));
   napi_value async_name;
   NAPI_CALL(env, napi_create_string_utf8(env, "N-API Thread-safe Function Test",
       NAPI_AUTO_LENGTH, &async_name));
@@ -223,16 +247,24 @@ static napi_value Release(napi_env env, napi_callback_info info) {
 
 // Startup
 static napi_value StartThread(napi_env env, napi_callback_info info) {
-  return StartThreadInternal(env, info, call_js, true);
+  return StartThreadInternal(env, info, call_js,
+    /** block_on_full */true, /** alt_ref_js_cb */false);
 }
 
 static napi_value StartThreadNonblocking(napi_env env,
                                          napi_callback_info info) {
-  return StartThreadInternal(env, info, call_js, false);
+  return StartThreadInternal(env, info, call_js,
+    /** block_on_full */false, /** alt_ref_js_cb */false);
 }
 
 static napi_value StartThreadNoNative(napi_env env, napi_callback_info info) {
-  return StartThreadInternal(env, info, NULL, true);
+  return StartThreadInternal(env, info, NULL,
+    /** block_on_full */true, /** alt_ref_js_cb */false);
+}
+
+static napi_value StartThreadNoJsFunc(napi_env env, napi_callback_info info) {
+  return StartThreadInternal(env, info, call_ref,
+    /** block_on_full */true, /** alt_ref_js_cb */true);
 }
 
 // Module init
@@ -269,6 +301,7 @@ static napi_value Init(napi_env env, napi_value exports) {
     DECLARE_NAPI_PROPERTY("StartThread", StartThread),
     DECLARE_NAPI_PROPERTY("StartThreadNoNative", StartThreadNoNative),
     DECLARE_NAPI_PROPERTY("StartThreadNonblocking", StartThreadNonblocking),
+    DECLARE_NAPI_PROPERTY("StartThreadNoJsFunc", StartThreadNoJsFunc),
     DECLARE_NAPI_PROPERTY("StopThread", StopThread),
     DECLARE_NAPI_PROPERTY("Unref", Unref),
     DECLARE_NAPI_PROPERTY("Release", Release),

diff --git a/test/addons-napi/test_threadsafe_function/test.js b/test/addons-napi/test_threadsafe_function/test.js
@@ -102,6 +102,17 @@ new Promise(function testWithoutJSMarshaller(resolve) {
 }))
 .then((result) => assert.deepStrictEqual(result, expectedArray))
 
+// Start the thread in blocking mode, and assert that all values are passed.
+// Quit after it's done.
+// Doesn't pass the callback js function to napi_create_threadsafe_function.
+// Instead, use an alternative reference to get js function called.
+.then(() => testWithJSMarshaller({
+  threadStarter: 'StartThreadNoJsFunc',
+  maxQueueSize: binding.MAX_QUEUE_SIZE,
+  quitAfter: binding.ARRAY_LENGTH
+}))
+.then((result) => assert.deepStrictEqual(result, expectedArray))
+
 // Start the thread in blocking mode with an infinite queue, and assert that all
 // values are passed. Quit after it's done.
 .then(() => testWithJSMarshaller({