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

src,napi: add helper for addons to get the event loop #17109

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions doc/api/n-api.md
Original file line number Diff line number Diff line change
Expand Up @@ -3684,6 +3684,23 @@ NAPI_EXTERN napi_status napi_run_script(napi_env env,
- `[in] script`: A JavaScript string containing the script to execute.
- `[out] result`: The value resulting from having executed the script.

## libuv event loop

N-API provides a function for getting the current event loop associated with
a specific `napi_env`.

### napi_uv_get_event_loop
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be napi_get_uv_event_loop I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

@cjihrig Thanks, fixed!

<!-- YAML
added: REPLACEME
-->
```C
NAPI_EXTERN napi_status napi_get_uv_event_loop(napi_env env,
uv_loop_t** loop);
```

- `[in] env`: The environment that the API is invoked under.
- `[out] loop`: The current libuv loop instance.

[Promises]: #n_api_promises
[Simple Asynchronous Operations]: #n_api_simple_asynchronous_operations
[Custom Asynchronous Operations]: #n_api_custom_asynchronous_operations
Expand Down
5 changes: 5 additions & 0 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4419,6 +4419,11 @@ void RunAtExit(Environment* env) {
}


uv_loop_t* GetCurrentEventLoop(v8::Isolate* isolate) {
return Environment::GetCurrent(isolate)->event_loop();
Copy link
Member

Choose a reason for hiding this comment

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

Leaks a handle into the caller's scope.

The overload that takes a v8::Isolate* calls Environment::GetCurrent(isolate->GetCurrentContext()) but that won't work when there is no current context or it doesn't have an associated Environment (i.e, non-node context.)

That should be documented because it's the kind of thing that add-ons will run into. You could robustify (def a word) this function by doing:

uv_loop_t* GetCurrentEventLoop(v8::Isolate* isolate) {
  HandleScope handle_scope(isolate);
  auto context = isolate->GetCurrentContext();
  if (context.IsEmpty()) return nullptr;
  return Environment::GetCurrent(context);
}

That can still crash with a non-node context, though. Maybe we should file a CL with V8 to expose
v8::Context::SlowGetAlignedPointerFromEmbedderData() in order to safely retrieve the Environment pointer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, thanks for catching those. Updated with your suggestion!

}


static uv_key_t thread_local_env;


Expand Down
2 changes: 2 additions & 0 deletions src/node.h
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,8 @@ NODE_EXTERN void EmitBeforeExit(Environment* env);
NODE_EXTERN int EmitExit(Environment* env);
NODE_EXTERN void RunAtExit(Environment* env);

NODE_EXTERN struct uv_loop_s* GetCurrentEventLoop(v8::Isolate* isolate);

/* Converts a unixtime to V8 Date */
#define NODE_UNIXTIME_V8(t) v8::Date::New(v8::Isolate::GetCurrent(), \
1000 * static_cast<double>(t))
Expand Down
19 changes: 13 additions & 6 deletions src/node_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
#include "node_api.h"
#include "node_internals.h"

#define NAPI_VERSION 1
#define NAPI_VERSION 2

