InstanceMethod does not accept function returning Napi::Object

Hello,
First off, I send my sincere regards for all the time and efforts you put into the project.

Preface

I have a simple C++ class as below:

class mmapIPC : public Napi::ObjectWrap<mmapIPC>
{
  public:
    static Napi::Object Init(Napi::Env env, Napi::Object exports);
    mmapIPC(const Napi::CallbackInfo& info);
    static Napi::Value CreateNewItem(const Napi::CallbackInfo& info);

  private:
    //-----------members
    shm_metadata shm_hint;
    Napi::Buffer<void> memoryBuffer;
    //-----------funcs
    Napi::Object buffer(const Napi::CallbackInfo& info);
    Napi::Value acquireReadLock(const Napi::CallbackInfo& info);
    Napi::Value acquireWriteLock(const Napi::CallbackInfo& info);
    Napi::Value removeLock(const Napi::CallbackInfo& info);
    //-----------statics
    static void freeCallback(Napi::Env, void* memoryAddress, shm_metadata* hint);
    static int createShmFile( const char *name, std::size_t memLenght);
    static void* mapMemory( int shm_fd, std::size_t regionLength, bool lockPagesToRam );
};

Which is based on examples of this repo for object_wrap; Now in my static Napi::Object Init I have the following code to export members:

// This method is used to hook the accessor and method callbacks
    Napi::Function functionList = DefineClass(env, "mmapIPC",
    {
        InstanceMethod<&mmapIPC::buffer>("buffer", static_cast<napi_property_attributes>(napi_writable | napi_configurable)),
        InstanceMethod<&mmapIPC::acquireReadLock>("acquireReadLock", static_cast<napi_property_attributes>(napi_writable | napi_configurable)),
        InstanceMethod<&mmapIPC::acquireWriteLock>("acquireWriteLock", static_cast<napi_property_attributes>(napi_writable | napi_configurable)),
        InstanceMethod<&mmapIPC::removeLock>("removeLock", static_cast<napi_property_attributes>(napi_writable | napi_configurable)),
        StaticMethod<&mmapIPC::CreateNewItem>("CreateNewItem", static_cast<napi_property_attributes>(napi_writable | napi_configurable))
    });

