NFD

As stated in #3361 note-5, there's a design choice of whether to use iterators or references as handles of table items.

My thinking is:

• When the callee can inspect the item, use a const reference.
• When the callee can modify the item itself but cannot delete it, use a mutable reference.
• When the callee can delete the item, use an iterator.

20160121 conference call agrees with the idea that iterators should be used in place of shared_ptrs.

I discussed with Zhuo about note-8 idea, and he agrees with this idea. I'll make the changes under #3205.

https://gerrit.named-data.net/2917 patchset3 adopts the new Strategy API from #3205, and completes FIB portion of this issue.
I will not move onto other tables until this Change is merge.

https://gerrit.named-data.net/2961 attaches unique_ptr<measurements::Entry> onto name tree entry, and changes Measurements MeasurementsAccessor APIs to return Entry& (non-nullable) or Entry* (nullable).

In Measurements::extendLifetime and Measurements::cleanup, nte != nullptr check is deleted, because in case the Entry& passed as argument is a dangling reference, calling NameTree::lookup(entry) is already undefined behavior.
This differs from the previous code in which a shared_ptr<measurements::Entry> elsewhere could keep the Entry alive after being detached from the name tree entry.

So far I have progressed through FIB, StrategyChoice, and Measurements. PIT is the last table left.

The plan is:

Change shared_ptr<Face> FaceRecord::m_face to Face&.

Having shared_ptr<Face> not only is a small performance issue, but also causes the lifetime of Face to be longer than necessary.

To avoid dangling reference, when a face is closed, the InRecord/OutRecord associated with this face should be deleted.
This would require a PIT enumeration which is expensive, but Fib::removeNextHopFromAllEntries is already doing a NameTree::fullEnumerate, and FIB+PIT enumeration can be implemented as a single enumeration over the NameTree.
I'll do the PIT enumeration changes in #3685.

Change shared_ptr<Face> parameters in PIT and PIT entry APIs to Face& or const Face&.

Keep shared_ptr<pit::Entry> available.

weak_ptr<pit::Entry> is widely used in strategies for timer callbacks. It's possible to eliminate those usages by introducing a single per-PIT-entry timer for strategy to use, but this would be a complicated change.
Therefore, PIT entry will continue be created and attached to name tree entry as shared_ptr<pit::Entry>.

Pass pit::Entry as const/mutable reference in forwarding.

This reduces the usage of shared_ptr<pit::Entry>.
pit::Entry should inherit from enable_shared_from_this, so that shared_ptr<pit::Entry> and weak_ptr<pit::Entry> can be obtained from the reference when strategy needs it.

To simplify strategy code that only wants a weak_ptr<pit::Entry>, C++17 enabled_shared_from_this<T>::weak_from_this() should be backported into ndn-cxx/util/backports.hpp.

Junxiao Shi wrote:

Pass pit::Entry as const/mutable reference in forwarding.

This reduces the usage of shared_ptr<pit::Entry>.
pit::Entry should inherit from enable_shared_from_this, so that shared_ptr<pit::Entry> and weak_ptr<pit::Entry> can be obtained from the reference when strategy needs it.

hmm... is it worth changing at this point though? I'm inclined to say no. Just keep passing a shared_ptr or weak_ptr by const&.

Junxiao Shi wrote:

Yes it's worth changing. There are only sparse use cases where shared_ptr<pit::Entry> and weak_ptr<pit::Entry> are needed.

But there are no benefits in changing from const shared_ptr<pit::Entry>& to pit::Entry&, unless the shared_ptr is removed completely and the ownership model changes from shared to exclusive, which is not happening for PIT entries. So, even if there was only one place where a shared/weak_ptr was needed, it's still not worthwhile to make this change.

When the ownership model is shared, it's always better to be explicit and pass a shared_ptr (possibly by const-ref) to a function that will (or may, in the const-ref case) keep a shared reference to the object. shared_from_this is ugly and hackish, and should never be used except in very few specific cases (e.g. obtaining a shared_ptr from naked this, as the function name itself suggests). What you're proposing is an abuse of that functionality.

Reply to note-19:

Semantic-wise, the sole owner of a pit::Entry is the name tree entry; strategy is allowed to retain weak_ptr<pit::Entry> only.
How about an approach similar to ndn-cxx:commit:5bb314a7ea86e915e3fc32796ec2f3745e712983 Key vs KeyImpl?

