Task #3088
closedRefactor Face as LinkService+Transport
Added by Junxiao Shi over 9 years ago. Updated about 9 years ago.
100%
Description
Refactor Face
to support NDNLPv2 link service and permanent faces.
High-level design: https://gist.github.com/yoursunny/22a7d53711b4e9bc198a
This issue implements:
LinkService
andTransport
base classes- minimal
GenericLinkService
that only does Interest/Data/Nack <=> LpPacket encoding and decoding - non-inheritable
Face
class that combines aLinkService
and aTransport
; to avoid conflicts with oldnfd::Face
, this class is temporarily namedLpFace
sendNack
method andonReceiveNack
signal on oldnfd::Face
, so that forwarding can use them; innfd::Face
base class, these do nothingLpFaceWrapper
class which is an adapter that inherits from oldnfd::Face
but wraps anLpFace
This is an initial step toward a Face that supports NDNLPv2 and full-featured permanent face.
Files
IMG_20150825_234945754.jpg (3 MB) IMG_20150825_234945754.jpg | Unicast Logic | Eric Newberry, 08/25/2015 11:53 PM | |
IMG_20150825_234955350.jpg (2.99 MB) IMG_20150825_234955350.jpg | Multicast Logic | Eric Newberry, 08/25/2015 11:54 PM |
Updated by Junxiao Shi over 9 years ago
- Related to Feature #2222: NDNLPv2 design: link service added
Updated by Junxiao Shi over 9 years ago
- Assignee set to Eric Newberry
Eric can look at the high-level design, and also the implementation of NFD Face System.
Pay attention to:
- Constructor and initialization logic of each
Face
subclass. - How
Face
subclass are created in eachChannel
andProtocolFactory
subclass.
The design in #2222 note-40 hasn't been approved. "Design initialization logic for every Socket
type" can start now, but don't go beyond that.
Updated by Junxiao Shi over 9 years ago
- Description updated (diff)
The purpose of NullLinkService
is to serve as a placeholder of GenericLinkService
.
It's very simple to implement because it only does a simple .wireEncode
or .wireDecode
+classify.
By using NullLinkService
, NFD would lose the ability to encode/decode LocalControlHeader on local links, and lose NDNLPv1 on EthernetFace.
These feature reductions are intentional.
Both will break applications, including NFD-RIB; IntegrationTests will not pass.
The rationale of having NullLinkService
is to allow this refactoring to be carried out without worrying about GenericLinkService
.
However, since it breaks IntegrationTests, the commit should be merged together with another commit that (1) implements GenericLinkService
and uses it in initialization logic (2) moves NullLinkService
into tests/
.
Ultimately, it's inevitable to have a breaking change to switch from NDNLPv1+LocalControlHeader to NDNLPv2, because they are incompatible with each other.
Updated by Eric Newberry over 9 years ago
- Status changed from New to In Progress
Updated by Eric Newberry over 9 years ago
What do you mean by "initialization logic"? I'm not certain what that term is referring to.
Updated by Junxiao Shi over 9 years ago
Answer to note-5:
When reading code, try to understand how a face is created, since the beginning.
A face creation is triggered by (a) loading configuration (multicast faces only), (b) accepting incoming connection, or (c) faces/create
management command.
"initialization logic" is a vague concept. It includes but is not limited to:
- create a socket (eg. TCP socket, pcap handle, not the
Socket
class) - initialize the socket (eg.
connect
, set socket options) - make a
Face
subclass instance usable with the socket; in new design, make aSocket
subclass instance usable
When you design new initialization logic and decide on the boundary between Socket
subclass and its caller (eg. Channel
subclass), keep in mind the Socket
must be able to recover from an error (eg. broken TCP connection needs reestablished) on its own (when it's configured to have persistency=permanent).
Updated by Junxiao Shi over 9 years ago
#2222 note-40 design has been approved. All items in this Task can proceed.
There is still possibility for Socket
to be renamed, but that would be a minor change without affecting structure.
Updated by Junxiao Shi over 9 years ago
- Blocks Feature #3104: NDNLPv2: GenericLinkService encoding/decoding added
Updated by Eric Newberry over 9 years ago
What is a NetworkAddress
, as referred to in the high-level design? Is it the address based upon the socket type (like IP address for TCP and UDP)?
Updated by Junxiao Shi over 9 years ago
Answer to note-10:
What is a
NetworkAddress
?
See #2222 note-45 for design intention.
In this Task, only remoteAddr
on incoming packets is necessary (it will be used in #3104 for reassembly);
localAddr
, and remoteAddr
on outgoing packets can be ignored.
Is it the address based upon the socket type (like IP address for TCP and UDP)?
Yes, but LinkService
shouldn't care about the socket type, so NetworkAddress
shall be a single type that can represent an address of any socket type.
Reference ns3::Address for how to design NetworkAddress
data structure.
Updated by Eric Newberry over 9 years ago
- File IMG_20150820_130627359_HDR.jpg added
- File IMG_20150820_130640494_HDR.jpg added
Attached are design documents for the initialization logic.
Updated by Davide Pesavento over 9 years ago
There's an error in UdpSocket
: we don't set the DF flag, we disable PMTU discovery, which has the opposite effect since it prevents the kernel from setting the DF flag.
Updated by Junxiao Shi over 9 years ago
- Description updated (diff)
I realize a problem in the structure of this issue: it requires the deletion of the old Face
class and all subclasses, which requires lots of changes in unit testing, and would produce a huge commit.
A gradual approach could be:
- Create the new
Face
class but name itLpFace
. - Add a
LpFaceWrapper
class that inherits from oldFace
as a wrapper/proxy ofLpFace
. Having a separate wrapper instead of makingLpFace
inherit from oldFace
ensures the purity ofLpFace
API. - One at a time: delete
XFace
, addXSocket
, modifyXFactory
andXChannel
to initializeLpFace(NullLinkService+XSocket)
and wrap it withLpFaceWrapper
before returning, update test suite to test eitherXSocket
orLpFace(NullLinkService+XSocket)
. - Finally, delete old
Face
andLpFaceWrapper
, renameLpFace
toFace
, modifyFactory
andChannel
subclasses to return newFace
without wrapping, and update affected code outside face system.
This approach suggests the usage of a feature branch because it needs about 7 commits.
Updated by Junxiao Shi over 9 years ago
20150821 conference call decides that we should prefer the gradual approach in note-14.
Updated by Eric Newberry about 9 years ago
- File IMG_20150823_013019172.jpg added
- File IMG_20150823_013032543.jpg added
Here is the first revision of the Socket initialization logic and a diagram with the specific roles of the individual classes and their relationships with each other.
Updated by Davide Pesavento about 9 years ago
I'm not sure how to read the second diagram. Is it a sort of chain of responsibility (who does what)? A timeline?
Be careful that in most cases it's actually the channel, not the face, that instantiates the Asio socket and then passes (moves) it to the face constructor. This is especially important (and inevitable) for stream-based transports, due to async_accept()
requirements. Conversely, it isn't technically needed for the UDP face/channel, if I remember correctly, but was implemented in the same way for consistency reasons.
Updated by Eric Newberry about 9 years ago
Davide Pesavento wrote:
I'm not sure how to read the second diagram. Is it a sort of chain of responsibility (who does what)? A timeline?
From what I understood from my meeting with Junxiao on Friday, it's a chain of instantiation, similar to the one about the existing Face system in the NFD Developer Guide. For example, for unicast the Factory creates a Channel, which then listens for incoming connections, creating a Face upon acceptance of the connection. I need to move the Socket creation into the Channel for unicast connections (see below).
Be careful that in most cases it's actually the channel, not the face, that instantiates the Asio socket and then passes (moves) it to the face constructor. This is especially important (and inevitable) for stream-based transports, due to
async_accept()
requirements. Conversely, it isn't technically needed for the UDP face/channel, if I remember correctly, but was implemented in the same way for consistency reasons.
I wasn't aware of this. I'll modify my design to fit this requirement.
Updated by Junxiao Shi about 9 years ago
Comment on note-16:
There shouldn't be TcpFace UdpFace UnixFace EthernetFace WebSocketFace
.
Quote issue description:
Implement a non-inheritable
Face
class that connects aLinkService
andSocket
.
There is only one non-inheritable Face
class. The behavior of a Face
is entirely defined by the LinkService
and Socket
used to construct it.
Updated by Eric Newberry about 9 years ago
- File IMG_20150825_234945754.jpg IMG_20150825_234945754.jpg added
- File IMG_20150825_234955350.jpg IMG_20150825_234955350.jpg added
Here is the latest revision of the initialization logic.
Updated by Eric Newberry about 9 years ago
- File deleted (
IMG_20150820_130627359_HDR.jpg)
Updated by Eric Newberry about 9 years ago
- File deleted (
IMG_20150820_130640494_HDR.jpg)
Updated by Eric Newberry about 9 years ago
- File deleted (
IMG_20150823_013019172.jpg)
Updated by Eric Newberry about 9 years ago
- File deleted (
IMG_20150823_013032543.jpg)
Updated by Junxiao Shi about 9 years ago
- Blocks Feature #3156: NACK in forwarding and best-route added
Updated by Alex Afanasyev about 9 years ago
- Blocks Feature #3160: Permit scope=local in WebSocketTransport added
Updated by Eric Newberry about 9 years ago
I have finished up to and including part 1 of note 14. Will a feature branch be used or should I submit the commit to the master branch?
Updated by Eric Newberry about 9 years ago
- Status changed from In Progress to Code review
Updated by Junxiao Shi about 9 years ago
- Status changed from Code review to In Progress
Initial Change is at http://gerrit.named-data.net/2423.
Status should remain as InProgress until the final Change is uploaded to Gerrit.
Updated by Junxiao Shi about 9 years ago
- Description updated (diff)
In "Implement a NullLinkService
that merely passes through packets between network layer and link layer" I meant a simple this->sendPacket(interest.wireEncode());
, but commit:c2ee303880e6bfd79c107a4cda2cc28194356409 is encoding it into LpPacket.
It's okay, but to be consistent NullLinkService
should send/receive NACK as well.
Updated by Junxiao Shi about 9 years ago
- Subject changed from Refactor Face to Refactor Face as LinkService+Transport
- Description updated (diff)
- Estimated time changed from 9.00 h to 3.00 h
20150904 meeting decides to split this into multiple smaller issues so each can be individually assigned.
Updated by Junxiao Shi about 9 years ago
- Blocks Feature #3165: UnixStreamTransport added
Updated by Junxiao Shi about 9 years ago
- Blocks Feature #3166: TcpTransport added
Updated by Junxiao Shi about 9 years ago
- Blocks Feature #3168: UdpTransport added
Updated by Junxiao Shi about 9 years ago
- Blocks Feature #3169: WebSocketTransport added
Updated by Junxiao Shi about 9 years ago
- Blocks Feature #3170: EthernetTransport added
Updated by Junxiao Shi about 9 years ago
- Description updated (diff)
On another thought, the SimpleLinkService
is already a subset of GenericLinkService
(that only has network-layer packet encoding functionality), so we should just call it GenericLinkService
.
Updated by Junxiao Shi about 9 years ago
- Description updated (diff)
Also, we need sendNack
and onReceiveNack
in old nfd::Face
, so that #3156 can use NACK right away.
Updated by Junxiao Shi about 9 years ago
- Blocks deleted (Feature #3160: Permit scope=local in WebSocketTransport)
Updated by Junxiao Shi about 9 years ago
- Blocked by Feature #3174: NACK counters added
Updated by Junxiao Shi about 9 years ago
- Priority changed from Normal to Urgent
Raising priority because this is blocking all other issues about NDNLPv2 and Nack.
Updated by Eric Newberry about 9 years ago
- Status changed from In Progress to Code review
Updated by Junxiao Shi about 9 years ago
- Blocked by deleted (Feature #3174: NACK counters)
Updated by Junxiao Shi about 9 years ago
- Blocks Feature #3177: LpFace counters added
Updated by Junxiao Shi about 9 years ago
- Blocks Feature #3179: EndpointId added
Updated by Junxiao Shi about 9 years ago
- Estimated time changed from 3.00 h to 5.00 h
Some rework is needed in this issue due to design extension in #2222 note-57.
Most importantly, LpFace
should support NFD_LOG_FACE_*
macros similar to the old Face, and those macros should be available in LpFace LinkService Transport
.
I suggest:
- Declare
FaceLogHelper
andNFD_LOG_FACE*
macros in face-log.hpp. - Specialize
FaceLogHelper
for oldFace
and its subclasses in face.hpp; specializeFaceLogHelper
forLpFace
and its subclasses in lp-face.hpp; specializeFaceLogHelper
forLinkService
and its subclasses in link-service.hpp; specializeFaceLogHelper
forTransport
and its subclasses in transport.hpp. NFD_LOG_FACE
would contain<< FaceLogHelper(*this) <<
to output the necessary message recommended in #2450.
Updated by Eric Newberry about 9 years ago
The design in #2222 note 57 appears to cause circular dependencies, as link-service.hpp needs to include lp-face.hpp and transport.hpp needs to include lp-face.hpp, but lp-face.hpp needs to include link-service.hpp and transport.hpp.
Updated by Junxiao Shi about 9 years ago
Answer to note-50:
- forward declare
LpFace
in link-service.hpp and transport.hpp - include lp-face.hpp in link-service.cpp and transport.cpp
Updated by Eric Newberry about 9 years ago
Transport::Packet doesn't work well with forward declarations. We could possibly work around this by removing Packet as an inner class and adding it as a normal class?
Updated by Junxiao Shi about 9 years ago
Answer to note-52:
lp-face.hpp and link-service.hpp can still include transport.hpp, so that Transport::Packet
is never forward-declared.
Updated by Junxiao Shi about 9 years ago
LpFaceWrapper
needs another getter (see #3168 note-23 for reason):
LpFace*
getLpFace();
Note: it's unnecessary to declare a const
version because there's no use case.
Updated by Junxiao Shi about 9 years ago
From Eric's post in http://gerrit.named-data.net/2423 patchset18:
I believe with this current design,
NFD_LOG_FACE
would need to be defined in each file, as FaceLogHelper requires a template argument.
No. As given in note-49, NFD_LOG_FACE*
macros appear in face-log.hpp only, and this header is included by face.hpp, transport.hpp, link-service.hpp.
Updated by Junxiao Shi about 9 years ago
- Blocks Task #3225: Refactor InternalFace added
Updated by Junxiao Shi about 9 years ago
- Status changed from Code review to Closed