Object wrap has a memory leak bug?

[ammarfaizi2]

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

#include <napi.h>

class Example : public Napi::ObjectWrap<Example> {
  public:
    static Napi::Object Init(Napi::Env env, Napi::Object exports);
    Example(const Napi::CallbackInfo& info);
    static Napi::Value CreateNewItem(const Napi::CallbackInfo& info);

  private:
    double _value;
    Napi::Value GetValue(const Napi::CallbackInfo& info);
    Napi::Value SetValue(const Napi::CallbackInfo& info);
};

Napi::Object Example::Init(Napi::Env env, Napi::Object exports) {
    // This method is used to hook the accessor and method callbacks
    Napi::Function func = DefineClass(env, "Example", {
        InstanceMethod<&Example::GetValue>("GetValue", static_cast<napi_property_attributes>(napi_writable | napi_configurable)),
        InstanceMethod<&Example::SetValue>("SetValue", static_cast<napi_property_attributes>(napi_writable | napi_configurable)),
        StaticMethod<&Example::CreateNewItem>("CreateNewItem", static_cast<napi_property_attributes>(napi_writable | napi_configurable)),
    });

    Napi::FunctionReference* constructor = new Napi::FunctionReference();

    // Create a persistent reference to the class constructor. This will allow
    // a function called on a class prototype and a function
    // called on instance of a class to be distinguished from each other.
    *constructor = Napi::Persistent(func);
    exports.Set("Example", func);

    // Store the constructor as the add-on instance data. This will allow this
    // add-on to support multiple instances of itself running on multiple worker
    // threads, as well as multiple instances of itself running in different
    // contexts on the same thread.
    //
    // By default, the value set on the environment here will be destroyed when
    // the add-on is unloaded using the `delete` operator, but it is also
    // possible to supply a custom deleter.
    env.SetInstanceData<Napi::FunctionReference>(constructor);

    return exports;
}

Example::Example(const Napi::CallbackInfo& info) :
    Napi::ObjectWrap<Example>(info) {
  Napi::Env env = info.Env();
  // ...
  Napi::Number value = info[0].As<Napi::Number>();
  this->_value = value.DoubleValue();
}

Napi::Value Example::GetValue(const Napi::CallbackInfo& info){
    Napi::Env env = info.Env();
    return Napi::Number::New(env, this->_value);
}

Napi::Value Example::SetValue(const Napi::CallbackInfo& info){
    Napi::Env env = info.Env();
    // ...
    Napi::Number value = info[0].As<Napi::Number>();
    this->_value = value.DoubleValue();
    return this->GetValue(info);
}

// Initialize native add-on
Napi::Object Init (Napi::Env env, Napi::Object exports) {
    Example::Init(env, exports);
    return exports;
}

// Create a new item using the constructor stored during Init.
Napi::Value Example::CreateNewItem(const Napi::CallbackInfo& info) {
  // Retrieve the instance data we stored during `Init()`. We only stored the
  // constructor there, so we retrieve it here to create a new instance of the
  // JS class the constructor represents.
  Napi::FunctionReference* constructor =
      info.Env().GetInstanceData<Napi::FunctionReference>();
  return constructor->New({ Napi::Number::New(info.Env(), 42) });
}

// Register and initialize native add-on
NODE_API_MODULE(NODE_GYP_MODULE_NAME, Init)

JavaScript code

const chnet = require('bindings')('chnet');

while (1) {
	const example = new chnet.Example(11);
	example.GetValue();
	example.SetValue(19);
	example.GetValue();
}

[KevinEady]

Hi @ammarfaizi2 ,

We discussed this on today's Node API meeting. This is a similar issue as found on #1140 . We are waiting on some PRs in node core (nodejs/node#42651, nodejs/node#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.

[github-actions]

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.

[ammarfaizi2]

It will be closed soon unless the stale label is removed or a comment is made.

Dropping a comment, just to avoid closing.

[github-actions]

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.

[ammarfaizi2]

It will be closed soon unless the stale label is removed or a comment is made.

Dropping a comment, just to avoid closing.

[audetto]

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

[gabrielschulhof]

@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).

[audetto]

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.

[audetto]

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.

[Apidcloud]

which feature did you enable through NAPI_EXPERIMENTAL, @audetto ?

[KevinEady]

@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

[audetto]

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?

[audetto]

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,

      |                    ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~

[audetto]

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.

[mhdawson]

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.

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"

[audetto]

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?

node-addon-api/napi.h

Line 1416 in 23bb42b

static Function New(napi_env env,

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.

[audetto]

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.

[audetto]

Look at that:

https://github.com/nodejs/node/blob/22cb99d073b03414e33843550cca3bac2833a361/src/js_native_api.h#L520

napi_add_finalizer wants a finaliser of type node_api_nogc_finalize

But here

node-addon-api/napi-inl.h

Line 68 in 23bb42b

status = napi_add_finalizer(env, obj, data, finalizer, hint, nullptr);

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.

[KevinEady]

Hi @audetto ,

You are correct in that node-addon-api does not yet support these nogc finalizers. There is a wip here: #1409

[KevinEady]

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.

[audetto]

It looks to me that this extensions requires C++17, maybe it should be checked and an error returned otherwise.

[audetto]

I solved compilation problems, but I am back in a situation where the finaliser is not called synchronously.

I define NAPI_EXPERIMENTAL, and I can see all the destructors being called only after the end of the script.

How do I tell node if my finaliser is safe or not?

[audetto]

Finally, I got it to work. My finalisers were using Env and not a BasicEnv.

If I understand correctly, a BasicEnv cannot be used to interact with absolutely any node function.

[KevinEady]

Closing this issue as solved by #1514