Add custom deleter support for RefCounted

[phoad]

std::make_shared or std::make_unique supports custom deleters but yarpl::Reference don't.

Because we use Proxygen libraries, we use folly::DelayedDestruction as base class of Http2ClientStreamHandler and use this delayed destruction encapsulation to prevent premature object deletion. So, instead of calling "delete this" for destruction, it calls the custom deletion function, which doesn't perform deletion if a guard is holding a reference but the guard performs deletion later (delayed).

This will enable more scenarios to use yarpl::Reference instead of std::unique_ptr, std::shared_ptr.

[phoad]

I could be able to simulate folly::DelayedDestruction by yarpl::get_ref function and extending the class with yarpl::Refcounted.

[alexmalyshev]

Using yarpl::Refcounted for the folly::DelayedDestruction case works for me. I think custom deleter support for yarpl::Reference would be a nice thing to have, but it's not blocking anything.

[lexs]

Can we just switch to std::shared_ptr instead? yarpl::Reference doesn't buy as anything afaik.

[phoad]

The base class of Subscription and Subscriber is Refcounted. We don't use weak_ptr for cases and it might be causing cyclic references with using std::shared_ptr that Refcounted is used to cure. If the base is Refcounted, should it require Reference, as it calls inc/dec functions when deleted? It looks very similar to this: folly/io/DelayedDestructionBase.h. Perhaps we might incorporate that class.
@lehecka @somasun

[lexs]

I don't understand, Refcounted behaves exactly like how a shared_ptr behaves, they both implement reference counting. There are afaik no other features.

[vitaut]

I don't see much utility in Refcounted either. It just saves one integer (weak count) and looks like a premature optimization considering that we plan to use stack allocation for most of the objects in the future.

[alexmalyshev]

To be clear, it's not just shared_ptr, we'd need to bring back reactivesocket::EnableSharedFromThisBase because we use Refcounted with virtual inheritance and we need some equivalent of shared_from_this().

I kind of like having Reference be separate from shared_ptr to be honest. When I see shared_ptr values I'm not expecting them to be in a cycle, whereas with Reference I'm constantly worrying if I've broken all my cycles in the right place.

Either way I don't feel too strongly on the matter.

[lexs]

When I see shared_ptr values I'm not expecting them to be in a cycle, whereas with Reference I'm constantly worrying if I've broken all my cycles in the right place.

I don't understand this part, both can be cycles and it's a bad case in both?

We can just make Refcounted be EnableSharedFromThisBase and we're good right?

[alexmalyshev]

I don't understand this part, both can be cycles and it's a bad case in both?

Yes, both can, but my experience with shared_ptr is that generally it isn't used with cycles (for C++ code outside of rsocket). That's why I said I liked /not/ using shared_ptr in this way, and instead having a different class (Refcounted) be the horrible bad case.

We can just make Refcounted be EnableSharedFromThisBase and we're good right?

Yeah, pretty much.

[lexs]

@lehecka Can we go ahead with killing yarpl::Reference off?

@ragansa recently ran into issues with code mixing shared_ptr and yarpl::Reference producing hard to understand crashes. I would prefer yarpl::Reference not slowly take over our codebase when it doesn't bring much to the table.

[lehecka]

we don't have refcounted base class anymore..