src: define `NAPI_HAS_THREADS` to make TSFN available on Emscripten #1283

toyobayashi · 2023-02-08T14:46:46Z

Hi, Node-API team.

I created emnapi, that is a Node-API implementation for Emscripten/wasi-sdk/wasm32. This project aims to help users port their or existing Node-API native addons to wasm, and run it on browser! It is well tested by using Node.js official test cases. Recently napi-rs is also plan to integrate emnapi so that napi-rs can target wasm as well. See real world use cases and discussions here.

Currently emnapi shipped a modified copy of node-addon-api in @tybys/emnapi npm package, due to the original source disables threadsafe function API when building for wasm, though tsfn can work well by relying on emscripten pthreads. And CallbackScope can not be implemented, neither on browser nor on Node.js. Needless to say in the browser environment, even on Node.js, the reason is that native node::CallbackScope* can not be returned to JavaScript.

This PR defines NAPI_HAS_THREADS to make TSFN available on Emscripten, and disable CallbackScope in wasm. Thus emnapi no longer need to maintain a copy, users can just do npm install node-addon-api to work with emnapi.

toyobayashi · 2023-02-08T16:53:15Z

Sorry, I have no idea why npm run lint not work on my local machine. Manually do Alt + Shift + F use the clang-format in vscode, request run CI again.

legendecas

Hey Toyo, thanks for the work!

I'm wondering if classes like AsyncWorker should be scoped with NAPI_HAS_THREADS too?

toyobayashi · 2023-02-08T17:28:41Z

Because emnapi napi_*_async_work implementation relies on pthread too. I have ported libuv's threadpool to emnapi.

legendecas · 2023-02-10T07:08:33Z

I'm still curious about the reason that CallbackScope can not be implemented for missing JavaScript equivalent representatives. IMO, classes like HandleScope, AsyncWorker, and Reference are native structures, not JavaScript values.

On the other hand, CallbackScope and TSFN themselves don't create any threads or require threaded works. Instead, AsyncWorker and its subclasses are executed in the thread pool. Would it be more reasonable to only scope AsyncWorker in NAPI_HAS_THREADS instead?

toyobayashi · 2023-02-10T07:30:54Z

I'm still curious about the reason that CallbackScope can not be implemented for missing JavaScript equivalent representatives.

Imagine this, you call napi_open_callback_scope() in wasm, then wasm need to tell the host (Node.js) open a real node::CallbackScope or call real native napi_open_callback_scope, then return the pointer address to wasm memory to save.

wasm napi_open_callback_scope() -> Node.js JavaScript -> call napi_open_callback_scope in native node binding 

-> return pointer -> JavaScript -> set the pointer address to wasm memory

However, both of them failed, Node.js doesn't allow leaving an opening CallbackScope before returning to JavaScript from C++, it just crashed, saying something like callback scope mismatch. In addition, when native callback scope destructing, the pending process.nextTick() will be called before returning back to JavaScript, and there is no such async context in browser, so it can be not implemented on browser.

classes like HandleScope, AsyncWorker, and Reference are native structures, not JavaScript values

emnapi's napi_value / napi_reference / napi_handle_scope is not the native, they are implemented in JavaScript, just JavaScript numbers.

On the other hand, CallbackScope and TSFN themselves don't create any threads or require threaded works. Instead, AsyncWorker and its subclasses are executed in the thread pool. Would it be more reasonable to only scope AsyncWorker in NAPI_HAS_THREADS instead?

I understand your point, but without threads, TSFN and AsyncWorker never work, they are broken. TSFN also rely on pthread mutex. Currently, if no pthread support, napi_create_async_work and such APIs in emnapi will always return napi_generic_failure. Scope them in NAPI_HAS_THREADS can warn user at compile time.

toyobayashi · 2023-02-10T07:45:55Z

And this PR doesn't affect native platforms, NAPI_HAS_THREADS is always 1 in native.

legendecas · 2023-02-13T07:59:12Z

Imagine this, you call napi_open_callback_scope() in wasm, then wasm need to tell the host (Node.js) open a real node::CallbackScope or call real native napi_open_callback_scope, then return the pointer address to wasm memory to save.

Thanks for the explanation! However, HandleScope has the same limitation as CallbackScope: https://github.com/nodejs/node/blob/main/src/js_native_api_v8.h#L95-L96. How did you circumvent the issue for HandleScope, as it is essential for managing javascript value handles?

