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

gabrielschulhof · 2017-12-27T21:32:30Z

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 pointer and a
callback which will be invoked on the loop thread and which will
receive the napi_value resulting from the napi_ref so as to perform
the call into JS. The callback is run inside a node::CallbackScope.

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

This is an alternative, and perhaps simpler implementation of napi_threadsafe_function, alternative to #17809

gabrielschulhof · 2018-01-02T17:54:57Z

@mika-fischer:

The name napi_async_function with the suggested semantics is very misleading, because the expectation is that the function will be called whenever the async_function is invoked.

This PR does not address this issue. @bnoordhuis suggested that I add a mutex to allow the secondary thread to wait for the call to happen on the loop thread. I can do that, however,

If I do that, I need to expose both the mutex uv_mutex_lock() and uv_mutex_trylock() so that the secondary thread may wait in either a blocking or non-blocking mode.
The user of napi_threadsafe_function can do this themselves, unless we want to shield them from the libuv mutex APIs as well.

There will be lifetime issues if you put data into napi_call_async_function, expecting to free it in napi_async_function_args_to_js_cb, but that is never called if you're unlucky.

This PR addresses this issue, because it leaves the creation/destruction of the vector holding the napi_value items that are used as JavaScript function arguments to the user, and does not accept any per-call void* pointers. The expectation is that the call_js_cb will be able to create from the original void* with which the thread-safe function was created the JavaScript function arguments needed for this call. OTOH, this API has no control over the way the JavaScript function is called, which might actually be a good thing, because the user may do more than just make a function call on the loop thread. The PR takes care to make sure that the call_js_cb runs in a callback context.

Your control flow might wait for things to happen in napi_async_function_process_return_cb, but that may never get called.

This PR addresses this issue because it leaves the calling and return value processing up to the user.

I also made another PR which keeps the marshalling and the return value processing separate. Note that the situation wherein the callback(s) on the loop thread may never get called is one that cannot be avoided in either PR. I mean, that's the nature of the call being asynchronous, right? If you make a call from the secondary thread and the loop thread cleans up by calling napi_delete_threadsafe_function() (and uv_async_destroy() as a result of that) before the corresponding call happens on the loop thread then UV removes the pending call from the loop, AFAICT. @bnoordhuis right?

bnoordhuis · 2018-01-03T20:09:23Z

@bnoordhuis right?

Correct.

yizhang82 · 2018-01-29T05:33:16Z

@gabrielschulhof I think it would be great to allow user to provide additional data/context information in napi_call_threadsafe_function . Imagine if you are receiving callbacks from an async non-JS thread and it sends you a bunch of data (say network data buffer pointer, buffer size, etc) that you won't know ahead of time, and you would have to send those data to the threadsafe function.

yizhang82 · 2018-01-29T05:41:57Z

hmm.. looks like napi_call_threadsafe_function wraps over uv_async_send which seems to be why the limitation is there. This is really unfortunate as this forces everyone to implement their own event queue.... Hopefully libuv can fix it. Another option is for n-api to implement a event queue so that we don't have to.

gabrielschulhof · 2018-01-29T13:53:20Z

The idea with `napi_call_threadsafe_function()` is that the secondary thread first calls `napi_get_threadsafe_function_data()`, manipulates the data at that pointer, then calls `napi_call_threadsafe_function()`. When the callback gets called on the loop thread, it also calls `napi_get_threadsafe_function_data()`, retrieves the data created by the secondary thread, and calls into JS. This way, each call into JS can be different from the other. We did indeed discuss adding a queue to the implementation so as to ensure a one-to-one ratio of calls on the secondary thread to calls on the loop thread. I'd still rather leave the data management to the implementation, though.

yizhang82 · 2018-01-29T18:07:37Z

The idea with napi_call_threadsafe_function() is that the secondary
thread first calls napi_get_threadsafe_function_data(), manipulates the
data at that pointer, then calls napi_call_threadsafe_function(). When
the callback gets called on the loop thread, it also calls
napi_get_threadsafe_function_data(), retrieves the data created by the
secondary thread, and calls into JS.

As you've pointed out later, this only works with one call at a time. In the case of having multiple async threads sending events in parallel, the data needs to be a queue otherwise you would be overriding each other. Or even one thread sending events in parallel to libuv event loop - resulting in a race condition.

