5

Ok first off I'm very new to C++ so apologies if my understanding is poor. I'll try explain myself as best I can. What I have is I am using a library function that returns a std::shared_ptr<SomeObject>, I then have a different library function that takes a raw pointer argument (more specifically node-addon-api Napi::External<T>::New(Napi::Env env, T *data) static function). I want to create a Napi::External object using my std::shared_ptr. What I am currently doing is this:

{
    // ...
    std::shared_ptr<SomeObject> pSomeObject = something.CreateSomeObject();
    auto ext = Napi::External<SomeObject>::New(info.Env(), pSomeObject.get());
    auto instance = MyNapiObjectWrapper::Create({ ext });
    return instance;
}

But I am worried this will run into memory issues. My pSomeObject only exists in the current scope, so I imagine what should happen is after the return, it's reference count will drop to 0 and the SomeObject instance it points to will be destroyed and as such I will have issues with the instance I return which uses this object. However I have been able to run this code and call functions on SomeObject from my instance, so I'm thinking maybe my understanding is wrong.

My question is what should I do when given a shared pointer but I need to work off a raw pointer because of other third party library requirements? One option that was proposed to me was make a deep copy of the object and create a pointer to that

If my understanding on any of this is wrong please correct me, as I said I'm quite new to C++.

============================

Edit:

So I was missing from my original post info about ownership and what exactly this block is. The block is an instance method for an implementation I have for a Napi::ObjectWrap instance. This instance method needs to return an Napi::Object which will be available to the caller in node.js. I am using Napi::External as I need to pass a sub type of Napi::Value to the constructor New function when creating the Napi:Object I return, and I need the wrapped SomeObject object in the external which I extract in my MyNapiObjectWrapper constructor like so:

class MyNapiObjectWrapper
{
private:
    SomeObject* someObject;
    static Napi::FunctionReference constructor; // ignore for now
public:
    static void Init(Napi::Env env) {...}
    MyNapiObjectWrapper(const CallbackInfo& info)
    {
        Napi::Env env = info.Env();
        Napi::HandleScope scope(env);

        // My original code to match the above example
        this->someObject = info[0].As<const Napi::External<SomeObject>>().Data();
    }

    DoSomething()
    {
        this->someObject->DoSomething();
    }
}

I have since come to realise I can pass the address of the shared pointer when creating my external and use it as follows