I think CallbackScope can be used without threads. It is essential for any async callbacks to build proper async context tracking. So I'd find it would be great to ensure it is available unconditionally too.

toyobayashi · 2023-02-13T08:15:46Z

How did you circumvent the issue for HandleScope, as it is essential for managing javascript value handles?

As I explained above,

emnapi's napi_value / napi_reference / napi_handle_scope is not the native, they are implemented in JavaScript, just JavaScript numbers.

HandleScope in emnapi is completely implemented in JavaScript, so is napi_value / napi_reference / napi_deferred, they are independent of native Node.js runtime, all these things are available on browser!

I think CallbackScope can be used without threads.

Yeah, I'm scoping Napi::CallbackScope in !defined(__wasm__), not NAPI_HAS_THREADS.

node-addon-api/napi.h

Line 2408 in 69300c3

#if (NAPI_VERSION > 2 && !defined(__wasm__))

legendecas · 2023-02-13T08:18:35Z

HandleScope in emnapi is completely implemented in JavaScript, so is napi_value / napi_reference / napi_deferred, they are independent of native Node.js runtime, all these things are available on browser!
Yeah, I'm scoping Napi::CallbackInfo in !wasm, not NAPI_HAS_THREADS.

In this case, I'd find it is an implementation detail of emnapi.

legendecas · 2023-02-13T08:22:03Z

~~napi_async_init/napi_async_destroy and napi_open_callback_scope/napi_close_callback_scope should be able to be implemented with JS API AsyncResource.~~

Edit: probably there is a problem with asyncResource.runInAsyncScope. Sorry.

toyobayashi · 2023-02-13T08:34:28Z

Now I have removed !defined(__wasm__) on CallbackScope.

napi_async_init, napi_async_destroy and napi_make_callback have been implemented via node binding as a bridge.

https://github.com/toyobayashi/emnapi/blob/main/packages/emnapi/src/node.ts

napi-inl.h

napi.h

toyobayashi · 2023-02-13T08:56:15Z

In this case, I'd find it is an implementation detail of emnapi.

CallbackScope cannot be implemented in wasm, this thing itself is not strongly related to emnapi, so I personally prefer to put it in !defined(__wasm__). Or maybe there is probability to implement this in the future if Node.js add some ability to workaround?

legendecas · 2023-02-13T09:01:17Z

Yeah, I'm thinking of providing a built-in wasm node-api module in Node.js since we already allowed declaring a wasm addon in node_api.h. I'll create a tracking issue for this.

Previous discussion: nodejs/abi-stable-node#375

toyobayashi · 2023-02-13T09:49:10Z

By the way, would it be possible to consider mentioning emnapi in the official documentation Node-API chapter? I have confidence to say now emnapi is the only project which has so complete Node-API implementation for wasm, official test cases guarantees that the runtime behavior on browser is almost exactly the same as on native Node.js. Implementation detail also follows official source code as far as possible. I believe this project can make contribution to community :)

legendecas · 2023-02-13T09:50:23Z

Please feel free to submit a PR on https://github.com/nodejs/abi-stable-node/blob/doc/node-api-engine-bindings.md!

PR-URL: #46633 Refs: #33597 Refs: nodejs/node-addon-api#1283 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com>

mhdawson

LGTM

PR-URL: #1283 Reviewed-By: Michael Dawson <midawson@redhat.com Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

mhdawson · 2023-03-03T15:42:28Z

Landed in 0b53d88

PR-URL: #46633 Refs: #33597 Refs: nodejs/node-addon-api#1283 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs/node-addon-api#1283 Reviewed-By: Michael Dawson <midawson@redhat.com Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

toyobayashi added 2 commits February 8, 2023 22:09

src: define NAPI_HAS_THREADS

92d27b8

src: run clang-format

106abfa

legendecas reviewed Feb 8, 2023

View reviewed changes

mhdawson mentioned this pull request Feb 10, 2023

What's new in the Node-API and node-addon-api world? - 2023 version nodejs/abi-stable-node#446

Closed

src: scope <mutex> in NAPI_HAS_THREADS

69300c3

toyobayashi added a commit to toyobayashi/emnapi that referenced this pull request Feb 11, 2023

update naa to 6.0.0 and nodejs/node-addon-api#1283

869adfe

src: do not scope

fd8e9f7

legendecas mentioned this pull request Feb 13, 2023

node-api: add __wasm32__ guards on async works nodejs/node#46633

Merged

