Return std::shared_ptr of another wrapped class from a member function

TL;DR

returning class instance by value creates too many copies of it (mostly irrelevant)
returning class instance by shared_ptr doesn't "register" it in v8

electron v9.1 node.js v12.14.1 V8 v8.3 v8pp v1.7.0

Am i doing something wrong here? Is this behaviour intended? Is there a better solution?

I have a project that was using nbind for several years, but since it was abandoned, decision was made to upgrade to latest LTS node.js release & adopt another V8 binding library.

Please forgive me if code smells, I write in JS most of the time. I've set up a playground with v8pp + electron to check things out and immediately stumbled upon this weird behaviour:

// objects.hpp

...

class A {
private:
    int x_;
    static int instance_counter;

public:
    typedef std::shared_ptr<A> Ptr;
    int id;

    A(int x)
    : id(instance_counter++),
      x_(x)
    {
        std::cout << "C++: " << "A#" << id << " constructed" << std::endl;
    }

    A(const A& obj)
    : id(instance_counter++)
    {
        x_ = obj.x_;
        std::cout << "C++: " << "A#" << id << " copy-constructed from " << obj.id << std::endl;
    }

    ~A()
    {
        std::cout << "C++: " << "A#" << id << " destructed" << std::endl;
    }

    int get_x() { return x_; }
    void set_x(int value) { x_ = value; }
};
int A::instance_counter = 1;

class B {
public:
    // this works just as expected, no additional copies of A created
    void do_something_with_a(A::Ptr obj) {
        obj->set_x(obj->get_x() + 1);
    }

    // this creates 3(!) instances of A, 1 "original" + 2 copies
    A get_a_by_value() {
        A obj(1);

        return obj;
    }

    // this doesn't work when using shared_ptr_traits, but that's to be expected i guess
    A* get_a_by_rptr() {
        A* obj = new A(10);
        return obj;
    }

    // doesn't work, doesn't need to work
    A& get_a_by_ref() {
        A obj(100);
        return obj;
    }

    // this one doesn't work while expected to
    A::Ptr get_a_by_sptr() {
        A::Ptr obj = std::make_shared<A>(1000);
        return obj;
    }
};

// bindings.cpp

...

void InitAll(
    v8::Local<v8::Object> exports
) {
    v8::Isolate* isolate = v8::Isolate::GetCurrent();

    v8pp::module module(isolate);

    v8pp::class_<A, v8pp::shared_ptr_traits> A_class(isolate);
    A_class
        .ctor<int>()
        // .auto_wrap_objects()
        .set("x", v8pp::property(&A::get_x, &A::set_x));
    module.set("A", A_class);

    v8pp::class_<B, v8pp::shared_ptr_traits> B_class(isolate);
    B_class
        .ctor()
        // .auto_wrap_objects()
        .set("do_something_with_a", &B::do_something_with_a)
        .set("get_a_by_value", &B::get_a_by_value)
        // .set("get_a_by_rptr", &B::get_a_by_rptr)
        // .set("get_a_by_ref", &B::get_a_by_ref)
        .set("get_a_by_sptr", &B::get_a_by_sptr);
    module.set("B", B_class);

    exports->SetPrototype(isolate->GetCurrentContext(), module.new_instance());
}

// main.js

...

// exactly 1 isntance of A created, as expected
const a = new addon.A(1, "a name");

// exactly 1 instance of B created, as expected
const b = new addon.B();
console.log(`b = ${b}`); // b = [object B]
console.log();

// creates no additional copies of A, passes A by shared_ptr
b.do_something_with_a(a);

// creates 2 additional copies of A
console.log('### BY VALUE ###')
try {
    const a_value = b.get_a_by_value();
    console.log(`a_value = ${a_value}`);
} catch (error) {
    console.error(error);
}
console.log();

// doesn't work
console.log('### BY RAW PTR ###')
try {
    const a_rptr = b.get_a_by_rptr();
    console.log(`a_rptr = ${a_rptr}`);
} catch (error) {
    console.error(error);
}
console.log();

// doesn't work
console.log('### BY REF ###')
try {
    const a_ref = b.get_a_by_ref();
    console.log(`a_ref = ${a_ref}`);
} catch (error) {
    console.error(error);
}
console.log();

// now this is interesting
// while C++ compiles and doesn't seem to complain about anything
// instance of A is created and immediately disposed of, so on JS side i'm getting undefined
console.log('### BY SMART PTR ###')
try {
    const a_sptr = b.get_a_by_sptr();
    console.log(`a_sptr = ${a_sptr}`); // a_sptr = undefined
} catch (error) {
    console.error(error);
}
console.log();

As far as i understand, this happens because shared_ptr<A> instance from get_a_by_sptr is not "registered" anywhere in v8. That's why to_v8 call results in undefined.

But i have far too little experience with C++ to understand exactly why this happens, or if there's anything i'm doing that caused such a behaviour. I managed to come up with a workaround like this:

