Add QUIC RPC server and client #2519

scrye · 2019-03-13T11:34:32Z

This change is

lukedirtwalker

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

go/lib/infra/rpc/BUILD.bazel, line 17 at r1 (raw file):

        "//go/lib/snet:go_default_library",
        "//go/proto:go_default_library",
        "@com_github_lucas_clemente_quic_go//:go_default_library",

We still v0.8.0 would it make sense to upgrade? or did nothing relevant change?

go/lib/infra/rpc/request.go, line 15 at r1 (raw file):

// limitations under the License.

package rpc

I think the package needs a doc string.

go/lib/infra/rpc/rpc.go, line 35 at r1 (raw file):

const (
	CtxTimedOutError = iota + 1

Explain what this is. It is the error code for a context timeout while writing.

go/lib/infra/rpc/rpc.go, line 61 at r1 (raw file):

		return err
	}
	for {

Is there a clean way to tear this down?

go/lib/infra/rpc/rpc.go, line 64 at r1 (raw file):

		session, err := s.listener.Accept()
		if err != nil {
			if strings.Contains(err.Error(), "use of closed network connection") {

Why do we need to return here and not for other errors?

go/lib/infra/rpc/rpc.go, line 67 at r1 (raw file):

				return err
			}
			log.Warn("server accept error", "err", err)

I think it would make sense to prefix log messages with [quic]

go/lib/infra/rpc/rpc.go, line 91 at r1 (raw file):

}

func (s *Server) Close() error {

Maybe explain what close does.

go/lib/infra/rpc/rpc.go, line 136 at r1 (raw file):

}

func (c *Client) Request(ctx context.Context, request *Request, address net.Addr) (*Reply, error) {

Add a doc. Request sends the requests and blocks until the reply is back for example.

go/lib/infra/rpc/rpc.go, line 151 at r1 (raw file):

	go func() {
		<-ctx.Done()
		stream.CancelWrite(CtxTimedOutError)

Does that work on a closed stream?

go/lib/infra/rpc/rpc.go, line 202 at r1 (raw file):

// ReplyWriter provides handlers a way to respond to requests. ReplyWriter
// keeps a connection alive for replying. To force cleanup of the connection,
// call Close.

It isn't clear to me whether I need to call or not?

go/lib/infra/rpc/rpc.go, line 226 at r1 (raw file):

	}

	err = rw.stream.Close()

Why does this also call close?

go/lib/infra/rpc/rpc_test.go, line 38 at r1 (raw file):

	err := rw.WriteReply(reply)
	if err != nil {
		log.Warn("serve error", "err", err)

I wonder if it would make sense to have a t (testing.TB) reference in handler and use xtest.FailOnErr here?

go/lib/infra/rpc/rpc_test.go, line 49 at r1 (raw file):

func TestServer(t *testing.T) {
	Convey("Given a server", t, func() {
		cliConn, srvConn := getTestUDPConns(t, 60000, 60001)

I'd do that outside the convey block to make sure it is really only executed once.

go/lib/infra/rpc/rpc_test.go, line 81 at r1 (raw file):

			err := server.ListenAndServe()
			if err != nil {
				t.Fatalf("server err = %v\n", err)

xtest.FailOnErr

go/lib/infra/rpc/rpc_test.go, line 85 at r1 (raw file):

		}()
		reply, err := client.Request(
			context.Background(),

you could do a global timeout variable like 1s or something and use a context with timeout here. It prevents the test to run longer than it needs if stuff doesn't work.

go/lib/infra/rpc/rpc_test.go, line 90 at r1 (raw file):

		)
		if err != nil {
			t.Fatalf("client err = %v\n", err)

xtest.FailOnErr

go/lib/infra/rpc/rpc_test.go, line 104 at r1 (raw file):

t testing.TB

TIL about TB. We should use that in our xtest package.

go/lib/infra/rpc/rpc_test.go, line 130 at r1 (raw file):

	srvConn, err := net.ListenUDP("udp4", &net.UDPAddr{IP: net.IP{127, 0, 0, 1}, Port: srvPort})
	if err != nil {
		t.Fatalf("server listen err = %v\n", err)

xtest.FailOnErr

go/lib/infra/rpc/rpc_test.go, line 134 at r1 (raw file):

	cliConn, err := net.ListenUDP("udp4", &net.UDPAddr{IP: net.IP{127, 0, 0, 1}, Port: cliPort})
	if err != nil {
		t.Fatalf("client listen err = %v\n", err)

xtest.FailOnErr

scrye

Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @lukedirtwalker)

go/lib/infra/rpc/BUILD.bazel, line 17 at r1 (raw file):