NFD

Junxiao Shi wrote:

• If persistency == FacePersistency::PERMANENT, the face ignores socket errors.

Which errors are to be ignored and which ones are not? For example failures to create a socket should not be ignored in my opinion, as they are usually a symptom of a bigger problem.

• The face transmits packets whenever the underlying socket allows immediate transmission.

I have no idea what this sentence means.

Junxiao Shi wrote:

• ProtocolFactory::createFace needs a FacePersistency persistency parameter.
  If a ProtocolFactory subclass does not support the specified persistency, throw ProtocolFactory::Error.

Should I rewrite all subclass to support the persistency?

Reply to note-7:

In all ProtocolFactory subclasses except UdpFactory, the createFace method shall be rewritten to accept persistency parameter. If the argument value is not PERSISTENT, the implementation throws Error type of that subclass.

UdpFactory don't need to accept persistency parameter?

Answer to note-9:

UdpFactory::createFace certainly needs to accept persistency parameter, but it doesn't throw exception for PERMANENT. Instead:

UdpFactory::createFace should support FacePersistency::PERSISTENT and FacePersistency::PERMANENT.

Junxiao Shi wrote:

If the argument value is not PERSISTENT, the implementation throws Error type of that subclass.

if the value is not PERSISTENT? I guess you meant the opposite...

Also, I suggest to implement this feature with at least two separate changesets. First, refactor/change the existing code to use FacePersistency instead of bool. Then implement the actual permanent UDP face.

Answer to note-11:

I confirm note-8 is accurate, not the opposite.

Using multiple commits for the same issue is acceptable.

Junxiao Shi wrote:

I confirm note-8 is accurate, not the opposite.

