pointer wrapper constructor #72
Conversation
| // How does the following interact with the static storage of constructor? | ||
| // Does not doing this create problems for multiple instance of this add-on, in multiple worker threads? | ||
| // See: https://github.com/nodejs/node-addon-api/blob/main/doc/object_wrap.md#example | ||
| // env.SetInstanceData<Napi::FunctionReference>(&constructor); // is this so this is eventually freed? |
There was a problem hiding this comment.
It seems that storing this function reference in a static might be a problem if multiple instance of this add-on are created in the same Node process, in different environments (e.g. worker threads). (More details here.) Not sure how likely or useful that is, but I'd rather not create a potential problem if I don't have to.
But we can't store these constructors directly using SetInstanceData, because there is only one "data" slot per add-on instance, and we have multiple PointerHolders.
Ideas for how to do this better:
- Store a map from name to constructor in the instance data. This means we'd need to keep the name around in the PointerHolder (so we can look up the right constructor), but putting the name in a static seems fine. This is a bit more work to set up and a bit more overhead at runtime, but these are likely insignificant.
- Alternatively, since we're already storing the constructor in the exports, it seems like it should be possible to look it up using the environment (e.g. using Globals). I haven't quite figured that out, but I probably just haven't learned the right way.
There was a problem hiding this comment.
Oh, I just learned (by reading more docs) that the Napi::Addon uses Napi::Env::SetInstanceData internally, so it shouldn't be used to store other data:
Note: Napi::Addon uses Napi::Env::SetInstanceData() internally. This means that the add-on should only use Napi::Env::GetInstanceData explicitly to retrieve the instance of the Napi::Addon class. Variables whose scope would otherwise be global should be stored as instance variables in the Napi::Addon class.
The recommendation is to store stuff directly on your Addon instance object, and use GetInstanceData to get the Addon instance:
void ExampleBinding(const Napi::CallbackInfo& info) {
ExampleAddon* addon = info.Env().GetInstanceData<ExampleAddon>();
}
There was a problem hiding this comment.
Here's where node-addon-api puts the addon instance into the instance data: https://github.com/nodejs/node-addon-api/blob/cfcd2cf61d8d939bc2a2744af2ee72f07e4119c1/napi-inl.h#L6518
Use the actual constructor of the JS object when creating a new PointerWrapper instance in NewAndSet.
Generate actual JS classes for the
void *anduint64_t *PointerWrapper types (e.g.pointeranduint64_pointer).Also simplify use of PointerWrapper::Init by taking the object to which to add the export.