NFD

Would it be better if find has 2 callbacks, one for success and one for failure? This would simplify dispatching in Forwarder:

typedef std::function<void(const Data&)> CacheHitCallback;
typedef std::function<void()> CacheMissCallback;
void asyncFind(Interest interest, CacheHitCallback ch, CacheMissCallback cm) const;

I'm also suggesting to add "async" to explicitly indicate that the call is asyncrhornous.

Using two callbacks requires two binds or lambdas, which causes parameters such as references to PIT entry to be copied twice.

I plan to use a lambda inside incoming Interest pipeline as the single callback, and put a conditional in that lambda to enter hit or miss pipeline.

'async' is unnecessary because there isn't a synchronous operation.

Example: ndn::Face::expressInterest is asynchronous, but its name doesn't contain 'async'.

Using two callbacks requires two binds or lambdas, which causes parameters such as references to PIT entry to be copied twice.

This is a really bad argument, giving tons of other copyings that happen throughout Forwarder and Strategy classes.

I plan to use a lambda inside incoming Interest pipeline as the single callback, and put a conditional in that lambda to enter hit or miss pipeline.

Anyhow. I wouldn't object that hard, but it is less explicit than separate callback.

'async' is unnecessary because there isn't a synchronous operation.

Example: ndn::Face::expressInterest is asynchronous, but its name doesn't contain 'async'.

I kind of can go either way. However, expressInterest is not "getData", it as a phrase shows that we are "expressing Interests" for some data (for me, just a different way to say async). Also, you yourself were suggesting to be more explicit in other places. What's harm to be explicit here?

I would tend to agree with Alex on both points, although not strongly, so I'd be ok with either of the proposed APIs.

One question: what's the guaranteed lifetime of the "returned" pointer/reference? In other words, would a weak_ptr make sense?

Currently, the return Data packet is valid synchronously only.

This reminds me that return type needs to be shared_ptr:
a ContentStore may load a Data packet from flash drive but not retain it in memory.
In this case, a shared_ptr is necessary to delete the Data packet after Forwarder has done with it.

I disagree with 'async' naming when synchronous operation does not exist.

The function signature already indicates it's asynchronous.

I disagree with two callbacks due to reason stated in note-2.

Junxiao Shi wrote:

I disagree with two callbacks due to reason stated in note-2.

This is premature optimization and does not constitute a valid reason against the two-callbacks approach.

Junxiao Shi wrote:

This reminds me that return type needs to be shared_ptr:
a ContentStore may load a Data packet from flash drive but not retain it in memory.
In this case, a shared_ptr is necessary to delete the Data packet after Forwarder has done with it.

A possible alternative is guaranteeing the validity of the Data only for the duration of the callback. If the callback wants to retain the Data for a longer time, it must make a copy.

To support async, does that mean we have two threads, one for CS hit pipeline and another one for CS miss pipeline?

To support async, does that mean we have two threads, one for CS hit pipeline and another one for CS miss pipeline?

No, everything is still in one thread.

In this Feature, when void Cs::find(Interest interest, HitCallback hit, MissCallback miss) const is called, it synchronously performs the lookup, and immediately invokes either hit or miss callback.

This Feature is to change the API without changing how ContentStore works.

If a future ContentStore impl needs to run the lookup procedure in a separate thread, it shall ensure hit or miss is invoked in the main thread.

But how does this support async, still not quite understand about it.

For now it will be just a conceptual async. This separation by itself would allow us in the future to implement really async content store which will not needed to be fully synchronized with the forwarding pipelines.

Please reassign this issue to me after all code changes are completed and merged. I'll take care of updating NFD Developer Guide after the updates for #2185.

From Alex's review on commit:6d8e68c9f59cab507804709c83ae5622afb140bb

change prototype to typedef std::function<void(const Interest&)> MissCallback;

This is not symmetric with HitCallback.
Either pass Interest to both callbacks, or to neither.

Also, please explain why Interest should be passed to callbacks.

My suggestion was based entirely on the use case: the interest was already passed to the callback, but in an obscured (and error-prone) way by binding reference of the interest, lifetime of which is assumed to exist until the invocation.

I don't see how "symmetric" argument matters here. However, I think we should add Interest as a parameter to HitCallback as well. Though, we can add it later, when the need arises (if arises).

I disagree with having Interest in one callback but not the other. ContentStore should have a consistent API regardless of use case.

Either pass const Interest& in both callbacks, or keep the current API unchanged.

I prefer then that the interest is supplied to both callbacks.

https://gerrit.named-data.net/3652 improves the test suite.