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

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

Closed
wants to merge 8 commits into
base: master
from

Conversation

@gabrielschulhof
Contributor

gabrielschulhof commented Dec 27, 2017

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 gabrielschulhof changed the title from n-api: add API for asynchronous functions to n-api: add API for asynchronous functions (simpler version) Dec 27, 2017

@gabrielschulhof

This comment has been minimized.

Contributor

gabrielschulhof commented Jan 2, 2018

@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

This comment has been minimized.

Member

bnoordhuis commented Jan 3, 2018

@bnoordhuis right?

Correct.

@gabrielschulhof gabrielschulhof referenced this pull request Jan 11, 2018

Closed

n-api: add API for asynchronous functions #17809

4 of 4 tasks complete
@yizhang82

This comment has been minimized.

yizhang82 commented Jan 29, 2018

@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

This comment has been minimized.

yizhang82 commented Jan 29, 2018

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

This comment has been minimized.

Contributor

gabrielschulhof commented Jan 29, 2018

@yizhang82

This comment has been minimized.

yizhang82 commented Jan 29, 2018

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

This comment has been minimized.

yizhang82 commented Jan 29, 2018

cc @h27han

@gabrielschulhof

This comment has been minimized.

Contributor

gabrielschulhof commented Jan 29, 2018

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

This comment has been minimized.

yizhang82 commented Jan 30, 2018

Yes, having that would be super awesome. Thanks!

@gabrielschulhof

This comment has been minimized.

Contributor

gabrielschulhof commented Feb 2, 2018

@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.

@gabrielschulhof gabrielschulhof requested review from bnoordhuis and addaleax Feb 2, 2018

<!-- 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

This comment has been minimized.

@addaleax

addaleax Feb 2, 2018

Member

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

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

This comment has been minimized.

@addaleax

addaleax Feb 2, 2018

Member

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…

This comment has been minimized.

@gabrielschulhof

gabrielschulhof Feb 3, 2018

Contributor

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

@gabrielschulhof

This comment has been minimized.

Contributor

gabrielschulhof commented Feb 3, 2018

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

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

This comment has been minimized.

@yizhang82

yizhang82 Feb 15, 2018

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

This comment has been minimized.

@gabrielschulhof

gabrielschulhof Feb 15, 2018

Contributor

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.

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

This comment has been minimized.

@yizhang82

yizhang82 Feb 15, 2018

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

This comment has been minimized.

@yizhang82

yizhang82 Feb 15, 2018

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 comment has been minimized.

@gabrielschulhof

gabrielschulhof Feb 15, 2018

Contributor

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().

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

This comment has been minimized.

@yizhang82

yizhang82 Feb 15, 2018

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

This comment has been minimized.

@gabrielschulhof

gabrielschulhof Feb 15, 2018

Contributor

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.

This comment has been minimized.

@gabrielschulhof

gabrielschulhof Feb 15, 2018

Contributor

Actually, I found a way of removing the assumption.

API may be called from any thread.
- `[in] data`: Data to send into JavaScript via the callback `call_js_cb`
provided during the creation of the thread-safe JavaScript function.
- `[out] result`: Optional flag receiving whether the data item was

This comment has been minimized.

@yizhang82

yizhang82 Feb 15, 2018

Personally I think this is a bit counter-intuitive. I would probably have a bool should_block_when_full, and return napi_error_queue_full when should_block_when_full= false. I don't feel strongly, though.

@yizhang82

This comment has been minimized.

yizhang82 commented Feb 15, 2018

@gabrielschulhof I just left my comments on your PR. Sorry about the delay. This is the first time I review N-api related implementation so please bear with me if the details are off.
I think one of the question that needs to be answered is how delete interact with the event loop. IMHO delete should wait for the queue to drain to avoid crashes and/or leaks. Of course, it's user's responsibility to stop further events from arriving so that the wait can actually finish.

@gabrielschulhof

This comment has been minimized.

Contributor

gabrielschulhof commented Feb 15, 2018

@yizhang82 you're right. I'll add the queue-draining behaviour. It remains very important that the threads be stopped before napi_delete_threadsafe_function() be called, because once napi_delete_threadsafe_function() is called, there's a race between the secondary threads providing data and the loop thread draining data.

@yizhang82

This comment has been minimized.

yizhang82 commented Feb 16, 2018

Yes, I agree user should stop posting further events before calling delete.

After thinking about this a bit more, I think delete should not block on queue drain - it'll dead lock if people call delete on JS thread which then waits for the queue to drain in idle.

Instead, like what you suggested earlier, delete should post a deletion to the loop that deletes itself - basically your napi_delete_threadsafe_function_deferred() suggestion.

Is that what you are planning to do?

@gabrielschulhof

This comment has been minimized.

Contributor

gabrielschulhof commented Feb 16, 2018