void MyStrategy::oneFunction(pit::Entry& pitEntry)
{
  pit::Handle hdl(pitEntry);
  scheduler::schedule(time::seconds(1), [=] { this->anotherFunction(hdl); });
}

void MyStrategy::anotherFunction(pit::Handle hdl)
{
  pit::Entry* pitEntry = hdl.get();
  if (pitEntry == nullptr) {
    // PIT entry is gone
    return;
  }

  // use PIT entry
}

pit::Handle internally keeps a weak_ptr<pit::Entry>, but neither shared_ptr<pit::Entry> nor weak_ptr<pit::Entry> is directly exposed to the strategy.

Junxiao Shi wrote:

pit::Handle internally keeps a weak_ptr<pit::Entry>, but neither shared_ptr<pit::Entry> nor weak_ptr<pit::Entry> is directly exposed to the strategy.

What's the benefit of not exposing shared/weak_ptr to the strategy? What are you trying to accomplish? It seems to me that it would only obfuscate the code. The only advantage I see is that it would insulate the various strategies from further API breaks, but I don't think that's a goal for now (we don't have a stable strategy API yet).

The only advantage I see is that it would insulate the various strategies from further API breaks

It's the other way around. Strategy API is designed to prevent mistakes in strategy implementations.
For example, a strategy does not have direct access to Measurements table, but only filtered access through MeasurementsAccessor.

Ok, but you still haven't explained why you don't want to directly expose a weak_ptr to the strategies... is it to prevent strategies from calling lock() on the weak_ptr and keeping the object alive?

is it to prevent strategies from calling lock() on the weak_ptr and keeping the object alive?

yes

Ok. Then we should make this Handle class the only type through which strategies can manage/inspect PIT entries, i.e. we should never provide direct access to naked references or pointers, unlike your example in note-20, for memory safety reasons. All operations must be performed through a Handle object, that internally lock()s its private weak_ptr and then forwards the call to the real Entry object. This seems to be exactly what pib::Key does.

Unfortunately this approach doesn't scale very well (a "forwarding" method on the frontend object is necessary for every "real" method), and requires adding overloads that take a Handle to all APIs invokable by strategies that currently take an Entry& or Entry*...

make this Handle class the only type through which strategies can manage/inspect PIT entries

Although it's an eventual goal to have proxy/restricted types on everything a strategy can get hold of, this is out of scope of this issue.

My only concern in this issue is the lifetime/ownership of pit::Entry.
note-20 solves this concern and is a step toward the eventual goal.

If you want a temporary solution, then using weak_ptr is a much safer and simpler (less code to maintain) approach. Just document that strategies are not allowed to call lock() and keep the resulting shared_ptr indefinitely.

Reply to note-28:

It's too easy for a strategy to make a mistake and retain shared_ptr<pit::Entry> unless I prevent them from getting a shared_ptr altogether.
A disallowed usage of shared_from_this is easily discoverable by glancing at the code, but a mistake of passing shared_ptr into bind or captured lambda isn't so obvious, and that's what note-20 is trying to avoid.

So you avoid the lesser evil (retaining shared_ptrs to PIT entries), but you encourage using dangerous and unsafe naked pointers, and you don't disallow keeping naked references and/or pointers to objects that can be deallocated at any time...!

Reply to note-30:

Keeping naked reference is already disallowed, although I could only enforce that through Doxygen for now.

I want a confirmation: can I proceed with note-20 design?

If not, this issue is completed, and PIT entry will be kept as shared_ptr<pit::Entry> everywhere.

Davide has threatened a CodeReview-1 if I introduce a pit::Handle class, and suggests:

Just use a weak_ptr... or a typedef for a weak_ptr so that we can later change it more easily to that "Handle" type that I was talking about in note-26. And simply document that locking the weak_ptr and keeping the returned shared_ptr beyond the execution of the strategy callback function is prohibited. In the same way that keeping a naked ref/ptr is already disallowed "only through doxygen" like you said in note-31.
In the future we can design and implement a proper wrapper class that prevents all these issues at compile time.

This implies anything similar to those listed in #3691-5 would not be usable.
There would be no benefit in passing weak_ptr<pit::Entry> to Strategy APIs because it always needs to call lock.

Therefore, I'd just adds some asserts to ensure no shared_ptr is retained, but pass the parameter as const shared_ptr<pit::Entry>&.