static
napi_status napi_set_last_error(napi_env env, napi_status error_code,
Expand Down Expand Up @@ -3401,15 +3401,22 @@ napi_status napi_delete_async_work(napi_env env, napi_async_work work) {
return napi_clear_last_error(env);
}

napi_status napi_get_uv_event_loop(napi_env env, uv_loop_t** loop) {
CHECK_ENV(env);
CHECK_ARG(env, loop);
*loop = node::GetCurrentEventLoop(env->isolate);
Copy link
Member

Choose a reason for hiding this comment

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

If you make that change, consider adding a sanity check here that loop != nullptr.

Copy link
Member Author

Choose a reason for hiding this comment

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

I’ve moved the GetCurrentEventLoop call to the napi_env initialization code, I think that should be fine.

return napi_clear_last_error(env);
}

napi_status napi_queue_async_work(napi_env env, napi_async_work work) {
CHECK_ENV(env);
CHECK_ARG(env, work);

// Consider: Encapsulate the uv_loop_t into an opaque pointer parameter.
// Currently the environment event loop is the same as the UV default loop.
// Someday (if node ever supports multiple isolates), it may be better to get
// the loop from node::Environment::GetCurrent(env->isolate)->event_loop();
uv_loop_t* event_loop = uv_default_loop();
napi_status status;
uv_loop_t* event_loop = nullptr;
status = napi_get_uv_event_loop(env, &event_loop);
if (status != napi_ok)
return napi_set_last_error(env, status);

uvimpl::Work* w = reinterpret_cast<uvimpl::Work*>(work);

Expand Down
6 changes: 6 additions & 0 deletions src/node_api.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
#include <stdbool.h>
#include "node_api_types.h"

struct uv_loop_s; // Forward declaration.

#ifdef _WIN32
#ifdef BUILDING_NODE_EXTENSION
#ifdef EXTERNAL_NAPI
Expand Down Expand Up @@ -581,6 +583,10 @@ NAPI_EXTERN napi_status napi_run_script(napi_env env,
napi_value script,
napi_value* result);

// Return the current libuv event loop for a given environment
NAPI_EXTERN napi_status napi_get_uv_event_loop(napi_env env,
struct uv_loop_s** loop);

EXTERN_C_END

#endif // SRC_NODE_API_H_
2 changes: 1 addition & 1 deletion test/addons-napi/test_general/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ assert.ok(test_general.testGetPrototype(baseObject) !==

// test version management functions
// expected version is currently 1
assert.strictEqual(test_general.testGetVersion(), 1);
assert.strictEqual(test_general.testGetVersion(), 2);

const [ major, minor, patch, release ] = test_general.testGetNodeVersion();
assert.strictEqual(process.version.split('-')[0],
Expand Down
8 changes: 8 additions & 0 deletions test/addons-napi/test_uv_loop/binding.gyp
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"targets": [
{
"target_name": "test_uv_loop",
"sources": [ "test_uv_loop.cc" ]
}
]
}
5 changes: 5 additions & 0 deletions test/addons-napi/test_uv_loop/test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
'use strict';
const common = require('../../common');
const { SetImmediate } = require(`./build/${common.buildType}/test_uv_loop`);

SetImmediate(common.mustCall());
79 changes: 79 additions & 0 deletions test/addons-napi/test_uv_loop/test_uv_loop.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
#include <node_api.h>
#include <uv.h>
#include <utility>
#include <memory>
#include <assert.h>
#include "../common.h"

template<typename T>
Copy link
Member

Choose a reason for hiding this comment

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

Space before <.

void* SetImmediate(napi_env env, T&& cb) {
T* ptr = new T(std::move(cb));
uv_loop_t* loop = nullptr;
uv_check_t* check = new uv_check_t;
check->data = static_cast<void*>(ptr);
Copy link
Member

Choose a reason for hiding this comment

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

Cast-to-void is not necessary.

NAPI_ASSERT(env,
napi_get_uv_event_loop(env, &loop) == napi_ok,
"can get event loop");
uv_check_init(loop, check);
uv_check_start(check, [](uv_check_t* check) {
std::unique_ptr<T> ptr {static_cast<T*>(check->data)};
T cb = std::move(*ptr);
uv_check_stop(check);
Copy link
Member

Choose a reason for hiding this comment

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

Not that it hurts but you don't have to stop the handle if you're closing it anyway.

uv_close(reinterpret_cast<uv_handle_t*>(check), [](uv_handle_t* handle) {
delete reinterpret_cast<uv_check_t*>(handle);
});

assert(cb() != nullptr);
});
return nullptr;
}

static char dummy;

napi_value SetImmediateBinding(napi_env env, napi_callback_info info) {
size_t argc = 1;
napi_value argv[1];
napi_value _this;
void* data;
NAPI_CALL(env,
napi_get_cb_info(env, info, &argc, argv, &_this, &data));
NAPI_ASSERT(env, argc >= 1, "Not enough arguments, expected 1.");

napi_valuetype t;
NAPI_CALL(env, napi_typeof(env, argv[0], &t));
NAPI_ASSERT(env, t == napi_function,
"Wrong first argument, function expected.");

napi_ref cbref;
NAPI_CALL(env,
napi_create_reference(env, argv[0], 1, &cbref));

SetImmediate(env, [=]() -> char* {
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, doesn't the compiler infer the return type? Or is this extra protection against someone changing dummy to something with non-static storage?

Copy link
Member Author

Choose a reason for hiding this comment

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

It gets confused because NAPI_CALL can return NULL, which is 0 in C++ mode, so considered an integer rather than a pointer… I guess that could be fixed by returning an integer type instead, but relying on NULL0 seems odd.

napi_value undefined;
napi_value callback;
napi_handle_scope scope;
NAPI_CALL(env, napi_open_handle_scope(env, &scope));
NAPI_CALL(env, napi_get_undefined(env, &undefined));
NAPI_CALL(env, napi_get_reference_value(env, cbref, &callback));
NAPI_CALL(env, napi_delete_reference(env, cbref));
NAPI_CALL(env,
napi_call_function(env, undefined, callback, 0, nullptr, nullptr));
NAPI_CALL(env, napi_close_handle_scope(env, scope));
return &dummy;
});

return nullptr;
}

napi_value Init(napi_env env, napi_value exports) {
napi_property_descriptor properties[] = {
DECLARE_NAPI_PROPERTY("SetImmediate", SetImmediateBinding)
};

NAPI_CALL(env, napi_define_properties(
env, exports, sizeof(properties) / sizeof(*properties), properties));

return exports;
}

NAPI_MODULE(NODE_GYP_MODULE_NAME, Init)
2 changes: 1 addition & 1 deletion test/addons/async-hello-world/binding.cc
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ void Method(const v8::FunctionCallbackInfo<v8::Value>& args) {
v8::Local<v8::Function> callback = v8::Local<v8::Function>::Cast(args[1]);
req->callback.Reset(isolate, callback);

uv_queue_work(uv_default_loop(),
uv_queue_work(node::GetCurrentEventLoop(isolate),
&req->req,
DoAsync,
(uv_after_work_cb)AfterAsync<use_makecallback>);
Expand Down
5 changes: 4 additions & 1 deletion test/addons/callback-scope/binding.cc
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,10 @@ static void TestResolveAsync(const v8::FunctionCallbackInfo<v8::Value>& args) {

uv_work_t* req = new uv_work_t;

uv_queue_work(uv_default_loop(), req, [](uv_work_t*) {}, Callback);
uv_queue_work(node::GetCurrentEventLoop(isolate),
req,
[](uv_work_t*) {},
Callback);
}

v8::Local<v8::Promise::Resolver> local =
Expand Down