Context-sensitive examples are not extensible
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
- Domain
- api, developer-experience
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
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from nodejs/node-addon-examples
-
enhancement
Difficulty 4/5 3-5 days Newbie friendliness 38/100
nodejs/node-addon-examples#585 · 1 reaction ·
-
Difficulty 3/5 1-2 days Newbie friendliness 38/100
nodejs/node-addon-examples#530 · 1 comment ·
-
Difficulty 4/5 3-5 days Newbie friendliness 35/100
nodejs/node-addon-examples#445 ·
-
Difficulty 3/5 1-2 days Newbie friendliness 32/100
nodejs/node-addon-examples#444 · 1 comment ·
-
Difficulty 3/5 1-2 days Newbie friendliness 35/100
nodejs/node-addon-examples#381 ·
All issues in nodejs/node-addon-examples
Similar issues
-
[BUG] Openbug
Difficulty 2/5 1-3 hours Newbie friendliness 85/100
eunomia-bpf/llvmbpf#51 · 1 comment ·
-
status:needs-triage
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
PX4/PX4-Autopilot#28776 ·
-
Website Doc Typo Open
Difficulty 1/5 Under an hour Newbie friendliness 92/100
-
Difficulty 1/5 1-3 hours Newbie friendliness 92/100
autowarefoundation/autoware_universe#13413 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 88/100