node-addon-examples icon indicating copy to clipboard operation
node-addon-examples copied to clipboard

Context-sensitive examples are not extensible

Open ikokostya opened this issue 4 years ago • 0 comments

This pull request https://github.com/nodejs/node-addon-examples/pull/139 removes global static references from the examples. However, proposed solution works only for single object wrap.

Consider the following example where two object wraps are created using the same code snippet:

Napi::FunctionReference* constructor = new Napi::FunctionReference();
*constructor = Napi::Persistent(func);
env.SetInstanceData(constructor);

test.cc

#include <napi.h>

#include <cassert>

class Foo : public Napi::ObjectWrap<Foo> {
 public:
  static void Init(Napi::Env env, Napi::Object exports) {
    Napi::Function func = DefineClass(env, "Foo", {});

    Napi::FunctionReference* constructor = new Napi::FunctionReference();
    *constructor = Napi::Persistent(func);
    env.SetInstanceData(constructor);

    exports.Set("Foo", func);
  }

  static Napi::Object NewInstance(Napi::Env env) {
    const auto constructor = env.GetInstanceData<Napi::FunctionReference>();
    assert(constructor != nullptr);
    return constructor->New({});
  }

  explicit Foo(const Napi::CallbackInfo& info) : Napi::ObjectWrap<Foo>{info} {}
};

class Bar : public Napi::ObjectWrap<Foo> {
 public:
  static void Init(Napi::Env env, Napi::Object exports) {
    Napi::Function func = DefineClass(env, "Bar", {});

    Napi::FunctionReference* constructor = new Napi::FunctionReference();
    *constructor = Napi::Persistent(func);
    env.SetInstanceData(constructor);

    exports.Set("Bar", func);
  }

  static Napi::Object NewInstance(Napi::Env env) {
    const auto constructor = env.GetInstanceData<Napi::FunctionReference>();
    assert(constructor != nullptr);
    return constructor->New({});
  }

  explicit Bar(const Napi::CallbackInfo& info) : Napi::ObjectWrap<Foo>{info} {}
};

Napi::Value CreateFoo(const Napi::CallbackInfo& info) {
  return Foo::NewInstance(info.Env());
}

Napi::Value CreateBar(const Napi::CallbackInfo& info) {
  return Bar::NewInstance(info.Env());
}

Napi::Object InitAll(Napi::Env env, Napi::Object exports) {
  Foo::Init(env, exports);
  Bar::Init(env, exports);
  exports.Set("createFoo", Napi::Function::New(env, CreateFoo));
  exports.Set("createBar", Napi::Function::New(env, CreateBar));
  return exports;
}

NODE_API_MODULE(NODE_GYP_MODULE_NAME, InitAll)

test.js

'use strict';

const assert = require('assert');
const {createFoo, createBar, Foo, Bar} = require('./build/Debug/test');

const foo = createFoo();
assert(foo instanceof Foo); // fails
const bar = createBar();
assert(bar instanceof Bar);
$ node test.js
assert.js:383
    throw err;
    ^

AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:

  assert(foo instanceof Foo)

    at Object.<anonymous> (/home/kostya/tmp/napi-test/test.js:7:1)
    at Module._compile (internal/modules/cjs/loader.js:1063:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1092:10)
    at Module.load (internal/modules/cjs/loader.js:928:32)
    at Function.Module._load (internal/modules/cjs/loader.js:769:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12)
    at internal/main/run_main_module.js:17:47 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: false,
  expected: true,
  operator: '=='
}

As we can see second call of Env::SetInstanceData() in Bar::Init() overrides reference to Foo constructor. Moreover, this causes memory leak, because destructor of the first reference will not be called

From https://nodejs.org/dist/latest-v14.x/docs/api/n-api.html#n_api_napi_set_instance_data

Any existing data associated with the currently running Agent which was set by means of a previous call to napi_set_instance_data() will be overwritten. If a finalize_cb was provided by the previous call, it will not be called.

ikokostya avatar Jan 16 '21 14:01 ikokostya