src,napi: add helper for addons to get the event loop by addaleax · Pull Request #17109 · nodejs/node

addaleax · 2017-11-17T23:35:49Z

C++ variant

Add a utility functions for addons to use when they need
a reference to the current event loop.

Currently, uv_default_loop() works if the embedder is the
single-threaded default node executable, but even without
the presence of e.g. workers that might not really an API
guarantee for when Node is embedded.

N-API variant

Add a utility functions for addons to use when they need
a reference to the current event loop.

While the libuv API is not directly part of N-API, it
provides a quite stable C API as well, and is tightly
integrated with Node itself.

As a particular use case, without access to the event loop
it is hard to do something interesting from inside a N-API
finalizer function, since calls into JS and therefore virtually
all other N-API functions are not allowed.

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)

node.h, n-api

/cc @nodejs/n-api

Add a utility functions for addons to use when they need a reference to the current event loop. Currently, `uv_default_loop()` works if the embedder is the single-threaded default node executable, but even without the presence of e.g. workers that might not really an API guarantee for when Node is embedded.

Add a utility functions for addons to use when they need a reference to the current event loop. While the libuv API is not directly part of N-API, it provides a quite stable C API as well, and is tightly integrated with Node itself. As a particular use case, without access to the event loop it is hard to do something interesting from inside a N-API finalizer function, since calls into JS and therefore virtually all other N-API functions are not allowed.

addaleax · 2017-11-17T23:42:50Z

CI: https://ci.nodejs.org/job/node-test-commit/14111/

bnoordhuis · 2017-11-18T10:27:32Z

src/node.cc



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


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.

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

bnoordhuis · 2017-11-18T10:47:48Z

src/node_api.cc

+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);


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

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

bnoordhuis · 2017-11-18T10:49:23Z

test/addons-napi/test_uv_loop/test_uv_loop.cc

+#include <assert.h>
+#include "../common.h"
+
+template<typename T>


Space before <.

bnoordhuis · 2017-11-18T10:51:37Z

test/addons-napi/test_uv_loop/test_uv_loop.cc

+  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);


Cast-to-void is not necessary.

bnoordhuis · 2017-11-18T10:52:55Z

test/addons-napi/test_uv_loop/test_uv_loop.cc

+  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);


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

bnoordhuis · 2017-11-18T11:01:56Z

test/addons-napi/test_uv_loop/test_uv_loop.cc