What about on-demand faces then? (actually I didn't really mean the opposite in note 11... my point was that we should throw an error only if face persistency is permanent, except for udp unicast)

Answer to note-13:

ProtocolFactory::createFace is used to initiate an outgoing connection; FacePersistency::ON_DEMAND is for incoming connection only.
Previously, ProtocolFactory::createFace does not have a parameter to indicate whether the face is on-demand or persistent, and all faces are created as persistent.

This issue adds permanent face to UdpFactory::createFace.
It does not change that fact that on-demand is not supported by ProtocolFactory::createFace.

Ok got it now.

Just to make sure that I got it:

1. Rewrite ProtocolFactory::createFace to support a FacePersistency persistency parameter.
2. All ProtocolFactory subclasses should be rewrite. But only UdpFactory::createFace support FacePersistency::PERSISTENT and FacePersistency::PERMANENT.
3. Original onDemand system should be discarded.
4. Other subclasses only accept FacePersistency::PERSISTENT, passing FacePersistency::PERMANENT will throw Error.

I neither confirm nor deny note-16. Please refer to the issue description.

Junxiao Shi wrote:

* ALL subclasses, including `UdpFactory`, shall throw exception when `persistency != PERSISTENT` at this commit
* change `UdpFactory::createFace` to allow `PERMANENT`

If throw exception when persistency != PERSISTENT, how to allow PERMANENT?

Answer to note-21:

There is no conflict in note-20 design. The first quoted statement applies to the second commit, while the second quoted statement applies to the third commit which adds a new feature onto the second commit.

After cloning project from gerrit, I run ./waf configure --with-tests then ./waf, and it always builds fail. Is there any wrong with the codes?

../tests/core/segment-publisher.t.cpp: 在成员函数‘virtual std::size_t nfd::tests::SegmentPublisherTester<N>::generate(ndn::encoding::EncodingBuffer&)’中:
../tests/core/segment-publisher.t.cpp:76:81: 错误： ‘prependNonNegativeIntegerBlock’的实参不依赖模板参数，所以‘prependNonNegativeIntegerBlock’的声明必须可用 [-fpermissive]
../tests/core/segment-publisher.t.cpp:76:81: 附注： (如果您使用‘-fpermissive’，G++ 会接受您的代码，但是允许使用未定义的名称是不建议使用的风格)
../tests/core/segment-publisher.t.cpp: 在成员函数‘std::size_t nfd::tests::SegmentPublisherTester<N>::generate(ndn::encoding::EncodingBuffer&) [with long int N = 10000l, std::size_t = long unsigned int, ndn::encoding::EncodingBuffer = ndn::encoding::EncodingImpl<true>]’中:
../tests/core/segment-publisher.t.cpp:171:1：从此处实例化
../tests/core/segment-publisher.t.cpp:76:9: 错误： ‘prependNonNegativeIntegerBlock’在此作用域中尚未声明
../tests/core/segment-publisher.t.cpp:76:9: 附注： 建议的替代：
/usr/local/include/ndn-cxx/encoding/block-helpers.hpp:55:1: 附注：   ‘ndn::prependNonNegativeIntegerBlock’
../tests/core/segment-publisher.t.cpp: 在成员函数‘std::size_t nfd::tests::SegmentPublisherTester<N>::generate(ndn::encoding::EncodingBuffer&) [with long int N = 100l, std::size_t = long unsigned int, ndn::encoding::EncodingBuffer = ndn::encoding::EncodingImpl<true>]’中:
../tests/core/segment-publisher.t.cpp:171:1：从此处实例化
../tests/core/segment-publisher.t.cpp:76:9: 错误： ‘prependNonNegativeIntegerBlock’在此作用域中尚未声明
../tests/core/segment-publisher.t.cpp:76:9: 附注： 建议的替代：
/usr/local/include/ndn-cxx/encoding/block-helpers.hpp:55:1: 附注：   ‘ndn::prependNonNegativeIntegerBlock’
../tests/core/segment-publisher.t.cpp: 在成员函数‘std::size_t nfd::tests::SegmentPublisherTester<N>::generate(ndn::encoding::EncodingBuffer&) [with long int N = 10l, std::size_t = long unsigned int, ndn::encoding::EncodingBuffer = ndn::encoding::EncodingImpl<true>]’中:
../tests/core/segment-publisher.t.cpp:171:1：从此处实例化
../tests/core/segment-publisher.t.cpp:76:9: 错误： ‘prependNonNegativeIntegerBlock’在此作用域中尚未声明
../tests/core/segment-publisher.t.cpp:76:9: 附注： 建议的替代：
/usr/local/include/ndn-cxx/encoding/block-helpers.hpp:55:1: 附注：   ‘ndn::prependNonNegativeIntegerBlock’
../tests/core/segment-publisher.t.cpp: 在成员函数‘std::size_t nfd::tests::SegmentPublisherTester<N>::generate(ndn::encoding::EncodingBuffer&) [with long int N = 0l, std::size_t = long unsigned int, ndn::encoding::EncodingBuffer = ndn::encoding::EncodingImpl<true>]’中:
../tests/core/segment-publisher.t.cpp:171:1：从此处实例化
../tests/core/segment-publisher.t.cpp:76:9: 错误： ‘prependNonNegativeIntegerBlock’在此作用域中尚未声明
../tests/core/segment-publisher.t.cpp:76:9: 附注： 建议的替代：
/usr/local/include/ndn-cxx/encoding/block-helpers.hpp:55:1: 附注：   ‘ndn::prependNonNegativeIntegerBlock’

Reply to note-24:

It seems that you have conflicting versions of ndn-cxx and NFD.
Please ensure both are latest version and try again.

What should I do to rebase? I used command "git rebase" but it doesn't work.

Are you working on a separate branch? Did you pull from the upstream master branch before rebasing?

I working on the master branch. And git rebase command seems don't match the requirements. What command should I use?

Working on the master branch is a terrible idea as it doesn't let you work on multiple patchsets at the same time.

I believe you can use git pull --rebase on the master branch to fix the current situation (if you haven't already done a merge with upstream), but I strongly suggest using separate local branches next time.

How about adding ignore() into BaseFixture?

If the socket is closed or It receives the boost::asio::error::eof, should I reconnect to the remote endpoint? If it still fails, what should permanent face do?

Yukai Tu wrote:

If the socket is closed or It receives the boost::asio::error::eof, should I reconnect to the remote endpoint?

If the socket is closed, yes, it needs to be reopened and reconnected. If error::eof results in the socket being closed, then it should be reopened and reconnected as well, otherwise just ignore the eof and continue. This applies to all errors that do not leave the socket in a bad/inconsistent/unusable state.

If it still fails, what should permanent face do?

If what fails? The connection attempt?

Note that since UDP is connection-less, a connect operation does not in fact perform any network operations. Therefore I don't think that a connect on a UDP socket can actually fail for network problems. It can fail for "local" reasons (e.g. resource exhaustion), but in that case I believe retrying would be pointless and you should let the face fail. The issue description is not clear on this point however, so let's wait for Junxiao's comment.

It can fail for "local" reasons (e.g. resource exhaustion), but in that case I believe retrying would be pointless and you should let the face fail.

No, a permanent face can never fail. This is a strong guarantee.
It could be DOWN.

In the minimal implementation, there's no explicit DOWN state.
The face can be kept in an unusable state (not able to send or receive).
Before each attempt to send, retry connect.

Ok then. What if bind fails though? Or async_receive?

Btw, maybe we should use sendto/recvfrom instead of connect + send/receive?

Davide Pesavento wrote:

Ok then. What if bind fails though? Or async_receive?

If bind fails, we can ignore the error and reconnect until next attempt to send.

Btw, maybe we should use sendto/recvfrom instead of connect + send/receive?

Maybe 'connect' + 'send/receive' is acceptable.

Yukai Tu wrote:

Davide Pesavento wrote:

Ok then. What if bind fails though? Or async_receive?

If bind fails, we can ignore the error and reconnect until next attempt to send.

"reconnect" has nothing to do with bind. I'm talking about the receive path.

Btw, maybe we should use sendto/recvfrom instead of connect + send/receive?

Maybe 'connect' + 'send/receive' is acceptable.

Of course it's acceptable. The point is: which way is simpler? (connect, being an additional call, is potentially another source of errors).

@Alex/Junxiao: what were the original reasons to prefer connect + send/receive?

@Alex/Junxiao: what were the original reasons to prefer connect + send/receive?

To avoid reimplementing dst/port-based dispatch manually.

Alex Afanasyev wrote:

@Alex/Junxiao: what were the original reasons to prefer connect + send/receive?

To avoid reimplementing dst/port-based dispatch manually.

ok, makes sense. It's probably easier to keep the connect then.

All code changes are merged. Assignee also needs to update NFD Developer Guide before this issue can close.

How can I update NFD Developer Guide? Thanks.