// bindings.cpp

// Parts of this workaround were ripped straight from v8pp source code
template<typename T, typename Method>
MethodFunctor make_method_for_shared(Method mem_func) {
    return [mem_func] (const v8::FunctionCallbackInfo<v8::Value>& args) {
        using class_type = T;
        using class_pointer_type = std::shared_ptr<class_type>;

        using mem_func_traits = v8pp::detail::function_traits<Method>;
        using mem_func_type = typename mem_func_traits::template pointer_type<class_type>;

        v8::Isolate* isolate = args.GetIsolate();
        mem_func_type mf(mem_func);

        class_pointer_type self = v8pp::class_<T, v8pp::shared_ptr_traits>::unwrap_object(isolate, args.This());

        if (!self) {
            args.GetReturnValue().Set(v8::Null(isolate));
            return;
        }

        v8::EscapableHandleScope scope(isolate);

        mem_func_traits::return_type result = v8pp::detail::call_from_v8<
            v8pp::detail::call_from_v8_traits<mem_func_type, 0>,
            class_type,
            mem_func_type
        >
        (*self, std::forward<mem_func_type>(mf), args);

        v8::Local<v8::Object> local = v8pp::class_<mem_func_traits::return_type::element_type, v8pp::shared_ptr_traits>::reference_external(isolate, result);
        args.GetReturnValue().Set(scope.Escape(local));
    };
}

...

// Used like this
v8pp::class_<B, v8pp::shared_ptr_traits> B_class(isolate);
B_class
    .ctor()
    ....
    .set("get_a_by_sptr", make_method_for_shared<B>(&B::get_a_by_sptr));

it seems like there's already a PR https://github.com/pmed/v8pp/pull/134 that solves the exact problem i've been struggling with

I tried to incorporate that code by deriving fromv8pp::class_ and ptr traits structs, that didn't go well. Apparently v8pp::class_ wasn't designed as something to inherit from, since it has no virtual methods, frequently returns *this by reference and uses 'private' instead of 'protected'.

Am i missing something obvious here? About 7 months has passed since that PR was created. As there are no comments there, i must assume that the issue PR solves is not an issue at all and v8pp already provides a way to elegantly handle such cases.

Hi Dmitriy,

thanks for the sharing!

I actually don't remember why that PR is still hasn't been merged. Despite the auto wrapping feature is there.

I need some time to recall the details. Unfortunately I can't promise anything because I don't have much spare time for the project right now.

Best regards Pavel

чт, 9 июл. 2020 г., 13:44 Dmitriy Luchikov notifications@github.com:

it seems like there's already a PR #134 https://github.com/pmed/v8pp/pull/134 that solves the exact problem i've been struggling with

I tried to incorporate that code by deriving fromv8pp::class and ptr traits structs, that didn't go well. Apparently v8pp::class wasn't designed as something to inherit from, since it has no virtual methods, frequently returns *this by reference and uses 'private' instead of 'protected'.

Am i missing something obvious here? About 7 months has passed since that PR was created. As there are no comments there, i must assume that the issue PR solves is not an issue at all and v8pp already provides a way to elegantly handle such cases.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/pmed/v8pp/issues/139#issuecomment-656079132, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAISAUX4KZKGQZPJJICFOZDR2WURHANCNFSM4OU5RTJA .

After playing around with debugger for a bit i realized that there's indeed a much easier way to automatically wrap shared_ptr instances. All one need to do is to provide a specialization for a v8pp::convert. Since i need to wrap like couple dozens of classes, i had to use a macro.

Auto-wrapping would still be nice though.

// wrapper.hpp

