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

n-api: add API for asynchronous functions #17809

Closed

Conversation

gabrielschulhof
Copy link
Contributor

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

@nodejs-github-bot 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 gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Dec 21, 2017
doc/api/n-api.md Outdated
-->
```C
NAPI_EXTERN napi_status
napi_create_async_function(napi_env env,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell napi_async_call?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

napi_threadsafe_function?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

threadsafe works for me :)

doc/api/n-api.md Outdated
- `[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`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

doc/api/n-api.md Outdated

### napi_call_async_function
<!-- YAML
added: UPDATEME
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aaaah! Forgot the magic word :)

@gabrielschulhof
Copy link
Contributor Author

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

@gabrielschulhof
Copy link
Contributor Author

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

@gabrielschulhof
Copy link
Contributor Author

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

@gabrielschulhof
Copy link
Contributor Author

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

doc/api/n-api.md Outdated
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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

#### napi_threadsafe_function_marshal
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

doc/api/n-api.md Outdated
-->
```C
NAPI_EXTERN napi_status napi_get_threadsafe_function_data(napi_threadsafe_function func,
void** data,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you line up the function arguments.

src/node_api.cc Outdated
const char* message) {
napi_value result, js_message;
if (napi_create_string_utf8(env, message, NAPI_AUTO_LENGTH, &js_message)
== napi_ok) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/node_api.cc Outdated
}
}

napi_fatal_error("N-API async function", NAPI_AUTO_LENGTH,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should async be threadsafe here?

doc/api/n-api.md Outdated
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stray line.

doc/api/n-api.md Outdated

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`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/node_api.cc Outdated

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use ContainerOf().

src/node_api.cc Outdated
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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/node_api.cc Outdated

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

goto throw;
}

TestAsyncData* async_data = malloc(sizeof(TestAsyncData));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sizeof(*async_data)

goto delete_async_function;
}

if (uv_thread_create((uv_thread_t*)async_data, BasicTestThread,
Copy link
Member

@bnoordhuis bnoordhuis Dec 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{"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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arraysize(props)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise.

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
Copy link
Contributor Author

Closing this in favour of #17887.

@gabrielschulhof gabrielschulhof deleted the napi-async-function branch June 6, 2018 02:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

N-API: no ability to schedule work on different thread? (via uv_async_t)
6 participants