Refactor reliable socket implementation

[scrye]

Decoupled streaming component of reliable socket from message parsing.
This allows the server side of the connection to parse the messages, and
simplifies the transition to SEQPACKET sockets.

---

This change is 

[lukedirtwalker]

Reviewed 13 of 13 files at r1.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @scrye and @sgmonroy)

---

go/lib/sock/reliable/packetizer.go, line 43 at r1 (raw file):

	for {
		packet := r.haveNextPacket(r.data)
		if packet != nil {

Could be merged into the previous line.

---

go/lib/sock/reliable/packetizer.go, line 80 at r1 (raw file):

// haveNextPacket returns a slice with the next packet in b, or nil, if a full
// packet is not available.
// is the case, the second return value contains the length of the packet.

The last line no longer applies.

---

go/lib/sock/reliable/util.go, line 20 at r1 (raw file):

const (
	ErrNoAddress             = "no address found"

I think it would make sense to move those into the errors.go go file.

---

go/lib/sock/reliable/util.go, line 35 at r1 (raw file):

func getAddressInformation(address *net.UDPAddr) (AddressType, int) {
	c := getAddressType(address)

Nit: what does c stand for here? I think t might apply better?

---

go/lib/sock/reliable/util.go, line 43 at r1 (raw file):

		return AddressTypeNone
	}
	if address.IP.To4() != nil {

return getIPAddressType(address.IP)

---

go/lib/sock/reliable/util.go, line 50 at r1 (raw file):

func getIPAddressType(ip net.IP) AddressType {
	if ip.To4() != nil {

Should we handle nil ip values?

---

go/lib/sock/reliable/util.go, line 81 at r1 (raw file):

func (c AddressType) AddressLength() int {
	switch c {
	case AddressTypeNone:

This case is not strictly needed, it would be covered by the default.

---

go/lib/sock/reliable/util.go, line 93 at r1 (raw file):

func (c AddressType) PortLength() int {
	switch c {

Could be simplified to:

	switch c {
	case AddressTypeIPv4, AddressTypeIPv6:
		return 2
	default:
		return 0
	}

But I'm also fine with the current version

---

go/lib/sock/reliable/util.go, line 107 at r1 (raw file):

// IsValid returns true only for address types that are defined.
func (c AddressType) IsValid() bool {
	if c == AddressTypeNone || c == AddressTypeIPv4 || c == AddressTypeIPv6 {

the if is not really needed you can just return c == AddressTypeNone || c == AddressTypeIPv4 || c == AddressTypeIPv6

[sustrik]

---

go/lib/sock/reliable/frame.go, line 57 at r1 (raw file):

}

// Frame describes the wire format of the reliable socket framing protocol.

Fro a non-insider this entire thing is very confusing. It's "reliable" socket. But what's that? There's nothing about that in the SCION book. In what sense is it reliable? Is it in an overlay protocol? A core SCION protocol? There's "Frame" but that's a pretty generic name that doesn't give any hints. Same thing with "cookie", "address" etc. I would suggest adding a short README to the folder explaining what's going on here.

[sgmonroy]

Reviewable status: all files reviewed, 22 unresolved discussions (waiting on @scrye)

---

go/lib/sock/reliable/frame.go, line 35 at r1 (raw file):

	f.AddressType = byte(getAddressType(p.Address))
	f.Length = uint32(len(p.Payload))
	if p.Address != nil {

can you send a frame without an Address if you have set up a proper AddressType?

---

go/lib/sock/reliable/registration.go, line 100 at r1 (raw file):

// RegistrationMessage is the wire format for a SCION Dispatcher registration
// message.
type RegistrationMessage struct {

Maybe this and other "internal" types should be private and not public

---

go/lib/sock/reliable/registration.go, line 156 at r1 (raw file):

		l.BindData = &bindData
	}
	switch len(b[13+addressLength+l.BindData.length():]) {

I would calculate the offset in its own variable, so it is clear that in case 2 you are using the same offset instead of something potentially slightly different.

---

go/lib/sock/reliable/registration.go, line 159 at r1 (raw file):

	case 0:
		return nil
	case 1:

you can get rid of this case

---

go/lib/sock/reliable/registration.go, line 169 at r1 (raw file):

}

type BindAddressLayer struct {

This could be more generic address struct used for both mandatory address and optional bind address

---

go/lib/sock/reliable/registration.go, line 176 at r1 (raw file):

func (l *BindAddressLayer) SerializeTo(b []byte) (int, error) {
	if len(b) < 3 {

should you not compare here against l.length() instead of 3?

---

go/lib/sock/reliable/reliable.go, line 148 at r1 (raw file):

// To check for timeout errors, type assert the returned error to *net.OpError and
// call method Timeout().
func RegisterTimeout(dispatcher string, ia addr.IA, public, bind *addr.AppAddr, svc addr.HostSVC,

bind should probably be an OverlayAddr

---

go/lib/sock/reliable/reliable.go, line 151 at r1 (raw file):

	timeout time.Duration) (*Conn, uint16, error) {

	publicUDP := createUDPAddr(public)

should check that public is not SVC

---

go/lib/sock/reliable/reliable.go, line 241 at r1 (raw file):

	var publicAddress *net.UDPAddr
	if dst != nil {
		overlayAddr := dst.(*overlay.OverlayAddr)

Just a note: it is becoming clear that OverlayAddr really is UDPAddr, and in the medium term future an IP only overlay with IPAddr.
It begs the question of OverlayAddr usability. It is very unlikely that we would ever support anything else for endhosts.

---

go/lib/sock/reliable/reliable.go, line 245 at r1 (raw file):

			publicAddress = &net.UDPAddr{
				IP:   overlayAddr.L3().IP(),
				Port: int(overlayAddr.L4().Port()),

L4 could be NIL

---

go/lib/sock/reliable/reliable.go, line 310 at r1 (raw file):

	}
	return &net.UDPAddr{
		IP:   address.L3.IP(),

This would be ok if there is a previous check for IPv4/IPv6 . None and SVC would return NIL

---

go/lib/sock/reliable/util.go, line 65 at r1 (raw file):

}

type AddressType byte

Not sure if using HostAddrType here would be better or it would end up with more bloat checking for unsupported values...

[scrye]

Reviewable status: 7 of 15 files reviewed, 21 unresolved discussions (waiting on @lukedirtwalker, @sustrik, @sgmonroy, and @scrye)

---

go/lib/sock/reliable/frame.go, line 35 at r1 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

can you send a frame without an Address if you have set up a proper AddressType?

The only frame without address that is currently valid is AddressType 0 (None). It is used for the registration message.

Code can create such a frame by passing in a nil Address in the OverlayPacket.

---

go/lib/sock/reliable/frame.go, line 57 at r1 (raw file):

Previously, sustrik (Martin Sustrik) wrote…

Fro a non-insider this entire thing is very confusing. It's "reliable" socket. But what's that? There's nothing about that in the SCION book. In what sense is it reliable? Is it in an overlay protocol? A core SCION protocol? There's "Frame" but that's a pretty generic name that doesn't give any hints. Same thing with "cookie", "address" etc. I would suggest adding a short README to the folder explaining what's going on here.

I agree regarding the confusing part.

The package level documentation here https://godoc.org/github.com/scionproto/scion/go/lib/sock/reliable includes answers to some of your questions, but I agree more documentation would be useful. However, we will rework a lot of the reliable socket aspects once we have a go dispatcher, so the documentation would quickly be outdated if we wrote it now.

---

go/lib/sock/reliable/packetizer.go, line 43 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Could be merged into the previous line.

Done.

---

go/lib/sock/reliable/packetizer.go, line 80 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

The last line no longer applies.

Done.

---

go/lib/sock/reliable/registration.go, line 100 at r1 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

Maybe this and other "internal" types should be private and not public

Done.

---

go/lib/sock/reliable/registration.go, line 156 at r1 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

I would calculate the offset in its own variable, so it is clear that in case 2 you are using the same offset instead of something potentially slightly different.

Done.

---

go/lib/sock/reliable/registration.go, line 159 at r1 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

you can get rid of this case

Done.

---

go/lib/sock/reliable/registration.go, line 169 at r1 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

This could be more generic address struct used for both mandatory address and optional bind address

Done.

---

go/lib/sock/reliable/registration.go, line 176 at r1 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

should you not compare here against l.length() instead of 3?

Done.

---

go/lib/sock/reliable/reliable.go, line 148 at r1 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

bind should probably be an OverlayAddr

Done.

---

go/lib/sock/reliable/reliable.go, line 151 at r1 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

should check that public is not SVC

Done.

---

go/lib/sock/reliable/reliable.go, line 245 at r1 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

L4 could be NIL

Done.

---

go/lib/sock/reliable/reliable.go, line 310 at r1 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

This would be ok if there is a previous check for IPv4/IPv6 . None and SVC would return NIL

Done.

---

go/lib/sock/reliable/util.go, line 20 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

I think it would make sense to move those into the errors.go go file.

Done.

---

go/lib/sock/reliable/util.go, line 43 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

return getIPAddressType(address.IP)

Done.

---

go/lib/sock/reliable/util.go, line 50 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Should we handle nil ip values?

Well, getAddressType can handle additional address types.

Here, if we get nil there's nothing we can do, as this can either be an ipv4 or an ipv6 address. If it's an ipv6 address and it's nil, the caller has to deal with it.

---

go/lib/sock/reliable/util.go, line 65 at r1 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

Not sure if using HostAddrType here would be better or it would end up with more bloat checking for unsupported values...

Done, it's a bit cleaner, but I still needed a few custom validators.

---

go/lib/sock/reliable/util.go, line 81 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

This case is not strictly needed, it would be covered by the default.

Done.

---

go/lib/sock/reliable/util.go, line 93 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Could be simplified to:

	switch c {
	case AddressTypeIPv4, AddressTypeIPv6:
		return 2
	default:
		return 0
	}

But I'm also fine with the current version

Done.

---

go/lib/sock/reliable/util.go, line 107 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

the if is not really needed you can just return c == AddressTypeNone || c == AddressTypeIPv4 || c == AddressTypeIPv6

Done.

[lukedirtwalker]

Reviewed 8 of 8 files at r2.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @sgmonroy and @scrye)

---

go/lib/sock/reliable/util.go, line 51 at r2 (raw file):

func getAddressLength(t addr.HostAddrType) int {
	n, _ := addr.HostLen(t)

I don't like this. What if the function returns an error? 👹

[scrye]

Reviewable status: 13 of 15 files reviewed, 13 unresolved discussions (waiting on @lukedirtwalker, @sgmonroy, and @scrye)

---

go/lib/sock/reliable/util.go, line 51 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

I don't like this. What if the function returns an error? 👹

It is intended for an error to be converted to 0 here (callers always check for valid address types first), and 0 for something that cannot be serialized is fine.

It's not great, but throwing an error here is going to create a code path that never gets triggered.

[lukedirtwalker]

Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @sgmonroy and @scrye)

[sgmonroy]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @scrye)

---

go/lib/sock/reliable/reliable.go, line 245 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Done.

change missed?

[scrye]

---

go/lib/sock/reliable/reliable.go, line 241 at r1 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

Just a note: it is becoming clear that OverlayAddr really is UDPAddr, and in the medium term future an IP only overlay with IPAddr.
It begs the question of OverlayAddr usability. It is very unlikely that we would ever support anything else for endhosts.

I agree. I've opened #2259 to track this discussion.

[scrye]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sgmonroy)

---

go/lib/sock/reliable/reliable.go, line 245 at r1 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

change missed?

Done.

[sgmonroy]

Reviewed 2 of 2 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved

[sgmonroy]

Reviewed 6 of 13 files at r1, 6 of 8 files at r2, 1 of 2 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

[sgmonroy]

Reviewable status: complete! all files reviewed, all discussions resolved