Context-sensitive examples are not extensible

Open
#171 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
4/5
Estimated time
3-5 days
Newbie friendliness
35/100
Issue type
Bug
Clarity
Mostly clear
Activity status
Stale
Tech stack
cpp, nodejs

Research direction

Start with the test.cc and test.js reproducer, then read the N-API SetInstanceData documentation cited in the issue. Verify how creating Foo and Bar causes the second instance-data value to replace the first. Done means both createFoo() and createBar() produce objects passing their respective instanceof checks without leaking the earlier constructor reference.

Written by the indexing model from the issue text.

Description

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.

Dominant language
C++
Stars
2.6k
Forks
602
PR merge metrics
No merged PRs in 30d

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from nodejs/node-addon-examples

All issues in nodejs/node-addon-examples

Similar issues

More C++ issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.