We did indeed discuss adding a queue to the implementation so as to ensure
a one-to-one ratio of calls on the secondary thread to calls on the loop
thread. I'd still rather leave the data management to the implementation,
though.

The problem is that this pattern forces applications with the async event pattern into doing data management work that the API should be doing. The underlying issue seems to be that uv_async_send unfortunately doesn't allow you to pass additional void *. This is a major oversight in libuv in my opinion. For any kind of callback user should be able to pass their own data with each invocation. Without this support, user would be forced into using a queue to compensate for the missing functionality.

Ideally uv_async_send should really support sending void * with the handle. N-api should just be consuming it.

yizhang82 · 2018-01-29T19:47:54Z

cc @h27han

gabrielschulhof · 2018-01-29T21:57:16Z

It looks like a queue of void* pointers as well as a one-to-one ratio of secondary-thread-to-loop-thread calls is needed.

This means that:

The secondary thread would be responsible for creating the void* that gets passed to napi_call_threadsafe_function().
The napi_threadsafe_function_call_js callback would be responsible for destroying the void* in the loop thread.
The napi_threadsafe_function_call_js callback would be invoked multiple times in an idle loop until the queue is drained.

yizhang82 · 2018-01-30T00:13:49Z

Yes, having that would be super awesome. Thanks!

gabrielschulhof · 2018-02-02T16:26:55Z

@yizhang82 @simon-p-r how does the newest incarnation look? I've added the queue, the max_queue_size, and blocking and non-blocking addition.

addaleax · 2018-02-02T20:22:59Z

doc/api/n-api.md

+<!-- it's very convenient to have all the anchors indexed -->
+<!--lint disable no-unused-definitions remark-lint-->
+## Asynchronous Thread-safe Function Calls
+JavaScript functions can normally only be called from a native addon's main


Nit: Can you leave a blank line after each heading?

addaleax · 2018-02-02T20:49:27Z

src/node_api.cc

+    status = napi_generic_failure;
+  }
+destroy_mutex:
+  uv_mutex_destroy(&ts_fn->mutex);


Can we use the helpers from node_mutex.h for these things? They really improve readability for C++, all these gotos are really hard to wrap your head around…

Those abort if the mutex is not created successfully. N-API returns napi_generic_failure.

gabrielschulhof · 2018-02-03T01:08:35Z

@addaleax I have replaced the gotos with concentric if-statements.

yizhang82 · 2018-02-15T02:05:07Z

doc/api/n-api.md

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


What will happen if the queue is not empty and one calls delete?

Then the queue will be deleted shallowly and there will be no further calls into JS. At this point, the secondary threads should already be stopped, if the napi_threadsafe_function was used correctly.

yizhang82 · 2018-02-15T02:19:59Z

src/node_api.cc

+  CHECK_ENV(env);
+  CHECK_ARG(env, ts_fn);
+
+  napi_status status = napi_delete_reference(env, ts_fn->ref);


Will this result in a crash when there are pending items? Taking the mutex might help.

It probably makes sense to wait for the queue to drain:

I can protect myself from the race between delete and call, but I can't protect myself from the main event loop draining remaining item clashing with delete. I might be able to keep a count and track my own pending items, and only call delete when the count is 0 (with proper locks), but I can only protect my own function call and when the function returns all bets are off. IMHO this should be handled by the API layer instead of pushing it to all applications that consume this API.

The function calls in the queue should be eventually gets drained - otherwise user data can leak.

This shouldn't result in a crash if the napi_threadsafe_function is used as documented - that is, the threads are terminated before napi_delete_threadsafe_function() is called. For that same reason I need not lock the mutex. napi_delete_threadsafe_function() must always be called from the loop thread, so it's synchronous wrt. the idle callback and async callback. So, after napi_delete_threadsafe_function() completes libuv guarantees that there will be no more calls to the idle handler which would access the queue nor to the async callback which would set up an idle handler.

The items in the queue would indeed leak in such a case.

I don't think it's necessarily a good idea to wait for the queue to drain. I think being able to abort is a good thing - perhaps if there's a fatal error somewhere. In normal usage, the user would be aware of the consequences of deleting the thread-safe function.

So, I think we should either document that it's the user's responsibility to avoid leaks perhaps by making the last item on the queue be a terminator, or provide a delete-once-empty API in addition to napi_delete_threadsafe_function(), like napi_delete_threadsafe_function_deferred().