// modified first sample
{{
    // ...
    std::shared_ptr<SomeObject> pSomeObject = something.CreateSomeObject();
    auto ext = Napi::External<SomeObject>::New(info.Env(), &pSomeObject);
    auto instance = MyNapiObjectWrapper::Create({ ext });
    return instance;
}

// modified second sample
class MyNapiObjectWrapper
{
private:
    std::shared_ptr<SomeObject> someObject;
    static Napi::FunctionReference constructor; // ignore for now
public:
    static void Init(Napi::Env env) {...}
    MyNapiObjectWrapper(const CallbackInfo& info)
    {
        Napi::Env env = info.Env();
        Napi::HandleScope scope(env);

        // My original code to match the above example
        this->someObject = 
            *info[0].As<const Napi::External<std::shared_ptr<SomeObject>>>().Data();
    }

    DoSomething()
    {
        this->someObject->DoSomething();
    }
}

So now I am passing a pointer to a shared_ptr to create my Napi::External, my question now though is this OK? Like I said at the start I'm new to c++ but this seems like a bit of a smell. However I tested it with some debugging and could see the reference count go up, so I'm thinking I'm in the clear???

Nick Hyland
  • 357
  • 1
  • 10
  • 1
    We can't know the ownership of objects without seeing code you don't/can't include. Obviously the object owner by `pSomeObject ` is destroyed at the end of the block. If any of the methods called don't expect this you will have UB. If however they deep copy the object pointed to by `pSomeObject` all will be OK. It's read the documentation time looking for ownership information. – Richard Critten Nov 09 '19 at 14:22
  • I am not sure about what is happening in the `CreateSomeObject` function, they must maintain ownership of the object, as the SomeObject instance is not getting destroyed after the block. What is happening from my end is I create the `External` to wrap the some object, then I am passing the external to a `Napi::ObjectWrapper` (`MyNapiObjectWrapper` in the example above) so I can extract the pointer and keep it as a member variable. I can see why my code is wrong but I can't see yet how I should fix it. – Nick Hyland Nov 09 '19 at 17:37

1 Answers1

3

Here the important part of the documentation:

The Napi::External template class implements the ability to create a Napi::Value object with arbitrary C++ data. It is the user's responsibility to manage the memory for the arbitrary C++ data.

So you need to ensure that the object passed by data to Napi::External Napi::External::New exits until the Napi::External<T> object is destructed.

So the code that you have shown is not correct.

What you could do is to pass a Finalize callback to the New function:

static Napi::External Napi::External::New(napi_env env,
                    T* data,
                    Finalizer finalizeCallback);

And use a lambda function as Finalize, that lambda could hold a copy through the capture to the shared pointer allowing to keep the shared pointer alive until finalize is called.

std::shared_ptr<SomeObject> pSomeObject = something.CreateSomeObject();
auto ext = Napi::External<SomeObject>::New(
                  info.Env(), 
                  pSomeObject.get(),
                  [pSomeObject](Env /*env*/, SomeObject* data) {});
t.niese
  • 39,256
  • 9
  • 74
  • 101
  • That would work, thank you. Although I think I would have to change my ObjectWrapper code. I am currently in the constructor getting the External and extracting the SomeObject pointer, then storing the SomeObject pointer as member variable. I would have to store the external as my member variable so the external wont get destroyed until my instance gets destroyed. – Nick Hyland Nov 09 '19 at 17:46
  • @NickHyland I don't understand what you mean. The capture of the lambda (`[pSomeObject]`) creates a copy of `pSomeObject` and that copy is kept alive as long as the lambda is, and that should be until the `Finalize` callback is called. – t.niese Nov 09 '19 at 20:10
  • Sorry I haven't actually made myself clear in that comment or probably my original post. In my constructor for `MyNapiObjectWrapper` to which the `External` is passed, I am just extracting the `SomeObject` pointer and storing that as a member variable. I am not holding a reference to the External passed to this, so I imagine the finalizer method would get called down along the line when the JS garbage collector kicks in. However I have since realised that my overall design was probably flawed so I have changed up my actual original implementation. Thanks for your help – Nick Hyland Nov 09 '19 at 23:41
  • Thank you for your help on this. I think I am going to ask the question again elsewhere in a more general sense. What I was really hoping to learn was what are the best practices / what are the most common ways C++ developers deal with having to downgrade from smart pointers to raw pointers because of constraints – Nick Hyland Nov 10 '19 at 08:55
  • 1
    @NickHyland you can't downgrade from a `shared_ptr` to a managed raw pointer. there is no `release` like there is for `unique_ptr`. If your `something.CreateSomeObject` returns a `unique_ptr` - which should be the first choice when creating a factory function - then you would be able to release the object from being owned by the `unique_ptr`. It is not really possible to tell how to solve it in your case, because the exact use-case is not known. it is not clear why/how you use `Napi::External`, how the ownership over `SomeObject` is handled, ... – t.niese Nov 10 '19 at 09:35
  • 1
    @NickHyland and if you need to use `Napi::External` to pass a `shared_ptr` from on part of your application to another, then you could create wrapper with a member `shared_ptr value` and `Napi::External::New`, but as I said with the given information it is not really possible to tell. – t.niese Nov 10 '19 at 09:46
  • So for my specific use case, I have an `Napi::ObjectWrapper` which is calling a function which is the above block in question. The function is being called from JS and expects a new JS object returned. The JS object returned needs to call C++ code when it's functions are being called, the C++ code it needs are the functions provided by the `SomeObject` instance. `MyNapiObjectWrapper::Create` basically calls `New` on a `Napi::FunctionReference` constructor to return a `Napi::Value` and create the underlying C++ object. The `External` is passed to new so we can use someObject in the c++ obj – Nick Hyland Nov 10 '19 at 10:48