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

fix: init after __wasm_call_ctors #18

Merged
merged 1 commit into from
Jan 6, 2023
Merged

fix: init after __wasm_call_ctors #18

merged 1 commit into from
Jan 6, 2023

Conversation

toyobayashi
Copy link
Owner

Fixes #15

@toyobayashi toyobayashi merged commit a7cc5c5 into main Jan 6, 2023
@@ -179,7 +179,7 @@ mergeInto(LibraryManager.library, {
return emnapiExports
}

addOnInit(function (Module) {
__ATINIT__.push(function (Module) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be easier to do all this init from C/C++ rather than by dealing with JS helpers?

For example, this callback could be just another __attribute__((constructor)) instead of adding it as a separate init from JS, and then all those surrounding calls would probably be simpler & smaller when express via napi.h too.

Copy link
Owner Author

Choose a reason for hiding this comment

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

This callback will not be called until addRunDependency("emnapi-init") is removed, it's asynchronous

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm why? Is there anything in actual N-API that is asynchronous?

Copy link
Owner Author

Choose a reason for hiding this comment

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

$emnapiInit__postset: 'addOnPreRun(function (Module) {' +
'addRunDependency("emnapi-init");' +
'emnapiRuntimeInit(Module).then(function () {' +
'emnapiInit();' +
'removeRunDependency("emnapi-init");' +
'}, function (err) {' +
'if (typeof Module.onEmnapiInitialized === "function") {' +
'Module.onEmnapiInitialized(err);' +
'} else {' +
'throw err;' +
'}' +
'});' +
'});',

$emnapiRuntimeInit: function emnapiRuntimeInit (Module: any) {

emnapiRuntimeInit (async) then emnapiInit (__ATINIT__.push())

Copy link
Owner Author

Choose a reason for hiding this comment

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

emnapiInit depends emnapiRt and emnapiCtx, which is asynchronously initialized in emnapiRuntimeInit

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, but then, C/C++, including __attribute__((constructor)), also won't be called until addRunDependency is removed, so the effect should be the same?

Copy link
Contributor

Choose a reason for hiding this comment

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

If not, I probably will need to look a bit deeper later.

Copy link
Owner Author

Choose a reason for hiding this comment

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

But we always need to create Env and export emnapiExport on JavaScript side, current implementation can easily use closure to save variables. (I found out Node.js official node_api.h uses __attribute__((constructor)) only on native platforms, but export napi_register_wasm_v1 on wasm, during initialization the host environment should pass env and export to it.)

https://github.com/nodejs/node/blob/513c15147cb44d2d27073e520820146185c45638/src/node_api.h#L76-L107

@toyobayashi toyobayashi deleted the fix-init branch April 6, 2023 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Initialization order regression
2 participants