NFD

I basically agree with this issue.

There's one problem to consider: is there a situation that a socket could be shared across multiple faces?

Ethernet unicast faces on the same NIC may need to share the pcap handle in order to send packets, while receiving is dispatched by the channel.

In those situations, shared_ptr shall be kept.

There's no such thing as a unicast Ethernet face (yet). In any case, why can't you have multiple pcap handles on the same interface?

make_unique needs a separate commit, in order to highlight its availability in COMMIT_MSG.

I don't care about whether socket and acceptor are separated.

Junxiao Shi wrote:

make_unique needs a separate commit, in order to highlight its availability in COMMIT_MSG.

Should it go in ndn-cxx?

make_unique needs a separate commit, in order to highlight its availability in COMMIT_MSG.

Should it go in ndn-cxx?

You can.
But if you put it into <ndn-cxx/common.hpp>, NFD is disallowed to using ndn::make_shared due to #2098.

So this needs a separate header in ndn-cxx/util in order to allow usage from NFD and other projects.

Just to records the reason why make_unique could be necessary: http://herbsutter.com/2013/05/29/gotw-89-solution-smart-pointers/ (question no.3)

(Unrelated to my previous comment) Alex says, about the new unique_ptr<socket> member variable:

honestly, i don't like this to be member variable, as it is just a transitional value. This change prevents any future optimization with thread pooling.
In different words. My opinion is that class members should be only about state that is inherent to the abstraction (channel). This socket that will belong to the Face should not belong (at any stage of the process) to the channel.

What do you propose then?

Right now, the socket is not really owned by anyone, it is kept alive only because it is bound to the handler and Asio happens to keep a copy of the handler. Asio doc clearly states:

Ownership of the socket object is retained by the caller, which must guarantee that it is valid until the handler is called.

Strictly speaking, I'm not sure if our current code satisfies this requirement.

What do you propose then?

I wish it was possible to just use unique_ptr for bind call, so the socket is just owned by bind until it is finally assigned to the face. If it is not possible (and I'm guessing that it is not), so I would prefer keeping shared_ptr, at least for the bind phase...

Strictly speaking, I'm not sure if our current code satisfies this requirement.

Yes, I know a few places where code doesn't satisfy this requirement. I just don't feel good about introducing new such places...

Alex Afanasyev wrote:

Yes, I know a few places where code doesn't satisfy this requirement. I just don't feel good about introducing new such places...

My patch doesn't introduce any new violations, in fact it may fix some.

Replying to a previous concern from Alex:

honestly, i don't like this to be member variable, as it is just a transitional value. This change prevents any future optimization with thread pooling.

I don't see how the proposed change prevents using a thread pool. Even with a thread pool (and one io_service per thread) you will still have one single acceptor and one single async_accept outstanding at any given time. In other words, the channel will still have to deal with at most one socket at a time. Therefore keeping a unique_ptr<socket> as a member variable of the channel doesn't impose any additional limitations.

Davide, I got the point and ok with the approach you're taking. It's just felt a little strange as I forgot that there is exactly one async_accept at a given time.

I hope there are no issues with socket move semantics in older boost (1.48).

Alex Afanasyev wrote:

I hope there are no issues with socket move semantics in older boost (1.48).

According to http://www.boost.org/doc/libs/1_57_0/doc/html/boost_asio/history.html , move semantics for sockets and POSIX descriptors is available since "Asio 1.6.0 / Boost 1.47".

Great. I just worry a little bit that this list not necessarily covers some temporary issues that could have existed at some point.

I'm still waiting for suggestions on the problem outlined in note-9

I think I suggested (may be on gerrit) to extend constructor parameters for UnixStreamFace.

Another not so nice way is to create setLocalUri/setRemoteUri methods.

Yet another, customize StreamFace constructor for each protocol type and do faceUri assignments there

#1912 is irrelevant: it's about passing shared_ptr by reference, not about usage of shared_ptr.

Well, "passing shared_ptr by reference" is a "usage of shared_ptr"... Moreover, in the discussion that developed in #1912 we agreed to follow Sutter's guidelines, and this task is very related to that (don't use shared_ptr if there's no shared ownership; use by-value unique_ptr or move semantics for "sinks"). Anyway, I don't really care...