yizhang82 · 2018-02-15T02:39:00Z

src/node_api.cc

+          });
+      });
+
+  return napi_clear_last_error(env);


I might be missing it - is there a place ts_fn gets deleted?

ts_fn == handle in the innermost lambda. @bnoordhuis advised me to stop making the assumption throughout the code that &ts_fn->async == ts_fn, but I don't believe I can avoid making that assumption when deleting the handle, because all I get from libuv is a uv_handle_t *handle for the innermost lambda.

Actually, I found a way of removing the assumption.

mhdawson

Still looks good to me.

gabrielschulhof · 2018-06-29T18:57:16Z

CI: https://ci.nodejs.org/job/node-test-pull-request/15669/

gabrielschulhof · 2018-06-29T21:19:52Z

Landed in 81f06ba.

Bundle a `uv_async_t`, a `uv_idle_t`, a `uv_mutex_t`, a `uv_cond_t`, and a `v8::Persistent<v8::Function>` to make it possible to call into JS from another thread. The API accepts a void data pointer and a callback which will be invoked on the loop thread and which will receive the `napi_value` representing the JavaScript function to call so as to perform the call into JS. The callback is run inside a `node::CallbackScope`. A `std::queue<void*>` is used to store calls from the secondary threads, and an idle loop is started by the `uv_async_t` callback on the loop thread to drain the queue, calling into JS with each item. Items can be added to the queue blockingly or non-blockingly. The thread-safe function can be referenced or unreferenced, with the same semantics as libuv handles. Re: nodejs/help#1035 Re: #20964 Fixes: #13512 PR-URL: #17887 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

private field 'async_context' is not used [-Wunused-private-field] PR-URL: nodejs#21597 Refs: nodejs#17887 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

private field 'async_context' is not used [-Wunused-private-field] PR-URL: #21597 Refs: #17887 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

Notable changes: * build: * Node.js should now be about 60% faster to startup than the previous version, thanks to the use V8's code cache feature for core modules. [#21405](#21405) * dns: * An experimental promisified version of the dns module is now available. Give it a try with `require('dns').promises`. [#21264](#21264) * fs: * `fs.lchown` has been undeprecated now that libuv supports it. [#21498](#21498) * lib: * `Atomics.wake` is being renamed to `Atomics.notify` in the ECMAScript specification ([reference](tc39/ecma262#1220)). Since Node.js now has experimental support for worker threads, we are being proactive and added a `notify` alias, while emitting a warning if `wake` is used. [#21413](#21413) [#21518](#21518) * n-api: * Add API for asynchronous functions. [#17887](#17887) * util: * `util.inspect` is now able to return a result instead of throwing when the maximum call stack size is exceeded during inspection. [#20725](#20725) * vm: * Add `script.createCachedData()`. This API replaces the `produceCachedData` option of the `Script` constructor that is now deprecated. [#20300](#20300) * worker: * Support for relative paths has been added to the `Worker` constructor. Paths are interpreted relative to the current working directory. [#21407](#21407) PR-URL: #21629

