Project

General

Profile

Actions

Feature #2989

closed

Minimal UDP permanent face

Added by Junxiao Shi over 8 years ago. Updated over 8 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Faces
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:
5.00 h

Description

Implement a minimal UDP permanent face.

In this minimal implementation,

  • UdpFace constructor accepts FacePersistency persistency parameter in place of bool isOnDemand.
  • If persistency == FacePersistency::PERMANENT, the face ignores all socket errors after successful creation.
    Errors during the initial face creation should be reported as usual.
  • There is no UP/DOWN state transitions.
    An operator who decides to use a permanent face must take care to choose a strategy that is able to try multiple faces (ie. not best-route).

This Feature also includes the following changes:

  • Face::isOnDemand and Face::setOnDemand are replaced with Face::getPersistency and Face::setPersistency (also, bool m_isOnDemand is replaced with FacePersistency m_persistency); all usages of those methods shall be replaced
  • ProtocolFactory::createFace needs a FacePersistency persistency parameter.
    If a ProtocolFactory subclass does not support the specified persistency, throw ProtocolFactory::Error.
  • UdpFactory::createFace should support FacePersistency::PERSISTENT and FacePersistency::PERMANENT.
  • FaceManager::createFace passes FacePersistency::PERSISTENT to ProtocolFactory::createFace.

To allow an operator to use UDP permanent faces, management and nfdc changes are needed, but they are not part of this issue.


Related issues 3 (0 open3 closed)

Related to NFD - Task #2491: Design permanent facesClosedJunxiao Shi

Actions
Related to NFD - Task #2696: Document the behavior of persistent and on-demand facesClosedJunxiao Shi

Actions
Blocks NFD - Feature #2991: FaceManager: specify face persistencyClosedYukai Tu

Actions
Actions #1

Updated by Junxiao Shi over 8 years ago

  • Related to Task #2491: Design permanent faces added
Actions #2

Updated by Junxiao Shi over 8 years ago

  • Blocks Feature #2991: FaceManager: specify face persistency added
Actions #3

Updated by Davide Pesavento over 8 years ago

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.

Actions #4

Updated by Junxiao Shi over 8 years ago

  • Description updated (diff)

All socket errors must be ignored, because the goal is to allow the face operate unattended.

However, errors are allowed during creation, as long as those errors are reported before FaceCreatedCallback is invoked.

For now, this means if socket or connect fails, the face will be closed.
After #1285, the face can also be closed if tunnel authentication fails.

Actions #5

Updated by Junxiao Shi over 8 years ago

  • Description updated (diff)
Actions #6

Updated by Junxiao Shi over 8 years ago

  • Assignee set to Yukai Tu

20150629 conference call approves this issue, and decides to assign it to @Yukai.

Actions #7

Updated by Yukai Tu over 8 years ago

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?

Actions #8

Updated by Junxiao Shi over 8 years ago

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.

Actions #9

Updated by Yukai Tu over 8 years ago

UdpFactory don't need to accept persistency parameter?

Actions #10

Updated by Junxiao Shi over 8 years ago

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.

Actions #11

Updated by Davide Pesavento over 8 years ago

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.

Actions #12

Updated by Junxiao Shi over 8 years ago

Answer to note-11:

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

Using multiple commits for the same issue is acceptable.

Actions #13

Updated by Davide Pesavento over 8 years ago

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)

Actions #14

Updated by Junxiao Shi over 8 years ago

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.

Actions #15

Updated by Davide Pesavento over 8 years ago

Ok got it now.

Actions #16

Updated by Yukai Tu over 8 years ago

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.
Actions #17

Updated by Junxiao Shi over 8 years ago

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

Actions #18

Updated by Yukai Tu over 8 years ago

  • Status changed from New to In Progress
Actions #19

Updated by Junxiao Shi over 8 years ago

  • Assignee changed from Yukai Tu to Junxiao Shi
  • % Done changed from 0 to 40

http://gerrit.named-data.net/2185

I notice Face::setOnDemand is problematic and needs a change.

I'll update the design for this.

Actions #20

Updated by Junxiao Shi over 8 years ago

  • Description updated (diff)
  • Assignee changed from Junxiao Shi to Yukai Tu
  • Estimated time changed from 3.00 h to 5.00 h

As recommended in note-11, the Feature should be completed in 3 commits:

  1. face: replace isOnDemand with FacePersistency
* replace `Face::isOnDemand` and `Face::setOnDemand` with API based on `Face::getPersistency` and `Face::setPersistency`
* make necessary changes in subclasses and callers
  1. face: ProtocolFactory::createFace with FacePersistency
* in `ProtocolFactory::createFace`, add `persistency` parameter
* make necessary changes in subclasses and callers
* ALL subclasses, including `UdpFactory`, shall throw exception when `persistency != PERSISTENT` at this commit
  1. face: minimal UDP permanent face
* change `UdpFace` constructor to accept `persistency`
* change `UdpFactory::createFace` to allow `PERMANENT`
* implement minimal UDP permanent face semantics in `UdpFace`
Actions #21

Updated by Yukai Tu over 8 years ago

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?

Actions #22

Updated by Junxiao Shi over 8 years ago

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.

Actions #23

Updated by Yukai Tu over 8 years ago

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’
Actions #24

Updated by Junxiao Shi over 8 years ago

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.

Actions #25

Updated by Yukai Tu over 8 years ago

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

Actions #26

Updated by Davide Pesavento over 8 years ago

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

Actions #27

Updated by Yukai Tu over 8 years ago

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

Actions #28

Updated by Davide Pesavento over 8 years ago

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.

Actions #29

Updated by Junxiao Shi over 8 years ago

  • Related to Task #2696: Document the behavior of persistent and on-demand faces added
Actions #30

Updated by Yukai Tu over 8 years ago

How about adding ignore() into BaseFixture?

Actions #31

Updated by Yukai Tu over 8 years ago

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?

Actions #32

Updated by Davide Pesavento over 8 years ago

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.

Actions #33

Updated by Junxiao Shi over 8 years ago

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.

Actions #34

Updated by Davide Pesavento over 8 years ago

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

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

Actions #35

Updated by Yukai Tu over 8 years ago

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.

Actions #36

Updated by Davide Pesavento over 8 years ago

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?

Actions #37

Updated by Alex Afanasyev over 8 years ago

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

To avoid reimplementing dst/port-based dispatch manually.

Actions #38

Updated by Davide Pesavento over 8 years ago

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.

Actions #39

Updated by Junxiao Shi over 8 years ago

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

Actions #40

Updated by Yukai Tu over 8 years ago

How can I update NFD Developer Guide? Thanks.

Actions #41

Updated by Junxiao Shi over 8 years ago

  • Description updated (diff)
  • Status changed from In Progress to Closed
  • % Done changed from 40 to 100

Documentation work is moved to #3173 because face refactoring is coming soon.

Actions

Also available in: Atom PDF