@yizhang82 that's what I'm planning to do. I added an is_closing flag to the structure which, when set, will cause the idle handler to delete the napi_threadsafe_function immediately after removing the last item from the queue and calling into JS for the last time.

@gabrielschulhof

This comment has been minimized.

Contributor

gabrielschulhof commented Feb 19, 2018

@yizhang82 I don't think you can delete the thread-safe function unless the queue is empty, because the deleter has to uv_thread_join() all the threads before calling napi_delete_threadsafe_function(). uv_thread_join() will block the JS thread, preventing the idle loop from draining the queue.

So, I think the workflow will be that the value(s) which indicate(s) that it is time to call the deleter will come from the secondary thread(s) and the JS async callback will decide based upon the value(s) received whether to call the deleter.

We also cannot allow that uv_thread_join() happen after the call to napi_delete_threadsafe_function() because, although napi_delete_threadsafe_function() could then be used to place any blocked threads into a state where they are poised to quit, it is conceivable that those threads will not wake up before the JS thread has drained the queue and has thus destroyed the thread-safe function. So when the threads do eventually wake up they'll be unlocking a mutex which has already been destroyed 💣

I think we could use a semaphore, but then we would have to know a priori how many threads there are.

@gabrielschulhof

This comment has been minimized.

Contributor

gabrielschulhof commented Mar 7, 2018

@yizhang82 what do you think? Should we change the API to accept the expected number of threads a priori and then let you fire and forget?

@gabrielschulhof

This comment has been minimized.

Contributor

gabrielschulhof commented Jun 19, 2018

@mhdawson, @mcollina, @addaleax I had to do a substantial re-write to fix the Windows issue. On the upside, the implementation now makes proper use of node::AsyncResource.

Let's see if the CI is all good: https://ci.nodejs.org/job/node-test-pull-request/15523/

@gabrielschulhof

This comment has been minimized.

Contributor

gabrielschulhof commented Jun 19, 2018

@gabrielschulhof

This comment has been minimized.

Contributor

gabrielschulhof commented Jun 19, 2018

@gabrielschulhof

This comment has been minimized.

Contributor

gabrielschulhof commented Jun 19, 2018

@addaleax, @mhdawson, @mcollina it looks like the tests are green now. Please have another look and I'll land it.

@mhdawson

This comment has been minimized.

Member

mhdawson commented Jun 19, 2018

@gabrielf is there some way I can look at what changed for the windows issue as opposed to having to review the full set of changes again?

@gabrielschulhof

This comment has been minimized.

Contributor

gabrielschulhof commented Jun 20, 2018

@mhdawson the documentation hasn't changed at all, and neither has the test. Only the class TsFn, which replaces the struct napi_threadsafe_function__ from before the rewrite has really changed. So, if you concentrate on the changes in node_api.cc you would be restricting your review to the relevant parts.

@gabrielschulhof

This comment has been minimized.

Contributor

gabrielschulhof commented Jun 29, 2018

@mhdawson could you please take another look when you get a chance?

@mhdawson

Still looks good to me.

@gabrielschulhof

This comment has been minimized.

@gabrielschulhof

This comment has been minimized.

Contributor

gabrielschulhof commented Jun 29, 2018

Landed in 81f06ba.

gabrielschulhof added a commit that referenced this pull request Jun 29, 2018

n-api: add API for asynchronous functions
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>

cjihrig added a commit to cjihrig/node-1 that referenced this pull request Jun 29, 2018

n-api: fix compiler warning
private field 'async_context' is not used [-Wunused-private-field]

Refs: nodejs#17887

@cjihrig cjihrig referenced this pull request Jun 29, 2018

Merged

n-api: fix compiler warning #21597

2 of 2 tasks complete

targos added a commit that referenced this pull request Jun 30, 2018

n-api: add API for asynchronous functions
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>

cjihrig added a commit to cjihrig/node-1 that referenced this pull request Jul 2, 2018

n-api: fix compiler warning
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>

targos added a commit that referenced this pull request Jul 3, 2018

n-api: fix compiler warning
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>

targos added a commit that referenced this pull request Jul 3, 2018

2018-07-04, Version 10.6.0 (Current)
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)

@targos targos referenced this pull request Jul 3, 2018

Merged

v10.6.0 proposal #21629

targos added a commit that referenced this pull request Jul 3, 2018

2018-07-04, Version 10.6.0 (Current)
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

@gabrielschulhof gabrielschulhof deleted the gabrielschulhof:napi-threadsafe-function-call-js branch Jul 3, 2018

targos added a commit that referenced this pull request Jul 4, 2018

2018-07-04, Version 10.6.0 (Current)
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

targos added a commit that referenced this pull request Jul 4, 2018

2018-07-04, Version 10.6.0 (Current)
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment