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 ?