Notable changes: * dns: * An experimental promisified version of the dns module is now available. Give it a try with `require('dns').promises`. [#21264](#21264) * fs: * `fs.lchown` has been undeprecated now that libuv supports it. [#21498](#21498) * lib: * `Atomics.wake` is being renamed to `Atomics.notify` in the ECMAScript specification ([reference](tc39/ecma262#1220)). Since Node.js now has experimental support for worker threads, we are being proactive and added a `notify` alias, while emitting a warning if `wake` is used. [#21413](#21413) [#21518](#21518) * n-api: * Add API for asynchronous functions. [#17887](#17887) * util: * `util.inspect` is now able to return a result instead of throwing when the maximum call stack size is exceeded during inspection. [#20725](#20725) * vm: * Add `script.createCachedData()`. This API replaces the `produceCachedData` option of the `Script` constructor that is now deprecated. [#20300](#20300) * worker: * Support for relative paths has been added to the `Worker` constructor. Paths are interpreted relative to the current working directory. [#21407](#21407) PR-URL: #21629

Bundle a `uv_async_t`, a `uv_idle_t`, a `uv_mutex_t`, a `uv_cond_t`, and a `v8::Persistent<v8::Function>` to make it possible to call into JS from another thread. The API accepts a void data pointer and a callback which will be invoked on the loop thread and which will receive the `napi_value` representing the JavaScript function to call so as to perform the call into JS. The callback is run inside a `node::CallbackScope`. A `std::queue<void*>` is used to store calls from the secondary threads, and an idle loop is started by the `uv_async_t` callback on the loop thread to drain the queue, calling into JS with each item. Items can be added to the queue blockingly or non-blockingly. The thread-safe function can be referenced or unreferenced, with the same semantics as libuv handles. Re: nodejs/help#1035 Re: nodejs#20964 Fixes: nodejs#13512 PR-URL: nodejs#17887 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

private field 'async_context' is not used [-Wunused-private-field] PR-URL: nodejs#21597 Refs: nodejs#17887 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

Bundle a `uv_async_t`, a `uv_idle_t`, a `uv_mutex_t`, a `uv_cond_t`, and a `v8::Persistent<v8::Function>` to make it possible to call into JS from another thread. The API accepts a void data pointer and a callback which will be invoked on the loop thread and which will receive the `napi_value` representing the JavaScript function to call so as to perform the call into JS. The callback is run inside a `node::CallbackScope`. A `std::queue<void*>` is used to store calls from the secondary threads, and an idle loop is started by the `uv_async_t` callback on the loop thread to drain the queue, calling into JS with each item. Items can be added to the queue blockingly or non-blockingly. The thread-safe function can be referenced or unreferenced, with the same semantics as libuv handles. Re: nodejs/help#1035 Re: #20964 Fixes: #13512 Backport-PR-URL: #25002 PR-URL: #17887 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

private field 'async_context' is not used [-Wunused-private-field] PR-URL: #21597 Refs: #17887 Backport-PR-URL: #25002 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

Bundle a `uv_async_t`, a `uv_idle_t`, a `uv_mutex_t`, a `uv_cond_t`, and a `v8::Persistent<v8::Function>` to make it possible to call into JS from another thread. The API accepts a void data pointer and a callback which will be invoked on the loop thread and which will receive the `napi_value` representing the JavaScript function to call so as to perform the call into JS. The callback is run inside a `node::CallbackScope`. A `std::queue<void*>` is used to store calls from the secondary threads, and an idle loop is started by the `uv_async_t` callback on the loop thread to drain the queue, calling into JS with each item. Items can be added to the queue blockingly or non-blockingly. The thread-safe function can be referenced or unreferenced, with the same semantics as libuv handles. Re: nodejs/help#1035 Re: #20964 Fixes: #13512 Backport-PR-URL: #25002 PR-URL: #17887 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

private field 'async_context' is not used [-Wunused-private-field] PR-URL: #21597 Refs: #17887 Backport-PR-URL: #25002 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

Notable Changes: * n-api: - add API for asynchronous functions (Gabriel Schulhof) #17887 - mark thread-safe function as stable (Gabriel Schulhof) #25556 PR-URL: #26933

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 27, 2017

gabrielschulhof changed the title ~~n-api: add API for asynchronous functions~~ n-api: add API for asynchronous functions (simpler version) Dec 27, 2017

gabrielschulhof force-pushed the napi-threadsafe-function-call-js branch from abb2e1f to ca6f1a8 Compare December 27, 2017 22:45

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

gabrielschulhof mentioned this pull request Jan 11, 2018

n-api: add API for asynchronous functions #17809

Closed

4 tasks

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

gabrielschulhof force-pushed the napi-threadsafe-function-call-js branch from ca6f1a8 to 4134195 Compare January 29, 2018 21:50

mhdawson added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 1, 2018

gabrielschulhof force-pushed the napi-threadsafe-function-call-js branch from 4134195 to e3e7324 Compare February 2, 2018 16:26

gabrielschulhof requested review from bnoordhuis and addaleax February 2, 2018 16:27

addaleax reviewed Feb 2, 2018

View reviewed changes

gabrielschulhof force-pushed the napi-threadsafe-function-call-js branch from e3e7324 to 33f5e4f Compare February 3, 2018 01:07

gabrielschulhof force-pushed the napi-threadsafe-function-call-js branch from 33f5e4f to a226f89 Compare February 10, 2018 17:14

yizhang82 reviewed Feb 15, 2018

View reviewed changes

mhdawson approved these changes Jun 29, 2018

View reviewed changes

forgot messages for new error statuses

f2da8d2

gabrielschulhof closed this Jun 29, 2018

cjihrig mentioned this pull request Jun 29, 2018

n-api: fix compiler warning #21597

Merged

2 tasks

targos mentioned this pull request Jul 3, 2018

v10.6.0 proposal #21629

Merged

gabrielschulhof deleted the napi-threadsafe-function-call-js branch July 3, 2018 21:44

MylesBorins mentioned this pull request Mar 26, 2019

v8.16.0 proposal #26933

Merged

MylesBorins added a commit that referenced this pull request Apr 5, 2019

2019-04-18, Version 8.16.0 'Carbon' (LTS)

3a7f44e

Notable Changes: * n-api: - add API for asynchronous functions (Gabriel Schulhof) #17887 - mark thread-safe function as stable (Gabriel Schulhof) #25556 PR-URL: #26933

MylesBorins added a commit that referenced this pull request Apr 16, 2019

2019-04-18, Version 8.16.0 'Carbon' (LTS)

4a6662c

Notable Changes: * n-api: - add API for asynchronous functions (Gabriel Schulhof) #17887 - mark thread-safe function as stable (Gabriel Schulhof) #25556 PR-URL: #26933

MylesBorins added a commit that referenced this pull request Apr 16, 2019

2019-04-16, Version 8.16.0 'Carbon' (LTS)

5f93cf7

Notable Changes: * n-api: - add API for asynchronous functions (Gabriel Schulhof) #17887 - mark thread-safe function as stable (Gabriel Schulhof) #25556 PR-URL: #26933

MylesBorins added a commit that referenced this pull request Apr 16, 2019

2019-04-16, Version 8.16.0 'Carbon' (LTS)

1c26169

Notable Changes: * n-api: - add API for asynchronous functions (Gabriel Schulhof) #17887 - mark thread-safe function as stable (Gabriel Schulhof) #25556 PR-URL: #26933

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gabrielschulhof commented Dec 27, 2017

gabrielschulhof commented Jan 2, 2018

bnoordhuis commented Jan 3, 2018

yizhang82 commented Jan 29, 2018

yizhang82 commented Jan 29, 2018 •

edited

gabrielschulhof commented Jan 29, 2018 via email

yizhang82 commented Jan 29, 2018 •

edited

yizhang82 commented Jan 29, 2018

gabrielschulhof commented Jan 29, 2018

yizhang82 commented Jan 30, 2018

gabrielschulhof commented Feb 2, 2018

addaleax Feb 2, 2018

addaleax Feb 2, 2018

gabrielschulhof Feb 3, 2018 •

edited

gabrielschulhof commented Feb 3, 2018

yizhang82 Feb 15, 2018

gabrielschulhof Feb 15, 2018

yizhang82 Feb 15, 2018

yizhang82 Feb 15, 2018

gabrielschulhof Feb 15, 2018

yizhang82 Feb 15, 2018

gabrielschulhof Feb 15, 2018

gabrielschulhof Feb 15, 2018

mhdawson left a comment

gabrielschulhof commented Jun 29, 2018

gabrielschulhof commented Jun 29, 2018

Navigation Menu

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

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

Conversation

gabrielschulhof commented Dec 27, 2017

Checklist

Affected core subsystem(s)

gabrielschulhof commented Jan 2, 2018

bnoordhuis commented Jan 3, 2018

yizhang82 commented Jan 29, 2018

yizhang82 commented Jan 29, 2018 • edited

gabrielschulhof commented Jan 29, 2018 via email

yizhang82 commented Jan 29, 2018 • edited

yizhang82 commented Jan 29, 2018

gabrielschulhof commented Jan 29, 2018

yizhang82 commented Jan 30, 2018

gabrielschulhof commented Feb 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielschulhof Feb 3, 2018 • edited

Choose a reason for hiding this comment

gabrielschulhof commented Feb 3, 2018

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

mhdawson left a comment

Choose a reason for hiding this comment

gabrielschulhof commented Jun 29, 2018

gabrielschulhof commented Jun 29, 2018

yizhang82 commented Jan 29, 2018 •

edited

yizhang82 commented Jan 29, 2018 •

edited

gabrielschulhof Feb 3, 2018 •

edited