Open ammarfaizi2 opened 1 year ago
Hi @ammarfaizi2 ,
We discussed this on today's Node API meeting. This is a similar issue as found on https://github.com/nodejs/node-addon-api/issues/1140 . We are waiting on some PRs in node core (https://github.com/nodejs/node/pull/42651, https://github.com/nodejs/node/pull/44141) that we believe should resolve this problem.
As discussed in the above linked issue, if you make your code asynchronous, the finalizers will have a opportunity to run in between your JavaScript execution.
This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.
It will be closed soon unless the stale label is removed or a comment is made.
Dropping a comment, just to avoid closing.
This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.
It will be closed soon unless the stale label is removed or a comment is made.
Dropping a comment, just to avoid closing.
I have come here with exactly the same issue.
I use napi_create_external
and unless I force some setTimeout
, callbacks only happen at the end.
I understand the issues in allowing finalisers to run whenever, but if one did not give access to the napi_env
, then a pure c++ finaliser could be called as soon as scheduled.
typedef void (*napi_finalize_safe)(
void* finalize_data,
void* finalize_hint);
@audetto access to the napi_env is needed for things like napi_get_instance_data
, which do not involve the engine, but make context-aware add-ons possible. We're working on making finalizer unable to run JS and therefore synchronously executable, but we must also provide an API for deferring the execution of JS code to when it becomes possible (an implementation of SetImmediate for Node-API).
I see, I did not realise there are 3 cases: run normal js, context case and no js at all. I will wait for the full solution. Thanks.
I have enabled NAPI_EXPERIMENTAL
in node 21 and my finalisers run immediately, without the need of async
.
No need to change anything in node-addon-api.
which feature did you enable through NAPI_EXPERIMENTAL
, @audetto ?
@Apidcloud , you should just be be able to define NAPI_EXPERIMENTAL
and I think it should "just work" in node 21. The default behavior with experimental mode (and soon to be default behavior under non-experimental as well) is to have synchronous, non-JS state affecting finalizers, so the finalizer can run during the regular GC cycles.
Let us know if you have any other questions/problems.
Thanks, Kevin
Things have changed since November 2023. I am using node 22.1.0 and node-addon-api 8.
When I define NAPI_EXPERIMENTAL
I get plenty of compiler errors, which I am struggling to understand.
A few months ago, this macro only changed the behaviour, now there are changes to the node api.
It is about Function wrapping.
If I dont define it, the gc still runs only at the end.
I cannot see NAPI_EXPERIMENTAL
ever used in this project, so I am not sure anybody is testing it.
Is node-addon-api
compatible with recent versions of node apis (when NAPI_EXPERIMENTAL
is defined)?
And are there any examples?
This is the error
In file included from /home/username/projects/proj-node/node_modules/node-addon-api/napi.h:3199,
from /home/username/projects/proj-node/src/main.cpp:1:
/home/username/projects/proj-node/node_modules/node-addon-api/napi-inl.h: In instantiation of ‘napi_status Napi::details::AttachData(napi_env, napi_value, FreeType*, void*) [with FreeType = CallbackData<Napi::Value (*)(const Napi::CallbackInfo&), Napi::Value>; void (* finalizer)(napi_env, void*, void*) = default_finalizer<CallbackData<Napi::Value (*)(const Napi::CallbackInfo&), Napi::Value> >; napi_env = napi_env__*; napi_value = napi_value__*]’:
/home/username/projects/proj-node/node_modules/node-addon-api/napi-inl.h:2379:39: required from ‘napi_status Napi::CreateFunction(napi_env, const char*, napi_callback, CbData*, napi_value__**) [with CbData = details::CallbackData<Value (*)(const CallbackInfo&), Value>; napi_env = napi_env__*; napi_callback = napi_value__* (*)(napi_env__*, napi_callback_info__*); napi_value = napi_value__*]’
/home/username/projects/proj-node/node_modules/node-addon-api/napi-inl.h:2436:21: required from ‘static Napi::Function Napi::Function::New(napi_env, Callable, const char*, void*) [with Callable = Napi::Value (*)(const Napi::CallbackInfo&); napi_env = napi_env__*]’
/home/username/projects/proj-node/src/main.cpp:69:24: required from here
/home/username/projects/proj-node/node_modules/node-addon-api/napi-inl.h:68:30: error: invalid conversion from ‘void (*)(napi_env, void*, void*)’ {aka ‘void (*)(napi_env__*, void*, void*)’} to ‘node_api_nogc_finalize’ {aka ‘void (*)(const napi_env__*, void*, void*)’} [-fpermissive]
68 | status = napi_add_finalizer(env, obj, data, finalizer, hint, nullptr);
| ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| void (*)(napi_env, void*, void*) {aka void (*)(napi_env__*, void*, void*)}
In file included from /home/username/.nvm/versions/node/v22.1.0/include/node/node_api.h:12,
from /home/username/projects/proj-node/node_modules/node-addon-api/napi.h:13:
/home/username/.nvm/versions/node/v22.1.0/include/node/js_native_api.h:523:43: note: initializing argument 4 of ‘napi_status napi_add_finalizer(napi_env, napi_value, void*, node_api_nogc_finalize, void*, napi_ref__**)’
523 | node_api_nogc_finalize finalize_cb,
| ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
The offending line does this
Napi::Value AFunction(const Napi::CallbackInfo &info)
{
...
}
Napi::Function::New(env, AFunction); <<< this is main.cpp:69
the same code compiles without NAPI_EXPERIMENTAL
, but then it does not behave correctly.
There was a recent addition covered under NAPI_EXPERIMENTAL to help people avoid calling into JavaScript when they should not be in finalizers. See - https://github.com/nodejs/node/pull/50060.
The "compiles with NAPI_EXPERIMENTAL.. but does not behave correctly" likely means that the code was making calls to JS where it should not be and therefore "did not behave correctly"
You can opt out of this change with NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT if you use NAPI_EXPERIMENTAL which will likely let you compile again, but you will also be back to "it not behaving correctly".
Your best bet is to update so that you can compile with the change, that process should also hopefully help narrow down in the code what was causing the "did not behave correctly"
There was a recent addition covered under NAPI_EXPERIMENTAL to help people avoid calling into JavaScript when they should not be in finalizers. See - nodejs/node#50060.
Yes, this is exactly what I want to use. My finalisers just call delete ptr
, no other node / js code.
The "compiles with NAPI_EXPERIMENTAL.. but does not behave correctly" likely means that the code was making calls to JS where it should not be and therefore "did not behave correctly"
"correctly" here means that the finaliser is called immediately (i.e. synchronous), as soon as gc runs.
I can tell if it happens or not because I log / count when it happens, and even with a little noise from gc, I can tell if the number is 0 or something.
So, without NAPI_EXPERIMENTAL
the code compiles and shows the old behaviour (which I call "incorrect"), i.e. finalisers called later (i.e. at the end).
Or one has to add some await
, setImmediate
etc., which I am trying to avoid.
You can opt out of this change with NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT if you use NAPI_EXPERIMENTAL which will likely let you compile again, but you will also be back to "it not behaving correctly".
If I define NAPI_EXPERIMENTAL
alone, I get compiler errors above.
if I define NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT
too, then it compiles, but same old (incorrect) behaviour.
Your best bet is to update so that you can compile with the change, that process should also hopefully help narrow down in the code what was causing the "did not behave correctly"
"update" what exactly? I am using node 22.1 and node-addon-api 8.
Could you please show me an example when I can call this Function::New
with NAPI_EXPERIMENTAL
?
https://github.com/nodejs/node-addon-api/blob/23bb42b5e47630c9082dddbabea555626571926e/napi.h#L1416
It might sound unrelated to the original problem, but it is the first compiler error I have when I enable NAPI_EXPERIMENTAL
.
I can call the other New
, at line 1394, but since I wrap objects with operator()
I need the one at line 1416.
Moreover, since I cannot find any match for node_api_nogc_finalize
in this repo,
I suspect node-addon-api
is not yet able to take advantage of this new feature.
But I hope to be completely wrong.
Look at that:
napi_add_finalizer
wants a finaliser of type node_api_nogc_finalize
we still pass a napi_finalize
(line 44).
So this only works if node_api_nogc_finalize
is typedef
'd to napi_finalize
.
So it still dont think node-addon-api
is ready for NAPI_EXPERIMENTAL
.
Hi @audetto ,
You are correct in that node-addon-api
does not yet support these nogc finalizers. There is a wip here: https://github.com/nodejs/node-addon-api/pull/1409
Hi @audetto ,
This came up in the 10 May Node-API meeting.
The current WIP linked above fixes compilation in experimental mode by wrapping finalizers in a node_api_post_finalizer
call. This would end up leading to the old behavior, where finalizers are ran asynchronously.
We are actively discussing the required node-addon-api changes in order to support the node_api_nogc_env
variant.
In the meantime, you can use the underlying Node-API call to napi_add_finalizer
and pass your own node_api_nogc_finalize
.
I have got several memory leak issues with this addon library. I thought it was me who wrote the code wrong. But when I am trying to run this example, it has the same memory leak too:
https://github.com/nodejs/node-addon-api/blob/main/doc/object_wrap.md
Run the JavaScript code in a
while (1)
loop will reveal the memory leak issue.What is going wrong with this?
C++ code
JavaScript code