legendecas approved these changes Feb 13, 2023

View reviewed changes

legendecas reviewed Feb 13, 2023

View reviewed changes

napi-inl.h Outdated Show resolved Hide resolved

legendecas reviewed Feb 13, 2023

View reviewed changes

napi.h Outdated Show resolved Hide resolved

src: add comment after endif

f5ad5ac

toyobayashi mentioned this pull request Feb 13, 2023

add emnapi to node-api-engine-bindings.md nodejs/abi-stable-node#447

Merged

toyobayashi added a commit to toyobayashi/emnapi that referenced this pull request Feb 13, 2023

define callback scope (nodejs/node-addon-api#1283)

c25d7a1

toyobayashi mentioned this pull request Feb 13, 2023

add napi_{open,close}_callback_scope even if they are unavailable toyobayashi/emnapi#32

Merged

mhdawson approved these changes Mar 2, 2023

View reviewed changes

mhdawson pushed a commit that referenced this pull request Mar 3, 2023

src: define NAPI_HAS_THREADS

0b53d88

PR-URL: #1283 Reviewed-By: Michael Dawson <midawson@redhat.com Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

mhdawson closed this Mar 3, 2023

mhdawson mentioned this pull request Mar 3, 2023

Next release #1294

Closed

toyobayashi mentioned this pull request Apr 21, 2023

WebAssembly build lovell/sharp#3522

Closed

toyobayashi mentioned this pull request Aug 6, 2023

node-api: update headers for better wasm support nodejs/node#49037

Merged

johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023

src: define NAPI_HAS_THREADS

85d34aa

PR-URL: nodejs/node-addon-api#1283 Reviewed-By: Michael Dawson <midawson@redhat.com Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: define `NAPI_HAS_THREADS` to make TSFN available on Emscripten #1283

src: define `NAPI_HAS_THREADS` to make TSFN available on Emscripten #1283

toyobayashi commented Feb 8, 2023 •

edited

Loading

toyobayashi commented Feb 8, 2023

legendecas left a comment

toyobayashi commented Feb 8, 2023

legendecas commented Feb 10, 2023

toyobayashi commented Feb 10, 2023 •

edited

Loading

toyobayashi commented Feb 10, 2023

legendecas commented Feb 13, 2023 •

edited

Loading

toyobayashi commented Feb 13, 2023 •

edited

Loading

legendecas commented Feb 13, 2023 •

edited

Loading

legendecas commented Feb 13, 2023 •

edited

Loading

toyobayashi commented Feb 13, 2023

toyobayashi commented Feb 13, 2023

legendecas commented Feb 13, 2023 •

edited

Loading

toyobayashi commented Feb 13, 2023

legendecas commented Feb 13, 2023

mhdawson left a comment

mhdawson commented Mar 3, 2023

src: define NAPI_HAS_THREADS to make TSFN available on Emscripten #1283

src: define NAPI_HAS_THREADS to make TSFN available on Emscripten #1283

Conversation

toyobayashi commented Feb 8, 2023 • edited Loading

toyobayashi commented Feb 8, 2023

legendecas left a comment

Choose a reason for hiding this comment

toyobayashi commented Feb 8, 2023

legendecas commented Feb 10, 2023

toyobayashi commented Feb 10, 2023 • edited Loading

toyobayashi commented Feb 10, 2023

legendecas commented Feb 13, 2023 • edited Loading

toyobayashi commented Feb 13, 2023 • edited Loading

legendecas commented Feb 13, 2023 • edited Loading

legendecas commented Feb 13, 2023 • edited Loading

toyobayashi commented Feb 13, 2023

toyobayashi commented Feb 13, 2023

legendecas commented Feb 13, 2023 • edited Loading

toyobayashi commented Feb 13, 2023

legendecas commented Feb 13, 2023

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Mar 3, 2023

src: define `NAPI_HAS_THREADS` to make TSFN available on Emscripten #1283

src: define `NAPI_HAS_THREADS` to make TSFN available on Emscripten #1283

toyobayashi commented Feb 8, 2023 •

edited

Loading

toyobayashi commented Feb 10, 2023 •

edited

Loading

legendecas commented Feb 13, 2023 •

edited

Loading

toyobayashi commented Feb 13, 2023 •

edited

Loading

legendecas commented Feb 13, 2023 •

edited

Loading

legendecas commented Feb 13, 2023 •

edited

Loading

legendecas commented Feb 13, 2023 •

edited

Loading