#define CREATE_V8PP_SHARED_CONVERTER(klass) \
namespace v8pp { \
template<> \
struct convert<std::shared_ptr<##klass>, typename std::enable_if<is_wrapped_class<##klass>::value>::type> \
{ \
    using from_type = std::shared_ptr<##klass>; \
    using to_type = v8::Local<v8::Object>; \
    using class_type = typename std::remove_cv<##klass>::type; \
    static bool is_valid(v8::Isolate*, v8::Local<v8::Value> value) \
    { \
        return !value.IsEmpty() && value->IsObject(); \
    } \
    static from_type from_v8(v8::Isolate* isolate, v8::Local<v8::Value> value) \
    { \
        if (!is_valid(isolate, value)) \
        { \
            return nullptr; \
        } \
        return class_<class_type, shared_ptr_traits>::unwrap_object(isolate, value); \
    } \
    static to_type to_v8(v8::Isolate* isolate, std::shared_ptr<##klass> const& value) \
    { \
        auto& class_info = detail::classes::find<shared_ptr_traits>(isolate, detail::type_id<class_type>()); \
        auto wrapped_obj = class_<class_type, shared_ptr_traits>::find_object(isolate, value); \
        if (wrapped_obj.IsEmpty() && class_info.auto_wrap_objects()) { \
            wrapped_obj = class_<class_type, shared_ptr_traits>::reference_external(isolate, value); \
        } \
        return wrapped_obj; \
    } \
}; \
}

// bindings.cpp

...

CREATE_V8PP_SHARED_CONVERTER(A);
CREATE_V8PP_SHARED_CONVERTER(B);
CREATE_V8PP_SHARED_CONVERTER(C);

I'm also affected by this bug. I want to create a DOM-like structure in which the root is created by C++ and then JS can append nodes to the tree. Using stack-allocated classes is not an option because that creates many copies and I lose referential stability. Using raw pointers is also not an option because they get deleted by the GC, leaving invalid pointers in the tree. std::shared_ptr is a nice alternative because I can have both C++ and JS share ownership of the nodes.

Imagine something like this:

class Node {};

class Group : public Node {
public:
  void addNode(std::shared_ptr<Node> child) { _children.push_back(child); }
  std::shared_ptr<Node> getNode(unsigned int index) { return _children[index]; }

private:
  std::vector<std::shared_ptr<Node>> _children;
};

After appending nodes from JS. If I try to read a node I just added, I get the correct pointer, but if I let the GC run (or foce it with --expose-gc), and then try to read that same node, I get undefined because the node is no longer registered.

The convert.hpp file in the library already includes a struct convert<std::shared_ptr<T>, ... that can be modified. This is the current contents:

  static to_type to_v8(v8::Isolate* isolate, std::shared_ptr<T> const& value)
  {
    return class_<class_type, shared_ptr_traits>::find_object(isolate, value);
  }

And changing it to this instead (following the code by @poniraq), seems to work well, at least in my case:

  static to_type to_v8(v8::Isolate *isolate, std::shared_ptr<T> const &value) 
  {
    auto &class_info = detail::classes::find<shared_ptr_traits>(
        isolate, detail::type_id<class_type>());
    auto wrapped_obj =
        class_<class_type, shared_ptr_traits>::find_object(isolate, value);
    if (wrapped_obj.IsEmpty() && class_info.auto_wrap_objects()) 
    {
      wrapped_obj = class_<class_type, shared_ptr_traits>::reference_external(
          isolate, value);
    }
    return wrapped_obj;
  }

The problem with changing that directly in convert.hpp is that it creates a dependency cycle when trying to use detail::classes, so it won't compile. To hack around this in my case, I had to remove the std::shared_ptr converter from convert.hpp and put it in my project files instead. I did it by wrapping the converter with an #ifndef so I can disable it conditionally from my project and minimize my changes to the v8pp library.

It would be nice if someone with a good mental map of the header dependencies could figure out a way to make this work directly in the v8pp project (i.e. how to untangle the dependency cycle).

Actually, that approach also doesn't work well. Consider the following Javascript:

const root = new Group();
root->addChild(new Group()); // Will be garbage collected because there are no more JS references.
console.log('CHILD', root.getChild(0).constructor.name); // Group, correct.
gc(); // Requires --expose-gc
console.log('CHILD', root.getChild(0).constructor.name); // Node, incorrect.

And it gets worse. Once the child gets wrapped as a Node, all methods from Group are undefined. Not sure how to make it wrap the correct subclass, though.

To follow up on this, I found a way to support my particular use case for v8pp. It requires having full control of the classes for which you are creating the bindings, because the idea is to have the class hierarchy collaborate with the implementation of to_v8. The good thing is I can create a converter for shared_ptr<Node> instead of hacking the v8pp library to create a generic one.

static to_type to_v8(v8::Isolate *isolate, std::shared_ptr<Node> const &value) {
  // value-> is used for run-time type identification. But we still have to pass
  // the shared_ptr in order to wrap it, otherwise we risk creating two chains
  // of shared_ptr pointing to the same object and having it destroyed twice.
  return value->wrap(isolate, value);
}

And then, every node of the hiearchy would implement their own wrap method:

class Group: public Node {
  // ...
  virtual v8::Local<v8::Object> wrap(v8::Isolate *isolate,
                                     std::shared_ptr<Node> ptr) override {
    auto wrapped_obj =
        v8pp::class_<Group, v8pp::shared_ptr_traits>::find_object(
            isolate, std::static_pointer_cast<Group>(ptr));
    if (!wrapped_obj.IsEmpty()) {
      return wrapped_obj;
    }
    return v8pp::class_<Group, v8pp::shared_ptr_traits>::reference_external(
        isolate, std::static_pointer_cast<Group>(ptr));
  }
};

Of course, most of that code can be extracted to a generic function to avoid repetition on every node of the hierarchy.

What I couldn't figure out is how to extract the RTTI out of the classes I'm wrapping, so it can be reused to create bindings of third-party class hierarchies, e.g. if wrapping existing libraries. Maybe the v8pp internals could make use of typeid to identify the correct subclass at runtime...

pmed / v8pp

Return std::shared_ptr of another wrapped class from a member function #139