Refactor snet writer; move path resolution outside main lock

[scrye]

This partially fixes #1702.

---

This change is 

[lukedirtwalker]

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @scrye)

---

go/lib/snet/writer.go, line 147 at r1 (raw file):

// WriteToSCION.
type connRemoteAddressResolver struct {
	remoteAddressResolver *remoteAddressResolver

It feels a bit heavy to have an extra object here ( for a single method). OTOH it makes the code quite clean.
Maybe we could try to move resolve to remoteAddressResolver and rename it's existing resolve method to resolveAddr ? But if you feel it is less nice feel free to leave it as is.

---

go/lib/snet/writer.go, line 158 at r1 (raw file):

	case connAddr != nil:
		return r.remoteAddressResolver.resolve(connAddr)
	default:

It's not directly obvious that in this case argAddr != nil maybe add a comment?

---

go/lib/snet/writer_test.go, line 21 at r1 (raw file):

	"testing"

	"github.com/golang/mock/gomock"

Fix the imports ;) also goconvey

---

go/lib/snet/internal/util/util.go, line 15 at r1 (raw file):

// limitations under the License.

package util

This only contains Pathsource why not call the package or at least the file like this?

---

go/lib/snet/internal/util/util.go, line 45 at r1 (raw file):

Passing in a nil resolver is

Why/Where is that useful?

[scrye]

Reviewable status: 2 of 6 files reviewed, 5 unresolved discussions (waiting on @lukedirtwalker)

---

go/lib/snet/writer.go, line 147 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

It feels a bit heavy to have an extra object here ( for a single method). OTOH it makes the code quite clean.
Maybe we could try to move resolve to remoteAddressResolver and rename it's existing resolve method to resolveAddr ? But if you feel it is less nice feel free to leave it as is.

Ah, good point, didn't even notice during refactoring how small this got. Done.

---

go/lib/snet/writer.go, line 158 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

It's not directly obvious that in this case argAddr != nil maybe add a comment?

Done.

---

go/lib/snet/writer_test.go, line 21 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Fix the imports ;) also goconvey

Done.

---

go/lib/snet/internal/util/util.go, line 15 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

This only contains Pathsource why not call the package or at least the file like this?

Done.

---

go/lib/snet/internal/util/util.go, line 45 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Passing in a nil resolver is

Why/Where is that useful?

When snet runs without a path resolver, it can still build the remote address resolution objects without having them deal with the fact that there is no source of paths.

So it moves error handling from deep insnet to the path source itself.

The alternative is to check if the PathSource is nil in the resolver. It depends on how one looks at the abstraction: does the path source have the responsibility of always providing a path, or does snet have the responsibility of checking it has a source, and invoking it (which in turn can eventually return a similar event, i.e., no paths). It just felt more natural to put it in the path source, but I don't have strong feelings on the matter.

[lukedirtwalker]

Reviewed 4 of 4 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved