cppgraphqlgen icon indicating copy to clipboard operation
cppgraphqlgen copied to clipboard

How to resolve objects not owned by shared_ptr?

Open gitmodimo opened this issue 2 years ago • 4 comments

Object models require me to pass shared_ptr to constructor. How do I pass object that is not owned by shred_ptr? Here is example schema fragment:

type MemberField{
   name: String!
}

type MyObject{
   member: MemberField!
}

And pseudocode:

struct MemberField{

   ...

}

struct MyObject: public std::enable_shared_from_this<MyObject>{

   ...

   service::AwaitableObject<std::shared_ptr<GQL::object::MemberField>> getMember(service::FieldParams&& params){
      return ???
   }
   MemberField member;//owned as member
}

For this simple case it is possible to return std::shared_ptr<MemberField>(shared_from_this(),&member); However such solutions does not scale with multiple nesting. I would need to keep weak_ptr from owning parent in all members. From what I understood of how it all works, parent model always outlives its child. So if I am correct it should be safe to pass not-owning pointer to resolver. I tried using fake shared ptr return std::shared_ptr<MemberField>(std::shared_ptr<void>{},&member); and it seems to be working. As long as parent object is owned by either resolver or otherwise nothing bad should happen?

Did I miss something that prohibits not owning model of child nodes? And if there is nothing that prohibits this shouldn't it be allowed to pass raw pointer(or reference) to model constructor?

gitmodimo avatar Jun 27 '22 11:06 gitmodimo

Interesting, I never noticed the aliasing contructor on std::shared_ptr before.👍🏼

Perhaps because of that, I would always be inclined to make the member a std::shared_ptr as well. I worry that using references (or pointers, which is equivalent for lifetime purposes) would be too error prone, but perhaps there could be an exception for the operation types which requires those to be wrapped in a std::shared_ptr.

I think it would also need to be possible to return a type-erased object wrapper by value then, and that version of the wrapper object would need to somehow make clear that it should never be stored in a std::shared_ptr which could out-live the reference to the contained type.

Alternatively, the resolvers could just flow a std::shared_ptr or std::weak_ptr for the root object down to all of the accessors in service::FieldParams. That wouldn't let you mix and match in your object hierarchy with lifetimes tied to other objects, but you could at least tie the lifetime of all of the aliased member std::shared_ptrs to the executing operation.

wravery avatar Jun 27 '22 18:06 wravery

I think it would also need to be possible to return a type-erased object wrapper by value then, and that version of the wrapper object would need to somehow make clear that it should never be stored in a std::shared_ptr which could out-live the reference to the contained type.

I don't think I understand the problem. How type-erase object can be returned instead of real object wrapper?

Alternatively, the resolvers could just flow a std::shared_ptr or std::weak_ptr for the root object down to all of the accessors in service::FieldParams. That wouldn't let you mix and match in your object hierarchy with lifetimes tied to other objects, but you could at least tie the lifetime of all of the aliased member std::shared_ptrs to the executing operation.

I've been there. It is a rabbithole of keeping stack of parent objects pushing new ones diving into graph and poping then on the way out.

PS. Migration guide is a bit misleading suggesting that shared_from_this is not essential.

Change your class declarations so that they no longer inherit from the generated object namespace classes. If you need the shared_from_this() method, you can replace that with std::enable_shared_from_this<T>.

Can I safely assume that wrapper of the parent outlives all children wrappers?

gitmodimo avatar Jun 27 '22 19:06 gitmodimo

I think it would also need to be possible to return a type-erased object wrapper by value then, and that version of the wrapper object would need to somehow make clear that it should never be stored in a std::shared_ptr which could out-live the reference to the contained type.

I don't think I understand the problem. How type-erase object can be returned instead of real object wrapper?

The real object::MemberField wrapper is the type-erased object in this case. I meant that AwaitableObject would need to accept a by-value version of the wrapper to guarantee there aren't any other std::shared_ptr references to it keeping it alive beyond the executing operation.

Alternatively, the resolvers could just flow a std::shared_ptr or std::weak_ptr for the root object down to all of the accessors in service::FieldParams. That wouldn't let you mix and match in your object hierarchy with lifetimes tied to other objects, but you could at least tie the lifetime of all of the aliased member std::shared_ptrs to the executing operation.

I've been there. It is a rabbithole of keeping stack of parent objects pushing new ones diving into graph and poping then on the way out.

Couldn't it all be chained to a single top-level std::shared_ptr so they all share the operation's lifetime? Come to think of it, you might even be able to use the service::RequestState for that without other modifications.

PS. Migration guide is a bit misleading suggesting that shared_from_this is not essential.

Change your class declarations so that they no longer inherit from the generated object namespace classes. If you need the shared_from_this() method, you can replace that with std::enable_shared_from_this.

It's required if you need shared_from_this(), but std::make_shared (or the std::shared_ptr constructors) should still work without it. I don't think it's essential in all cases.

Can I safely assume that wrapper of the parent outlives all children wrappers?

Sort of. The resolver won't hold onto it after it's returned and consumed, but if a user were caching the wrapper objects somewhere between operations with a std::shared_ptr, it might still outlive the reference. I don't think it's provably safe as long the wrapper object is itself in a std::shared_ptr.

wravery avatar Jun 27 '22 20:06 wravery

Couldn't it all be chained to a single top-level std::shared_ptr so they all share the operation's lifetime? Come to think of it, you might even be able to use the service::RequestState for that without other modifications.

Not all objects are owned by their parent(or by any single object). Graph is evolving in runtime. Some objects have direct parent (like to one held as class member) but others are not. Most of objects are actually held by shared_ptr and lifetime extension works nicely. However it seems a bit overkill to keep all members as shared_ptr even when logically their lifetime is tightly coupled with parent lifetime. Allowing members (MemberField in this example) outlive their parents breaks too many assumptions.

It's required if you need shared_from_this(), but std::make_shared (or the std::shared_ptr constructors) should still work without it. I don't think it's essential in all cases.

Oh. That makes sense.

Sort of. The resolver won't hold onto it after it's returned and consumed, but if a user were caching the wrapper objects somewhere between operations with a std::shared_ptr, it might still outlive the reference. I don't think it's provably safe as long the wrapper object is itself in a std::shared_ptr.

For now I think I will stick to fake pointers std::shared_ptr<MemberField>(std::shared_ptr<void>{},&member); until I find more elegant solution. I do not plan any wrapper caching. I consider wrappers as library internal objects.

gitmodimo avatar Jun 28 '22 09:06 gitmodimo

Couldn't it all be chained to a single top-level std::shared_ptr so they all share the operation's lifetime? Come to think of it, you might even be able to use the service::RequestState for that without other modifications.

I might try to build another sample that takes this approach and see how it turns out.

wravery avatar May 10 '23 17:05 wravery