And definition for Napi::Object buffer is (it's some kind of setter/getter accessor at the same?) :

Napi::Object mmapIPC::buffer(const Napi::CallbackInfo& info)
{
    return this->memoryBuffer;
}

Questions:

First of all, when I set the return type for buffer() function as Napi::Object the InstanceMethod complains about it, and when I change it to Napi::Value even tho it seems I can use buf1.buffer().write('Balh Blah', 'ascii') to write into the buffer, calling buf1.buffer().toString('ascii') prints the whole stack trace of node!
I should note that when I tested the code without OOP, exporting buffer() with Napi::Object as return type was fine and everything was working smoothly:

exports.Set(Napi::String::New(env, "buffer"),
                Napi::Function::New(env, buffer));

In this code, is it fine to supply void as the type? If not, what else? char or u_int8_t perhaps?:

this->memoryBuffer =  Napi::Buffer<void>::New(
                        env,
                        externalData,
                        this->shm_hint.mmapLength,
                        freeCallback,
                        &this->shm_hint);

In the example for object_wrap there's this static Napi::Value CreateNewItem(const Napi::CallbackInfo& info) method which retrieves the stored constructor and I quote: "to create a new instance of the JS class the constructor represents". What does this method do? Couldn't I just create another instance using the required constructor in my JS code? It says in the comments for static Napi::Object Init(Napi::Env env, Napi::Object exports) that: "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"! So is this method necessary?

P.S: I'm very truly sorry if this turned out to be a long question; I just wanted to ask it all once and get it over with...

Hi @th3r0b0t

Thanks for waiting on our response. This kinda slipped our radar -- but we are using a new project system to hopefully keep things organized better!

First of all, when I set the return type for buffer() function as Napi::Object the InstanceMethod complains about it,

This is correct behavior, because the an instance wrap's InstanceMethod must be either void MethodName(const Napi::CallbackInfo&) or Napi::Value MethodName(const Napi::CallbackInfo&). It cannot return an Object statically and must return a Napi::Value. However, inside MethodName, you can return an Object.

... when I change it to Napi::Value even tho it seems I can use buf1.buffer().write('Balh Blah', 'ascii') to write into the buffer, calling buf1.buffer().toString('ascii') prints the whole stack trace of node!

This may be because your mmapIPC class stores a Napi::Buffer<void> memoryBuffer instead of a reference to a buffer. The example in object_wrap.md uses a double, which can easily be memory-managed by C++. When using a Node-API object type, such as Napi::Buffer, the objectwrap must keep a reference to the buffer, so that when the objectwrap is garbage-collected, it can free its reference to the buffer, and the GC can free that buffer. You should be able to hold a Napi::Reference<Napi::Buffer<T>> inside your ObjectWrap class, and access that instead. See our CI's test object_wrap.cc, in particular:

The buffer member in the ObjectWrap class: bufref_.
Assigning the bufref_ member to a new, referenced buffer

You can then access the underlying buffer from the reference using eg. bufref_.Value().

When not using references, Node has no way of knowing that your mmapIPC class uses the buffer, so it may decide that some garbage collection event can occur and free the buffer.

2. In this code, is it fine to supply void as the type? If not, what else? char or u_int8_t perhaps?:

It is not good to use a void type if you need to run some sort of delete on your underlying native data. Again, looking at our test's bufref_ assignment, it has a finalizer:

node-addon-api/test/objectwrap.cc

Lines 49 to 53 in 294a43f

    
           bufref_ = Napi::Persistent(Napi::Buffer<uint8_t>::New( 
        
               Env(), 
        
               static_cast<uint8_t*>(malloc(1)), 
        
               1, 
        
               [](Napi::Env, uint8_t* bufaddr) { free(bufaddr); }));

A C++ call to delete cannot be done on a void* type, and since this finalizer must clean the native data, a Buffer<void> type is not applicable. If for some reason your implementation did not need to delete the data (for example, some static buffer), then a void may be acceptable.

3. In the example for object_wrap there's this static Napi::Value CreateNewItem(const Napi::CallbackInfo& info) method ... What does this method do?

There may be a time where you want to create an instance of your object within C++, ie. do the JavaScript equivalent of new Example(42) within C++ using node-addon-api. This is entirely up to your own use case: if your C++ code never needs to construct an instance within C++, you do not need to store the constructor's FunctionReference with SetInstanceData. Assigning the constructor as a member of exports is enough: Node gives your module exports to whatever require()'s it, and then the regular V8 garbage collection / reference handling happens.

3. ... Couldn't I just create another instance using the required constructor in my JS code?

Sure, you can absolutely do eg. new Example(11) in your JavaScript code and pass that to your native functions. The object_wrap example is just providing a use case if you needed to construct a JavaScript object within C++ ... Think of it it as a way to do Napi::Object::New() but with your constructor to create an instance of your class.

3. ... that: "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"! So is this method necessary?

Since object_wrap example add-on creates JavaScript instances within C++, the C++ add-on must have a reference to the constructor for the class. See https://nodejs.org/api/n-api.html#environment-life-cycle-apis and https://nodejs.github.io/node-addon-examples/special-topics/context-awareness/ for a little information on context-aware add-ons. Since an add-on can be loaded multiple times for different environments, it is not safe to have c-style, global static data, as it can be mangled when using multiple environments (worker threads, ...). This can be addressed using instance data, which associates any arbitrary data (in object_wrap's case, the constructor) on the Node-API environment. This allows us to later (within CreateNewItem) get the constructor that is associated with the current env.

Thanks again for your patience! Let us know if you have any more questions. If this clears everything up, feel free to close the issue.

Hi @th3r0b0t ,

We believe the message above answers all your questions. If not, please feel free to reopen this issue!

Hi @th3r0b0t ,

We believe the message above answers all your questions. If not, please feel free to reopen this issue!

Thanks for your time and support.
I've been a little busy since; I'll sure to try it out and in case of some confusion open this/anew issue.
Again, my humblest regards.
@KevinEady

	bufref_ = Napi::Persistent(Napi::Buffer<uint8_t>::New(
	Env(),
	static_cast<uint8_t*>(malloc(1)),
	1,
	[](Napi::Env, uint8_t* bufaddr) { free(bufaddr); }));