Refactor reconnecting logic as a dispatcher service

[scrye]

This simplifies how reconnecting logic is added to connections inside
snet.

---

This change is 

[oncilla]

Reviewed 1 of 18 files at r1.
Reviewable status: 1 of 18 files reviewed, 2 unresolved discussions (waiting on @oncilla and @scrye)

---

go/lib/snet/snetproxy/network.go, line 63 at r1 (raw file):

		return nil, 0, err
	}
	reconnecter := pn.newReconnecterFromListenArgs(ia, public, bind, svc, timeout)

What is the difference between the listener and the reconnector.
Why do we need both?
The logic here is not obvious to me, and I don't really grasp what it is supposed to do.

AFAIC, it is only used for testing?

---

go/lib/snet/snetproxy/network.go, line 72 at r1 (raw file):

	f := func(timeout time.Duration) (net.PacketConn, uint16, error) {
		return pn.dispatcher.RegisterTimeout(ia, public, bind, svc, timeout)

I do not see how this guarantees that you will get the same port after the initial call.
Please point me to or add a unit test that covers that a subsequent request is for the same port.

[scrye]

Reviewable status: 0 of 18 files reviewed, 2 unresolved discussions (waiting on @oncilla and @scrye)

---

go/lib/snet/snetproxy/network.go, line 63 at r1 (raw file):

Previously, Oncilla wrote…

What is the difference between the listener and the reconnector.
Why do we need both?
The logic here is not obvious to me, and I don't really grasp what it is supposed to do.

AFAIC, it is only used for testing?

In the old code, an initial connection attempt is needed to reserve resources (in this case, the port), and future attempts try to get those same resources.

In the refactor I introduced a bug where the resource allocation wasn't used in the second call, so thanks for spotting this, really good catch!

---

go/lib/snet/snetproxy/network.go, line 72 at r1 (raw file):

Previously, Oncilla wrote…

I do not see how this guarantees that you will get the same port after the initial call.
Please point me to or add a unit test that covers that a subsequent request is for the same port.

It wasn't taken into account, great catch! Fixed.

[scrye]

---

go/lib/snet/snetproxy/conn.go, line 169 at r2 (raw file):

		return nil, 0, err
	}
	_, oldPort := conn.getConnAndPort()

Note to self: refactor this into just getPort().

[oncilla]

Reviewed 10 of 18 files at r1, 5 of 7 files at r2, 5 of 5 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @scrye)

---

go/lib/snet/snetproxy/conn.go, line 164 at r3 (raw file):

// Reconnect is only used internally and should never be called from outside
// the package.
func (conn *ProxyConn) Reconnect() (net.PacketConn, uint16, error) {

Why does Reconnect expose a port?

---

go/lib/snet/snetproxy/conn.go, line 261 at r3 (raw file):

}

func (conn *ProxyConn) setConn(newConn net.PacketConn, newPort uint16) {

newPort is never used

---

go/lib/snet/snetproxy/network_test.go, line 49 at r3 (raw file):

					RegisterTimeout(localAddr.IA, newExpectedAddr, bindAddr, svc, timeout).
					Return(mockConn, uint16(80), nil)
				Convey("reconnect must not return error.", func() {

This this does never check whether an error has occurred.

[scrye]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @oncilla)

---

go/lib/snet/snetproxy/conn.go, line 164 at r3 (raw file):

Previously, Oncilla wrote…

Why does Reconnect expose a port?

It was used in checks that are no longer necessary. Removed.

---

go/lib/snet/snetproxy/conn.go, line 261 at r3 (raw file):

Previously, Oncilla wrote…

newPort is never used

Done.

---

go/lib/snet/snetproxy/network_test.go, line 49 at r3 (raw file):

Previously, Oncilla wrote…

This this does never check whether an error has occurred.

Text is wrong, it's not the error I'm testing for. Removed.

[oncilla]

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

[oncilla]

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