Add client library for SVC resolution #2604

scrye · 2019-04-18T16:00:05Z

This change is

oncilla

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

go/lib/mocks/context/mock_context/context.go, line 5 at r1 (raw file):

// Package mock_context is a generated GoMock package.
package mock_context

I think this is taking mocking a bit to far.
I do not see the need to mock context, just use concrete values

go/lib/snet/router.go, line 32 at r1 (raw file):

// interested in spinning their own implementations.
type Router interface {
	Route(ctx context.Context, dst addr.IA) (Path, error)

In the spirit of road to 100 (https://goreportcard.com/report/github.com/scionproto/scion#golint)
Add a comment

go/lib/snet/router.go, line 40 at r1 (raw file):

// access to the raw internals.
type Path interface {
	OverlayNextHop() *overlay.OverlayAddr

ditto

go/lib/snet/router.go, line 41 at r1 (raw file):

type Path interface {
	OverlayNextHop() *overlay.OverlayAddr
	// Path returns a raw (data-plane compatible) representation of the path.

This should specify whether the path is initialized

go/lib/snet/router.go, line 59 at r1 (raw file):

// BuildAppAddress returns a public address for the local machine. The port is
// set to 0.
func (m *LocalMachine) BuildAppAddress() *addr.AppAddr {

I think the idiomatic way is calling this method AppAddress

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

// BuildBindAddress returns a bind address for the local machine. The port is
// set to 0.
func (m *LocalMachine) BuildBindAddress() *overlay.OverlayAddr {

ditto

go/lib/snet/router_test.go, line 53 at r1 (raw file):

			},
			{
				Description: "if have public IP, it is used to construct app address",

"if public IP (is) set,..."

go/lib/snet/router_test.go, line 100 at r1 (raw file):

}

func MustNewOverlayAddr(l3 addr.HostAddr, l4 addr.L4Info) *overlay.OverlayAddr {

no need to make public.

go/lib/svc/resolver.go, line 63 at r1 (raw file):

	}
	defer conn.Close()
	defer ctxconn.CloseConnOnDone(ctx, conn)()

Why defer this call?
This sets the read deadline (if any) after the the call has finished.
Also, the connection is closed after the

go/lib/svc/resolver.go, line 93 at r1 (raw file):

// RoundTripper does a single SVC resolution request/reply interaction over a
// connection, using the specified request packet and overlay address.
type RoundTripper interface {

This seems to mimic the net/http.RoundTripper, however it is pretty different in its behavior.
I.e. packet generation is handled in the caller, and not in the "Transport" etc

I think RoundTrip could easily also take context, elevating the need for ctxconn.
Afaict, context is passed through the request for historical reasons.

go/lib/svc/resolver_test.go, line 165 at r1 (raw file):

				c.EXPECT().ReadFrom(gomock.Any(), gomock.Any()).DoAndReturn(
					func(pkt *snet.SCIONPacket, _ *overlay.OverlayAddr) error {
						pkt.Payload = common.RawBytes{42}

💯

go/lib/svc/internal/ctxconn/ctxconn.go, line 15 at r1 (raw file):

// limitations under the License.

// Package ctxconn provides a helper function to track context cancelation when

s/cancelation/cancellation

go/lib/svc/internal/ctxconn/ctxconn.go, line 40 at r1 (raw file):

//
// Call the returned cancelation function to free up resources.
func CloseConnOnDone(ctx context.Context, conn DeadlineCloser) CancelFunc {

Why do we need this?
The only value I see here is setting the deadline the same as the context, but that could easily be done by the caller, or with another helper function.

go/lib/svc/internal/ctxconn/ctxconn.go, line 63 at r1 (raw file):

		case <-cancelSignal:
		default:
			close(cancelSignal)

Neat code, however, it is not safe against double closures when called from different go routines:
This should be mentioned in the doc.

package main

func main() {
	for i := 0; i < 100; i++ {
		test()
	}
}

func test() {
	c := make(chan struct{})
	for i := 0; i < 100; i++ {
		go closeChan(c)
	}
	<-c
}

func closeChan(c chan struct{}) {
	select {
	case <-c:
	default:
		close(c)
	}
}

$ go run -race main.go
panic: close of closed channel

goroutine 335 [running]:
main.closeChan(0xc420086180)
        /home/roosd/go/src/github.com/scionproto/scion/main.go:22 +0x68
created by main.test
        /home/roosd/go/src/github.com/scionproto/scion/main.go:13 +0x78
exit status 2

go/lib/svc/internal/ctxconn/ctxconn_test.go, line 36 at r1 (raw file):

		Convey("if no deadline and no ctx canceled, close is not called", func() {
			ctx := mock_context.NewMockContext(ctrl)

Why mock this?
Same could be achieved by passing context.Background()

go/lib/svc/internal/ctxconn/ctxconn_test.go, line 46 at r1 (raw file):

		})
		Convey("if no deadline and ctx canceled, close is called once", func() {
			ctx := mock_context.NewMockContext(ctrl)

Why?

ctx, cancel := context.WithCancel(context.Background())
cancel()

no mocks needed

scrye

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

go/lib/mocks/context/mock_context/context.go, line 5 at r1 (raw file):

Previously, Oncilla wrote…

I think this is taking mocking a bit to far.
I do not see the need to mock context, just use concrete values

Done.

go/lib/snet/router.go, line 32 at r1 (raw file):

Previously, Oncilla wrote…

In the spirit of road to 100 (https://goreportcard.com/report/github.com/scionproto/scion#golint)
Add a comment

I like this #roadto100 and fully support it 💯

go/lib/snet/router.go, line 40 at r1 (raw file):

Previously, Oncilla wrote…

ditto

Done.

go/lib/snet/router.go, line 41 at r1 (raw file):

Previously, Oncilla wrote…

This should specify whether the path is initialized

Oof, I keep forgetting that our data-plane paths have state. We should decouple the state from the path at some point, cause it makes a basic object very awkward to use.

Added the remark in the comment.

go/lib/snet/router.go, line 59 at r1 (raw file):

Previously, Oncilla wrote…

I think the idiomatic way is calling this method AppAddress

Done.

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

Previously, Oncilla wrote…

ditto

Done.

go/lib/snet/router_test.go, line 53 at r1 (raw file):

Previously, Oncilla wrote…

"if public IP (is) set,..."

Done.

go/lib/snet/router_test.go, line 100 at r1 (raw file):

Previously, Oncilla wrote…

no need to make public.

It's not really public, since it cannot be imported. But local scope is a bit more intuitive. Done.

go/lib/svc/resolver.go, line 63 at r1 (raw file):

Previously, Oncilla wrote…

Why defer this call?
This sets the read deadline (if any) after the the call has finished.
Also, the connection is closed after the

Hmm, there might be a confusion here regarding the order of execution; CloseConnOnDone runs first, returns a function, and only that function is deferred.

Maybe the "fancy" syntax is too confusing in general. Refactored.

The connection is closed at the end to guarantee that every goroutine that has been spawned within the lifetime of this method is closed by the time the method returns.

go/lib/svc/resolver.go, line 93 at r1 (raw file):

Previously, Oncilla wrote…

This seems to mimic the net/http.RoundTripper, however it is pretty different in its behavior.
I.e. packet generation is handled in the caller, and not in the "Transport" etc

I think RoundTrip could easily also take context, elevating the need for ctxconn.
Afaict, context is passed through the request for historical reasons.

The name is similar to the net/http.RoundTripper one, but the behavior is completely different. They both do round trips though, so I thought the name fits, even if it's not a one-to-one mapping between the two.

I initially had packet generation in the RoundTripper, but it was really annoying to pass in all the packet data.

Added the ctx to the roundtripper.

go/lib/svc/resolver_test.go, line 165 at r1 (raw file):

Previously, Oncilla wrote…

💯

the only way to payload

go/lib/svc/internal/ctxconn/ctxconn.go, line 15 at r1 (raw file):

Previously, Oncilla wrote…

s/cancelation/cancellation

Haha, I remember searching for this and found that they are both (kinda) acceptable. I think I saw some standard libs also use the one "l" version.

go/lib/svc/internal/ctxconn/ctxconn.go, line 40 at r1 (raw file):

Previously, Oncilla wrote…

Why do we need this?
The only value I see here is setting the deadline the same as the context, but that could easily be done by the caller, or with another helper function.

The tricky part is detecting a cancellation function fired, because it can happen at any time.

go/lib/svc/internal/ctxconn/ctxconn.go, line 63 at r1 (raw file):

Previously, Oncilla wrote…

Neat code, however, it is not safe against double closures when called from different go routines:
This should be mentioned in the doc.

package main

func main() {
	for i := 0; i < 100; i++ {
		test()
	}
}

func test() {
	c := make(chan struct{})
	for i := 0; i < 100; i++ {
		go closeChan(c)
	}
	<-c
}

func closeChan(c chan struct{}) {
	select {
	case <-c:
	default:
		close(c)
	}
}

$ go run -race main.go
panic: close of closed channel

goroutine 335 [running]:
main.closeChan(0xc420086180)
        /home/roosd/go/src/github.com/scionproto/scion/main.go:22 +0x68
created by main.test
        /home/roosd/go/src/github.com/scionproto/scion/main.go:13 +0x78
exit status 2

Nice catch, done.

go/lib/svc/internal/ctxconn/ctxconn_test.go, line 36 at r1 (raw file):

Previously, Oncilla wrote…

Why mock this?
Same could be achieved by passing context.Background()

Done.

go/lib/svc/internal/ctxconn/ctxconn_test.go, line 46 at r1 (raw file):

Previously, Oncilla wrote…

Why?

ctx, cancel := context.WithCancel(context.Background())
cancel()

no mocks needed

Done.

oncilla

Reviewed 1 of 20 files at r1, 9 of 9 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @scrye)

go/lib/mocks/context/mock_context/context.go, line 5 at r1 (raw file):