+  NAPI_CALL(env,
+    napi_create_reference(env, argv[0], 1, &cbref));
+
+  SetImmediate(env, [=]() -> char* {


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?

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 NULL → 0 seems odd.

addaleax · 2017-11-18T13:02:58Z

Fresh CI: https://ci.nodejs.org/job/node-test-commit/14124/

cjihrig · 2017-11-20T16:12:43Z

doc/api/n-api.md

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


This should be napi_get_uv_event_loop I think.

@cjihrig Thanks, fixed!

addaleax · 2017-11-21T07:42:55Z

Landed in c0f3bc2, 9cf3525

Add a utility functions for addons to use when they need a reference to the current event loop. Currently, `uv_default_loop()` works if the embedder is the single-threaded default node executable, but even without the presence of e.g. workers that might not really an API guarantee for when Node is embedded. PR-URL: #17109 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Add a utility functions for addons to use when they need a reference to the current event loop. While the libuv API is not directly part of N-API, it provides a quite stable C API as well, and is tightly integrated with Node itself. As a particular use case, without access to the event loop it is hard to do something interesting from inside a N-API finalizer function, since calls into JS and therefore virtually all other N-API functions are not allowed. PR-URL: #17109 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Add a utility functions for addons to use when they need a reference to the current event loop. Currently, `uv_default_loop()` works if the embedder is the single-threaded default node executable, but even without the presence of e.g. workers that might not really an API guarantee for when Node is embedded. PR-URL: #17109 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Add a utility functions for addons to use when they need a reference to the current event loop. While the libuv API is not directly part of N-API, it provides a quite stable C API as well, and is tightly integrated with Node itself. As a particular use case, without access to the event loop it is hard to do something interesting from inside a N-API finalizer function, since calls into JS and therefore virtually all other N-API functions are not allowed. PR-URL: #17109 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Notable changes: * async\_hooks: - add trace events to async_hooks (Andreas Madsen) #15538 - add provider types for net server (Andreas Madsen) #17157 * console: - console.debug can now be used outside of the inspector (Benjamin Zaslavsky) #17033 * deps: - upgrade libuv to 1.18.0 (cjihrig) #17282 - patch V8 to 6.2.414.46 (Myles Borins) #17206 * module: - module.builtinModules will return a list of built in modules (Jon Moss) #16386 * n-api: - add helper for addons to get the event loop (Anna Henningsen) #17109 * process: - process.setUncaughtExceptionCaptureCallback can now be used to customize behavior for `--abort-on-uncaught-exception` (Anna Henningsen) #17159 - A signal handler is now able to receive the signal code that triggered the handler. (Robert Rossmann) #15606 * src: - embedders can now use Node::CreatePlatform to create an instance of NodePlatform (Cheng Zhao) #16981 * stream: - writable.writableHighWaterMark and readable.readableHighWaterMark will return the values the stream object was instantiated with (Calvin Metcalf) #12860 * **Added new collaborators** * [maclover7](https://github.com/maclover7) Jon Moss * [guybedford](https://github.com/guybedford) Guy Bedford * [hashseed](https://github.com/hashseed) Yang Guo PR-URL: Coming Soon

Notable changes: * async\_hooks: - add trace events to async_hooks (Andreas Madsen) #15538 - add provider types for net server (Andreas Madsen) #17157 * console: - console.debug can now be used outside of the inspector (Benjamin Zaslavsky) #17033 * deps: - upgrade libuv to 1.18.0 (cjihrig) #17282 - patch V8 to 6.2.414.46 (Myles Borins) #17206 * module: - module.builtinModules will return a list of built in modules (Jon Moss) #16386 * n-api: - add helper for addons to get the event loop (Anna Henningsen) #17109 * process: - process.setUncaughtExceptionCaptureCallback can now be used to customize behavior for `--abort-on-uncaught-exception` (Anna Henningsen) #17159 - A signal handler is now able to receive the signal code that triggered the handler. (Robert Rossmann) #15606 * src: - embedders can now use Node::CreatePlatform to create an instance of NodePlatform (Cheng Zhao) #16981 * stream: - writable.writableHighWaterMark and readable.readableHighWaterMark will return the values the stream object was instantiated with (Calvin Metcalf) #12860 * **Added new collaborators** * [maclover7](https://github.com/maclover7) Jon Moss * [guybedford](https://github.com/guybedford) Guy Bedford * [hashseed](https://github.com/hashseed) Yang Guo PR-URL: #17631

Add a utility functions for addons to use when they need a reference to the current event loop. Currently, `uv_default_loop()` works if the embedder is the single-threaded default node executable, but even without the presence of e.g. workers that might not really an API guarantee for when Node is embedded. PR-URL: #17109 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Add a utility functions for addons to use when they need a reference to the current event loop. While the libuv API is not directly part of N-API, it provides a quite stable C API as well, and is tightly integrated with Node itself. As a particular use case, without access to the event loop it is hard to do something interesting from inside a N-API finalizer function, since calls into JS and therefore virtually all other N-API functions are not allowed. PR-URL: #17109 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Notable changes: * deps: * update V8 to 6.2.414.46 (Michaël Zasso) [#16413](#16413) * revert ABI breaking changes in V8 6.2 (Anna Henningsen) [#16413](#16413) * upgrade libuv to 1.19.1 (cjihrig) [#18260](#18260) * re land npm 5.6.0 (Myles Borins) [#18625](#18625) * ICU 60 bump (Steven R. Loomis) [#16876](#16876) * crypto: * Support both OpenSSL 1.1.0 and 1.0.2 (David Benjamin) [#16130](#16130) * warn on invalid authentication tag length (Tobias Nießen) [#17566](#17566) * async_hooks: * update defaultTriggerAsyncIdScope for perf (Anatoli Papirovski) [#18004](#18004) * use typed array stack as fast path (Anna Henningsen) [#17780](#17780) * use scope for defaultTriggerAsyncId (Andreas Madsen) [#17273](#17273) * separate missing from default context (Andreas Madsen) [#17273](#17273) * rename initTriggerId (Andreas Madsen) [#17273](#17273) * deprecate undocumented API (Andreas Madsen) [#16972](#16972) * add destroy event for gced AsyncResources (Sebastian Mayr) [#16998](#16998) * add trace events to async_hooks (Andreas Madsen) [#15538](#15538) * set HTTPParser trigger to socket (Andreas Madsen) [#18003](#18003) * add provider types for net server (Andreas Madsen) [#17157](#17157) * n-api: * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109) * cli: * add --stack-trace-limit to NODE_OPTIONS (Anna Henningsen) [#16495](#16495) * console: * add support for console.debug (Benjamin Zaslavsky) [#17033](#17033) * module: * add builtinModules (Jon Moss) [#16386](#16386) * replace default paths in require.resolve() (cjihrig) [#17113](#17113) * src: * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109) * add process.ppid (cjihrig) [#16839](#16839) * http: * support generic `Duplex` streams (Anna Henningsen) [#16267](#16267) * add rawPacket in err of `clientError` event (XadillaX) [#17672](#17672) * better support for IPv6 addresses (Mattias Holmlund) [#14772](#14772) * net: * remove ADDRCONFIG DNS hint on Windows (Bartosz Sosnowski) [#17662](#17662) * process: * fix reading zero-length env vars on win32 (Anna Henningsen) [#18463](#18463) * tls: * unconsume stream on destroy (Anna Henningsen) [#17478](#17478) * process: * improve unhandled rejection message (Madara Uchiha) [#17158](#17158) * stream: * remove usage of *State.highWaterMark (Calvin Metcalf) [#12860](#12860) * trace_events: * add executionAsyncId to init events (Andreas Madsen) [#17196](#17196) PR-URL: #18336

Add a utility functions for addons to use when they need a reference to the current event loop. While the libuv API is not directly part of N-API, it provides a quite stable C API as well, and is tightly integrated with Node itself. As a particular use case, without access to the event loop it is hard to do something interesting from inside a N-API finalizer function, since calls into JS and therefore virtually all other N-API functions are not allowed. PR-URL: nodejs#17109 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Add a utility functions for addons to use when they need a reference to the current event loop. While the libuv API is not directly part of N-API, it provides a quite stable C API as well, and is tightly integrated with Node itself. As a particular use case, without access to the event loop it is hard to do something interesting from inside a N-API finalizer function, since calls into JS and therefore virtually all other N-API functions are not allowed. Backport-PR-URL: #19447 PR-URL: #17109 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Notable changes: * deps: * update V8 to 6.2.414.46 (Michaël Zasso) [nodejs#16413](nodejs#16413) * revert ABI breaking changes in V8 6.2 (Anna Henningsen) [nodejs#16413](nodejs#16413) * upgrade libuv to 1.19.1 (cjihrig) [nodejs#18260](nodejs#18260) * re land npm 5.6.0 (Myles Borins) [nodejs#18625](nodejs#18625) * ICU 60 bump (Steven R. Loomis) [nodejs#16876](nodejs#16876) * crypto: * Support both OpenSSL 1.1.0 and 1.0.2 (David Benjamin) [nodejs#16130](nodejs#16130) * warn on invalid authentication tag length (Tobias Nießen) [nodejs#17566](nodejs#17566) * async_hooks: * update defaultTriggerAsyncIdScope for perf (Anatoli Papirovski) [nodejs#18004](nodejs#18004) * use typed array stack as fast path (Anna Henningsen) [nodejs#17780](nodejs#17780) * use scope for defaultTriggerAsyncId (Andreas Madsen) [nodejs#17273](nodejs#17273) * separate missing from default context (Andreas Madsen) [nodejs#17273](nodejs#17273) * rename initTriggerId (Andreas Madsen) [nodejs#17273](nodejs#17273) * deprecate undocumented API (Andreas Madsen) [nodejs#16972](nodejs#16972) * add destroy event for gced AsyncResources (Sebastian Mayr) [nodejs#16998](nodejs#16998) * add trace events to async_hooks (Andreas Madsen) [nodejs#15538](nodejs#15538) * set HTTPParser trigger to socket (Andreas Madsen) [nodejs#18003](nodejs#18003) * add provider types for net server (Andreas Madsen) [nodejs#17157](nodejs#17157) * n-api: * add helper for addons to get the event loop (Anna Henningsen) [nodejs#17109](nodejs#17109) * cli: * add --stack-trace-limit to NODE_OPTIONS (Anna Henningsen) [nodejs#16495](nodejs#16495) * console: * add support for console.debug (Benjamin Zaslavsky) [nodejs#17033](nodejs#17033) * module: * add builtinModules (Jon Moss) [nodejs#16386](nodejs#16386) * replace default paths in require.resolve() (cjihrig) [nodejs#17113](nodejs#17113) * src: * add helper for addons to get the event loop (Anna Henningsen) [nodejs#17109](nodejs#17109) * add process.ppid (cjihrig) [nodejs#16839](nodejs#16839) * http: * support generic `Duplex` streams (Anna Henningsen) [nodejs#16267](nodejs#16267) * add rawPacket in err of `clientError` event (XadillaX) [nodejs#17672](nodejs#17672) * better support for IPv6 addresses (Mattias Holmlund) [nodejs#14772](nodejs#14772) * net: * remove ADDRCONFIG DNS hint on Windows (Bartosz Sosnowski) [nodejs#17662](nodejs#17662) * process: * fix reading zero-length env vars on win32 (Anna Henningsen) [nodejs#18463](nodejs#18463) * tls: * unconsume stream on destroy (Anna Henningsen) [nodejs#17478](nodejs#17478) * process: * improve unhandled rejection message (Madara Uchiha) [nodejs#17158](nodejs#17158) * stream: * remove usage of *State.highWaterMark (Calvin Metcalf) [nodejs#12860](nodejs#12860) * trace_events: * add executionAsyncId to init events (Andreas Madsen) [nodejs#17196](nodejs#17196) PR-URL: nodejs#18336

jinfeihan57 · 2022-08-10T04:16:21Z

@addaleax Is napi_get_uv_event_loop() thread safe?

addaleax · 2022-08-10T10:36:48Z

@jinfeihan57 No. None of the Node-API methods that aren’t explicitly designed for usage with multithreading are thread-safe.

jinfeihan57 · 2022-08-11T07:32:23Z

@jinfeihan57 No. None of the Node-API methods that aren’t explicitly designed for usage with multithreading are thread-safe.

That means I can't call it from another thread, Am I right?
uv loop is a global resource Can you make it thread safe?

addaleax added 2 commits November 18, 2017 00:28

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 Nov 17, 2017

bnoordhuis reviewed Nov 18, 2017

View reviewed changes

[squash] bnoordhuis comments

421f9d1

bnoordhuis approved these changes Nov 18, 2017

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 19, 2017

cjihrig approved these changes Nov 20, 2017

View reviewed changes

[squash] cjihrig comment

6812449

addaleax closed this Nov 21, 2017

addaleax deleted the get-current-loop branch November 21, 2017 07:43

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 28, 2017

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

gibfahn added dont-land-on-v6.x labels Jan 15, 2018

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged



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

Uh oh!

Conversation

addaleax commented Nov 17, 2017

C++ variant

N-API variant

Checklist

Affected core subsystem(s)

Uh oh!

addaleax commented Nov 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Nov 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Nov 21, 2017

Uh oh!

jinfeihan57 commented Aug 10, 2022

Uh oh!

addaleax commented Aug 10, 2022

Uh oh!

jinfeihan57 commented Aug 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants